Re: [HACKERS] Changeset Extraction v7.7
On 2014-02-24 17:06:53 -0500, Robert Haas wrote:
- heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalXmin);
+ if (IsSystemRelation(scan-rs_rd)
+ || RelationIsAccessibleInLogicalDecoding(scan-rs_rd))
+ heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalXmin);
+ else
+ heap_page_prune_opt(scan-rs_rd, buffer,
RecentGlobalDataXmin);
Instead of changing the callers of heap_page_prune_opt() in this way,
I think it might be better to change heap_page_prune_opt() to take
only the first two of its current three parameters; everybody's just
passing RecentGlobalXmin right now anyway.
I've changed stuff this way, and it indeed looks better.
I am wondering about the related situation of GetOldestXmin()
callers. There's a fair bit of duplicated logic in the callers, before
but especially after this patchset. What about adding 'Relation rel'
parameter instead of `allDbs' and `systable'? That keeps the logic
centralized and there's been a fair amount of talk about vacuum
optimizations that could also use it.
It's a bit sad that that requires including rel.h from procarray.h...
What do you think? Isolated patch attached.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From b23fe717c699ad5e7cac217c3a5725b57c722ff2 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 26 Feb 2014 18:23:55 +0100
Subject: [PATCH] fixup! wal_decoding: Introduce logical changeset extraction.
Centralize knowledge from GetOldestXmin() callers into GetOldestXmin()
itself.
---
src/backend/access/transam/xlog.c | 4 +--
src/backend/catalog/index.c | 12 +
src/backend/commands/analyze.c| 4 +--
src/backend/commands/cluster.c| 4 +--
src/backend/commands/vacuum.c | 9 +++
src/backend/commands/vacuumlazy.c | 6 ++---
src/backend/replication/walreceiver.c | 2 +-
src/backend/storage/ipc/procarray.c | 50 +++
src/include/commands/vacuum.h | 4 +--
src/include/storage/procarray.h | 3 ++-
10 files changed, 49 insertions(+), 49 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b6b2cd4..9f48fa5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8619,7 +8619,7 @@ CreateCheckPoint(int flags)
* StartupSUBTRANS hasn't been called yet.
*/
if (!RecoveryInProgress())
- TruncateSUBTRANS(GetOldestXmin(true, false, true));
+ TruncateSUBTRANS(GetOldestXmin(NULL, false));
/* Real work is done, but log and update stats before releasing lock. */
LogCheckpointEnd(false);
@@ -8997,7 +8997,7 @@ CreateRestartPoint(int flags)
* this because StartupSUBTRANS hasn't been called yet.
*/
if (EnableHotStandby)
- TruncateSUBTRANS(GetOldestXmin(true, false, true));
+ TruncateSUBTRANS(GetOldestXmin(NULL, false));
/* Real work is done, but log and update before releasing lock. */
LogCheckpointEnd(true);
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8c1803e..877d767 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2154,19 +2154,9 @@ IndexBuildHeapScan(Relation heapRelation,
}
else
{
- /*
- * We can ignore a) pegged xmins b) shared relations if we don't scan
- * something acting as a catalog.
- */
- bool include_systables =
- IsCatalogRelation(heapRelation) ||
- RelationIsAccessibleInLogicalDecoding(heapRelation);
-
snapshot = SnapshotAny;
/* okay to ignore lazy VACUUMs here */
- OldestXmin = GetOldestXmin(heapRelation-rd_rel-relisshared,
- true,
- include_systables);
+ OldestXmin = GetOldestXmin(heapRelation, true);
}
scan = heap_beginscan_strat(heapRelation, /* relation */
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index fe569f5..a04adea 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1082,9 +1082,7 @@ acquire_sample_rows(Relation onerel, int elevel,
totalblocks = RelationGetNumberOfBlocks(onerel);
/* Need a cutoff xmin for HeapTupleSatisfiesVacuum */
- OldestXmin = GetOldestXmin(onerel-rd_rel-relisshared, true,
- IsCatalogRelation(onerel) ||
- RelationIsAccessibleInLogicalDecoding(onerel));
+ OldestXmin = GetOldestXmin(onerel, true);
/* Prepare for sampling block numbers */
BlockSampler_Init(bs, totalblocks, targrows);
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index fc5c013..b6b40e7 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -850,9 +850,7 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose,
* Since we're going to rewrite the whole table anyway, there's no reason
* not to be aggressive about this.
*/
- vacuum_set_xid_limits(0, 0,
Re: [HACKERS] Changeset Extraction v7.7
Andres Freund escribió: I am wondering about the related situation of GetOldestXmin() callers. There's a fair bit of duplicated logic in the callers, before but especially after this patchset. What about adding 'Relation rel' parameter instead of `allDbs' and `systable'? That keeps the logic centralized and there's been a fair amount of talk about vacuum optimizations that could also use it. It's a bit sad that that requires including rel.h from procarray.h... relcache.h, not rel.h. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changeset Extraction v7.7
On 2014-02-26 15:30:55 -0300, Alvaro Herrera wrote: Andres Freund escribió: I am wondering about the related situation of GetOldestXmin() callers. There's a fair bit of duplicated logic in the callers, before but especially after this patchset. What about adding 'Relation rel' parameter instead of `allDbs' and `systable'? That keeps the logic centralized and there's been a fair amount of talk about vacuum optimizations that could also use it. It's a bit sad that that requires including rel.h from procarray.h... relcache.h, not rel.h. RelationData is declared in rel.h, not relcache.h, no? Alternatively we could just forward declare it in the header... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changeset Extraction v7.7
Andres Freund escribió: On 2014-02-26 15:30:55 -0300, Alvaro Herrera wrote: Andres Freund escribió: I am wondering about the related situation of GetOldestXmin() callers. There's a fair bit of duplicated logic in the callers, before but especially after this patchset. What about adding 'Relation rel' parameter instead of `allDbs' and `systable'? That keeps the logic centralized and there's been a fair amount of talk about vacuum optimizations that could also use it. It's a bit sad that that requires including rel.h from procarray.h... relcache.h, not rel.h. RelationData is declared in rel.h, not relcache.h, no? Sure, but with your patch AFAICT procarray.h header only needs Relation, which is declared in relcache.h. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changeset Extraction v7.7
On Wed, Feb 26, 2014 at 12:29 PM, Andres Freund and...@2ndquadrant.com wrote: On 2014-02-24 17:06:53 -0500, Robert Haas wrote: - heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalXmin); + if (IsSystemRelation(scan-rs_rd) + || RelationIsAccessibleInLogicalDecoding(scan-rs_rd)) + heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalXmin); + else + heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalDataXmin); Instead of changing the callers of heap_page_prune_opt() in this way, I think it might be better to change heap_page_prune_opt() to take only the first two of its current three parameters; everybody's just passing RecentGlobalXmin right now anyway. I've changed stuff this way, and it indeed looks better. I am wondering about the related situation of GetOldestXmin() callers. There's a fair bit of duplicated logic in the callers, before but especially after this patchset. What about adding 'Relation rel' parameter instead of `allDbs' and `systable'? That keeps the logic centralized and there's been a fair amount of talk about vacuum optimizations that could also use it. It's a bit sad that that requires including rel.h from procarray.h... What do you think? Isolated patch attached. Seems reasonable to me. + * considered, but for non-shared non-shared relations that's not required, Duplicate word. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changeset Extraction v7.7
On Mon, Feb 24, 2014 at 6:16 PM, Andres Freund and...@2ndquadrant.com wrote: I actually thought they'd be too ugly to live and we'd remove them pre-commit. Might be getting to be about that time, then. - if (nrels 0 || nmsgs 0 || RelcacheInitFileInval || forceSyncCommit) + if (nrels 0 || nmsgs 0 || RelcacheInitFileInval || forceSyncCommit || + XLogLogicalInfoActive()) Mmph. Is this really necessary? If so, why? The comments could elucidate. We could get rid of it by (optionally) adding information about the database oid to compact commit, but that'd increase the size of the record. So why do we need the database OID? + bool fail_softly = slot-data.persistency == RS_EPHEMERAL; This should be contingent on whether we're being called in the error pathway, not the slot type. I think you should pass a bool. Why? I had it that way at first, but for persistent slots this won't be called in error pathways as we won't drop there. I was thinking more the reverse - that a non-persistent slot might be dropped in a non-error pathway. It seems to be indicating, roughly, whether the relation should participate in RecentGlobalXmin or RecentGlobalDataXmin. But is there any point at all of separating those when !XLogLogicalInfoActive()? The test expands to: IsSystemRelation() || (XLogLogicalInfoActive() RelationNeedsWAL(relation) (IsCatalogRelation(relation) || RelationIsUsedAsCatalogTable(relation))) So basically this is all tables created in pg_catalog during initdb plus all TOAST tables in the system. If wal_level=logical, then we also include tables marked with the reloption user_catalog_table=true, unless they're unlogged. This all seems a bit complex. Why not this: IsSystemRelation() || || RelationIsUsedAsCatalogTable(relation) Because that'd possibly retain too much when !XLogLogicalInfoActive(), there's no need to look for RelationIsUsedAsCatalogTable() for those. We could decide not to care? But when !XLogLogicalInfoActive() I think we could just make this always false, right? I mean, if PROC_IN_LOGICAL_DECODING is never going to be set, the values are always going to be the same anyway. I think. + /* +* This is a bit tricky: We need to determine a safe xmin horizon to start +* decoding from, to avoid starting from a running xacts record referring +* to xids whose rows have been vacuumed or pruned +* already. GetOldestSafeDecodingTransactionId() returns such a value, but +* without further interlock it's return value might immediately be out of +* date. +* +* So we have to acquire the ProcArrayLock to prevent computation of new +* xmin horizons by other backends, get the safe decoding xid, and inform +* the slot machinery about the new limit. Once that's done the +* ProcArrayLock can be be released as the slot machinery now is +* protecting against vacuum. +* +*/ I can't claim to be very excited about this. Because of the already_locked parameters, or any wider concerns? Passing down already_locked through several layers is kind of ugly, but also, holding ProcArrayLock more is sad. That is not a lightly-contended lock. I'm assuming you've spent a lot of time thinking about ways to avoid this and utterly failed to come up with any reasonable alternative, but let me take a shot. Suppose we take ProcArrayLock in exclusive mode and compute the oldest running XID, install it as our xmin, and then release ProcArrayLock. At that point, nobody else can compute an oldest-xmin value that precedes that value, so we can take our time installing that value as the slot's xmin, without needing to hold a lock meanwhile. I actually had it that way for a while, but what if the backend already has a xmin set? Then we need to reason about whether the xmin is newer, restore it afterwards and such. That doesn't seem nice. It's not too far removed from the problem snapmgr.c is already designed to solve, though, is it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changeset Extraction v7.7
Hi, On 2014-02-25 13:47:49 -0500, Robert Haas wrote: On Mon, Feb 24, 2014 at 6:16 PM, Andres Freund and...@2ndquadrant.com wrote: I actually thought they'd be too ugly to live and we'd remove them pre-commit. Might be getting to be about that time, then. I want to leave them in until the slot semantics aren't going to change anymore, they are pretty useful for testing that. But I'll separate them out into a separate commit again. - if (nrels 0 || nmsgs 0 || RelcacheInitFileInval || forceSyncCommit) + if (nrels 0 || nmsgs 0 || RelcacheInitFileInval || forceSyncCommit || + XLogLogicalInfoActive()) Mmph. Is this really necessary? If so, why? The comments could elucidate. We could get rid of it by (optionally) adding information about the database oid to compact commit, but that'd increase the size of the record. So why do we need the database OID? To ignore commits from other databases. Since we don't decode changes from other databases, it's really confusing (and pointless overhead) to see transactions from there. + bool fail_softly = slot-data.persistency == RS_EPHEMERAL; This should be contingent on whether we're being called in the error pathway, not the slot type. I think you should pass a bool. Why? I had it that way at first, but for persistent slots this won't be called in error pathways as we won't drop there. I was thinking more the reverse - that a non-persistent slot might be dropped in a non-error pathway. Well, currently EPHEMERAL slots are documented to be dropped at release since that's what changeset extraction (and possibly basebackup and receivexlog) need afaics. You'd prefer DROP_ON_ERROR semantics? It seems to be indicating, roughly, whether the relation should participate in RecentGlobalXmin or RecentGlobalDataXmin. But is there any point at all of separating those when !XLogLogicalInfoActive()? The test expands to: IsSystemRelation() || (XLogLogicalInfoActive() RelationNeedsWAL(relation) (IsCatalogRelation(relation) || RelationIsUsedAsCatalogTable(relation))) So basically this is all tables created in pg_catalog during initdb plus all TOAST tables in the system. If wal_level=logical, then we also include tables marked with the reloption user_catalog_table=true, unless they're unlogged. This all seems a bit complex. Why not this: IsSystemRelation() || || RelationIsUsedAsCatalogTable(relation) Because that'd possibly retain too much when !XLogLogicalInfoActive(), there's no need to look for RelationIsUsedAsCatalogTable() for those. We could decide not to care? But when !XLogLogicalInfoActive() I think we could just make this always false, right? I mean, if PROC_IN_LOGICAL_DECODING is never going to be set, the values are always going to be the same anyway. I think. It seems confusing and bug-prone to use the wrong horizon variable just because right now they'd be the same if wal_level logical. I can't claim to be very excited about this. Because of the already_locked parameters, or any wider concerns? Passing down already_locked through several layers is kind of ugly, but also, holding ProcArrayLock more is sad. That is not a lightly-contended lock. Absolutely true, but this is very far from a operation that will be frequent enough to matter. Creating a slot so frequently that a lock on the procarray hold while iterating the slot array matters, will be painful long before the contention on that is the problem. I'm assuming you've spent a lot of time thinking about ways to avoid this and utterly failed to come up with any reasonable alternative, but let me take a shot. Suppose we take ProcArrayLock in exclusive mode and compute the oldest running XID, install it as our xmin, and then release ProcArrayLock. At that point, nobody else can compute an oldest-xmin value that precedes that value, so we can take our time installing that value as the slot's xmin, without needing to hold a lock meanwhile. I actually had it that way for a while, but what if the backend already has a xmin set? Then we need to reason about whether the xmin is newer, restore it afterwards and such. That doesn't seem nice. It's not too far removed from the problem snapmgr.c is already designed to solve, though, is it? Hm, I don't immediately see how it would fit in there. PgXact-xmin is set by procarray.c, all snapmgr does is reset it. And there's no logic about resetting it back to higher values and such. I'll ponder on getting rid of this, but I am not of too high hopes. Thanks, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changeset Extraction v7.7
On Mon, Feb 24, 2014 at 10:11 AM, Andres Freund and...@2ndquadrant.com wrote: Changes in this version include: * changed slot error handling log by introducing ephermal slots which get dropped on errors. This is the biggest change. * added quoting in the test_decoding output plugin * closing of a tight race condition during slot creation where WAL could have been removed * comment and other adjustments, many of them noticed by robert I did another read-through of this this afternoon, focusing on the stuff you changed and parts I hadn't looked at carefully yet. Comments below. Documentation needs to be updated for pg_stat_replication view. I still think pg_create_logical_replication_slot should be in slotfuncs.c. /* Size of an indirect datum that contains an indirect TOAST pointer */ #define INDIRECT_POINTER_SIZE (VARHDRSZ_EXTERNAL + sizeof(struct varatt_indirect)) +/* Size of an indirect datum that contains a standard TOAST pointer */ +#define INDIRECT_POINTER_SIZE (VARHDRSZ_EXTERNAL + sizeof(struct varatt_indirect)) Isn't the new hunk a duplicate of the existing definition, except for a one-word change to the comment? I don't think the completely-unsecured directory operations in test_decoding_regsupport.c are acceptable. Tom fought tooth and nail to make sure that similar capabilities in adminpack carried meaningful security restrictions. /* +* Check whether there are, possibly unconnected, logical slots that refer +* to the to-be-dropped database. The database lock we are holding +* prevents the creation of new slots using the database. +*/ + if (ReplicationSlotsCountDBSlots(db_id, nslots, nslots_active)) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_IN_USE), +errmsg(database \%s\ is used in a logical decoding slot, + dbname), +errdetail(There are %d slot(s), %d of them active, + nslots, nslots_active))); What are you going to do when we get around to supporting this on a standby? Whatever the answer is, maybe add a TODO comment. + * loop for now.. +* more than twice.. Extra periods. +* The replicatio slot mechanism is used to prevent removal of required Typo. + + /* +* GetRunningTransactionData() acquired ProcArrayLock, we must release +* it. We can do that before inserting the WAL record because +* ProcArrayApplyRecoveryInfo can recheck the commit status using the +* clog. If we're doing logical replication we can't do that though, so +* hold the lock for a moment longer. +*/ + if (wal_level WAL_LEVEL_LOGICAL) + LWLockRelease(ProcArrayLock); + recptr = LogCurrentRunningXacts(running); + /* Release lock if we kept it longer ... */ + if (wal_level = WAL_LEVEL_LOGICAL) + LWLockRelease(ProcArrayLock); + This seems unfortunate. The comment should clearly explain why it's necessary. + /* +* Startup logical state, needs to be setup now so we have proper data +* during restore. +*/ + StartupReorderBuffer(); Should add blank line before this. + CheckPointSnapBuild(); + CheckpointLogicalRewriteHeap(); Shouldn't the capitalization be consistent? - heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalXmin); + if (IsSystemRelation(scan-rs_rd) + || RelationIsAccessibleInLogicalDecoding(scan-rs_rd)) + heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalXmin); + else + heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalDataXmin); Instead of changing the callers of heap_page_prune_opt() in this way, I think it might be better to change heap_page_prune_opt() to take only the first two of its current three parameters; everybody's just passing RecentGlobalXmin right now anyway. Then, we could change the first check in heap_page_prune_opt() to check first whether PageIsPrunable(page, RecentGlobalDataXmin). If not, give up. If so, then check that (!IsSystemRelation(scan-rs_rd) !RelationIsAccessibleInLogicalDecoding(scan-rs_rd)) || PageIsPrunable(page, RecentGlobalXmin)). The advantage of this is that we avoid code duplication, and we avoid checking a couple of conditions if pd_prune_xmin is very recent. - if (nrels 0 || nmsgs 0 || RelcacheInitFileInval || forceSyncCommit) + if (nrels 0 || nmsgs 0 || RelcacheInitFileInval || forceSyncCommit || + XLogLogicalInfoActive()) Mmph. Is this really necessary? If so, why? The comments could elucidate. + bool fail_softly = slot-data.persistency == RS_EPHEMERAL; This should be contingent on whether we're being called in the error pathway, not the slot type. I think you should pass a bool.
Re: [HACKERS] Changeset Extraction v7.7
Hi, On 2014-02-24 17:06:53 -0500, Robert Haas wrote: I still think pg_create_logical_replication_slot should be in slotfuncs.c. Ok, I don't feel too strongly, so I can change it. I wanted to keep logical/ stuff out of slotfuncs.c, but there's not really a strong reason for that. I don't think the completely-unsecured directory operations in test_decoding_regsupport.c are acceptable. Tom fought tooth and nail to make sure that similar capabilities in adminpack carried meaningful security restrictions. I actually thought they'd be too ugly to live and we'd remove them pre-commit. There's no security problem though afaics, since they aren't actually created, and you need to be superuser to create C functions. /* +* Check whether there are, possibly unconnected, logical slots that refer +* to the to-be-dropped database. The database lock we are holding +* prevents the creation of new slots using the database. +*/ + if (ReplicationSlotsCountDBSlots(db_id, nslots, nslots_active)) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_IN_USE), +errmsg(database \%s\ is used in a logical decoding slot, + dbname), +errdetail(There are %d slot(s), %d of them active, + nslots, nslots_active))); What are you going to do when we get around to supporting this on a standby? Whatever the answer is, maybe add a TODO comment. I think it should actually mostly work out, anybody actively connected to a slot will be kicked of (normal HS mechanisms)... But the slot would currently live on which obviously isn't nice. Will add TODO. + /* +* GetRunningTransactionData() acquired ProcArrayLock, we must release +* it. We can do that before inserting the WAL record because +* ProcArrayApplyRecoveryInfo can recheck the commit status using the +* clog. If we're doing logical replication we can't do that though, so +* hold the lock for a moment longer. +*/ + if (wal_level WAL_LEVEL_LOGICAL) + LWLockRelease(ProcArrayLock); + recptr = LogCurrentRunningXacts(running); + /* Release lock if we kept it longer ... */ + if (wal_level = WAL_LEVEL_LOGICAL) + LWLockRelease(ProcArrayLock); + This seems unfortunate. The comment should clearly explain why it's necessary. There's another (existing) comment ontop of the function giving a bit more context, but I'll expand. I'd actually prefer to remove that special case alltogether, I don't have much trust in those codepaths for HS... But that's not an argument I want to fight out right nwo. - heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalXmin); + if (IsSystemRelation(scan-rs_rd) + || RelationIsAccessibleInLogicalDecoding(scan-rs_rd)) + heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalXmin); + else + heap_page_prune_opt(scan-rs_rd, buffer, RecentGlobalDataXmin); Instead of changing the callers of heap_page_prune_opt() in this way, I think it might be better to change heap_page_prune_opt() to take only the first two of its current three parameters; everybody's just passing RecentGlobalXmin right now anyway. Sounds like a plan. - if (nrels 0 || nmsgs 0 || RelcacheInitFileInval || forceSyncCommit) + if (nrels 0 || nmsgs 0 || RelcacheInitFileInval || forceSyncCommit || + XLogLogicalInfoActive()) Mmph. Is this really necessary? If so, why? The comments could elucidate. We could get rid of it by (optionally) adding information about the database oid to compact commit, but that'd increase the size of the record. + bool fail_softly = slot-data.persistency == RS_EPHEMERAL; This should be contingent on whether we're being called in the error pathway, not the slot type. I think you should pass a bool. Why? I had it that way at first, but for persistent slots this won't be called in error pathways as we won't drop there. There are a bunch of places where you're testing IsSystemRelation() || RelationIsAccessibleInLogicalDecoding(). Maybe we need a macro encapsulating that test, with a name chose to explain the point of it. Sounds like a idea. It seems to be indicating, roughly, whether the relation should participate in RecentGlobalXmin or RecentGlobalDataXmin. But is there any point at all of separating those when !XLogLogicalInfoActive()? The test expands to: IsSystemRelation() || (XLogLogicalInfoActive() RelationNeedsWAL(relation) (IsCatalogRelation(relation) || RelationIsUsedAsCatalogTable(relation))) So basically this is all tables created in pg_catalog during initdb plus all TOAST tables in the system. If
