On Sun, Jul 12, 2026 at 1:26 PM Bharath Rupireddy
<[email protected]> wrote:
>
> Hi,
>
> On Thu, Jul 9, 2026 at 2:53 PM Masahiko Sawada <[email protected]> wrote:
> >
> > > I partially agree with your suggestion for case 2. My preference is to
> > > not add any blockers for vacuum. It does opportunistic XID-age
> > > invalidation, invalidates the slots it can take without waiting, and
> > > leaves the held ones to the checkpointer, which is the guaranteed
> > > path. It is also easier to reason about and to explain to users.
> >
> > > The tradeoff is that vacuum won't invalidate a held slot in the same
> > > pass, so the relation being vacuumed right then doesn't get the
> > > advanced horizon. Once the checkpointer has invalidated the slot,
> > > later relations pick it up, which may well be in the same vacuum cycle
> > > or next. I am fine with that.
> >
> > > I would skip having the vacuum explicitly wake the checkpointer when
> > > it sees a held slot. Imagine one walsender holding the slot's xmin and
> > > 8 autovacuum workers running. Each worker hits that same held slot and
> > > would signal the checkpointer, so we would get a signal per worker for
> > > a single slot, repeated every cycle until the checkpointer acts. If
> > > the checkpointer is already running, another request gets queued, and
> > > this can happen repeatedly, creating a flood of checkpoint requests.
> >
> > I agree that we should avoid requesting checkpoints just to invalidate
> > XID-aged slots.
> >
> > If we rely on the checkpointer for held slot cases, XID-aged slots
> > held by someone could be left for up to one day, the maximum value of
> > checkpoint_timeout, in the worst case.
> >
> > Given that it's not common for XID-aged slots to
> > be still held by someone, it would be okay to skip the held slots. The
> > same applies on replicas, where the restartpoint pass is the only
> > mechanism anway. So I agree to simply skip held slots.
>
> Thank you! Please find the attached v12 patch with this change. I
> dropped the 0002 patch that was pushing the tests to reach a
> production-like XID wraparound since it takes a bit of time to run
> such tests and I understand that our test infrastructure and resources
> are not free. I folded the tests into simpler ones in 0001 itself.
> Tests now cover vacuum, autovacuum invalidating unheld slots, skipping
> held slots, checkpoint and restartpoint doing the invalidation, and
> both logical and physical replication slots. I believe this gives good
> coverage for the feature.
>
Thank you for updating the patch! I've reviewed the patch and here are comments:
---
Autovacuum workers keep holding MyReplicationSlot even after raising
an error. They recover from error state and continue to the next
table, so it would keep holding it until
---
@@ -2124,7 +2198,8 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
ReportSlotInvalidation(invalidation_cause, true, active_pid,
slotname, restart_lsn,
oldestLSN, snapshotConflictHorizon,
- slot_idle_secs);
+ slot_idle_secs, s->data.xmin,
+ s->data.catalog_xmin, xidLimit);
if (MyBackendType == B_STARTUP)
(void)
SignalRecoveryConflict(GetPGProcByNumber(active_proc),
@@ -2177,7 +2252,8 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
ReportSlotInvalidation(invalidation_cause, false, active_pid,
slotname, restart_lsn,
oldestLSN, snapshotConflictHorizon,
- slot_idle_secs);
+ slot_idle_secs, s->data.xmin,
+ s->data.catalog_xmin, xidLimit);
Both changes access the slot fields without taking a spinlock.
---
+ /* translator: %s is a GUC variable name */
+ appendStringInfo(&err_detail,
+ TransactionIdIsValid(xmin)
+ ? _("The slot's xmin age of %d
exceeds the configured \"%s\" of %d by %d transactions")
+ : _("The slot's catalog xmin age of
%d exceeds the configured \"%s\" of %d by %d transactions"),
+ slot_age, "max_slot_xid_age",
max_slot_xid_age, exceeded_by);
Error detail messages should be full stop.
I think that having the translator comment doesn't work for the actual
messages two lines later.
---
+$primary5->append_conf(
+ 'postgresql.conf', qq{
+max_slot_xid_age = $max_slot_xid_age
+autovacuum = off
+});
Most newly added regression tests could fail if the checkpointer
invalidates the XID-aged slot before autovacuum does. I think we
should set checkpoint_timeout = 1h to avoid unpredictability.
---
+# A catalog table's OldestXmin includes the slot's catalog_xmin, so vacuum
+# invalidates the unheld slot.
+$primary5->safe_psql('postgres', "VACUUM pg_class");
+wait_for_xid_aged_invalidation($primary5, 'lsub5_slot');
Why does it need to wait for the slot to be invalidated even though
VACUUM pg_class synchronously invalidates it?
---
vacuum_get_cutoffs() write WARNING "cutoff for removin and freezing
tuples ...", and it could be logged twice due to this patch.
---
It's better to have an assertion in
InvalidateObsoleteReplicationSlots() for (possible_cuases &
RS_INVAL_XID_AGE) cases.
---
+ if (slot_xmin)
+ *slot_xmin = horizons.slot_xmin;
+ if (slot_catalog_xmin)
+ *slot_catalog_xmin = horizons.slot_catalog_xmin;
These NULL checks for slot_xmin and slot_catalog_xmin seem not necessary.
---
maintenance.sgml should be updated to mention about the XID-age based
slot invalidation. I think
---
#max_slot_wal_keep_size = -1 # in megabytes; -1 disables
#idle_replication_slot_timeout = 0 # in seconds; 0 disables
+#max_slot_xid_age = 0 # maximum XID age before a replication slot
+ # gets invalidated; 0 disables
#wal_sender_timeout = 60s # in milliseconds; 0 disables
How about rewriting the description to "in transaction age; 0
disables" to match similar GUC parameters?
---
How about splitting this patch into two parts? 1. introduce
max_slot_xid_age and let the checkpointer invalidate slots for this
reason and 2. introduce invalidation path to vacuum logic.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
