Re: Introduce XID age based replication slot invalidation

Hi,

On Wed, Aug 5, 2026 at 9:53 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Thank you for updating the patch! I've reviewed the patch and here are comments:

Thanks for reviewing.

> ---
> 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

Any error after the slot is acquired but before it is released in
InvalidatePossiblyObsoleteSlot() leaks the slot. Today the only such
error is ReplicationSlotSave() failing on the disk write or fsync, so
it is rare. I still want to fix it for autovacuum, because the worker
recovers and moves to the next table, so it would carry the leaked
slot forward. The v13 patch releases the slot in do_autovacuum()'s PG
try-catch block (a regular backend is already covered by the sigsetjmp
handler in PostgresMain()). The same leak can happen for the
checkpointer and the startup process too, but I would prefer to fix
those separately if others think it is worth it.

> ---
> - slot_idle_secs);
> + slot_idle_secs, s->data.xmin,
> + s->data.catalog_xmin, xidLimit);
>
> if (MyBackendType == B_STARTUP)
> (void)
>
> Both changes access the slot fields without taking a spinlock.

Fixed.

> ---
> + /* 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.

Fixed.

> ---
> +$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.

Fixed.

> ---
> +# 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?

Fixed.

> ---
> vacuum_get_cutoffs() write WARNING "cutoff for removin and freezing
> tuples ...", and it could be logged twice due to this patch.

If the first cutoff calculation emitted the warning because a
replication slot was the reason, and the xid-aged slot then gets
invalidated, the second cutoff calculation would not emit the warning
at all.

However, in scenarios where the slot is holding back the xmin and then
a prepared transaction turns out to be the next thing holding it back,
the warning could appear again immediately even after the xid-aged
slot gets invalidated. I think the warning appearing twice is fine. It
won't flood the server logs, and even if it did, this happens at a
stage where immediate action is expected to avoid XID wraparound.

I lean towards not adding additional code complexity in the vacuum
cutoff calculation code. Thoughts?

> ---
> It's better to have an assertion in
> InvalidateObsoleteReplicationSlots() for (possible_cuases &
> RS_INVAL_XID_AGE) cases.

Added.

> ---
> + 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.

Fixed.

> ---
> maintenance.sgml should be updated to mention about the XID-age based
> slot invalidation. I think

Added.

> ---
> #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?

Reworded.

> ---
> 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.

Works for me. Done.

Thanks for the off-list discussion on not invalidating logical slots
when vacuuming non-catalog tables. I implemented this in 0002 and
added some tests. A logical slot's xmin does not block vacuum on user
tables, and invalidating it there does not help that vacuum advance
the xmin horizon. The slot still has a chance to move forward before
autovacuum runs on catalog tables or the checkpointer runs.

Please find the attached v13 patches. I addressed all the above review comments.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com