MIME-Version: 1.0
References: 
 <CA+-JvFsMHckBMzsu5Ov9HCG3AFbMh056hHy1FiXazBRtZ9pFBg@mail.gmail.com>
 <OSCPR01MB149667506BCFD684FEFDCB919F511A@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <CALj2ACUmPbkcj4y4oeXvzUkBejG68QDtrFF7QHDC_qz2vQcTCg@mail.gmail.com>
 <CAHg+QDfnK7tQxsEZox=kOkYfqANmL70mwn0N=eRrJxE1Z+1ygg@mail.gmail.com>
 <CALj2ACX_o+dKeAaK76mpAtG646UnDHpGUWziUkCvicVz8mz6=A@mail.gmail.com>
 <CAD21AoATM=Un8ejnbcDQ7q+CaCoxpkA7Ln9bacvQEoymqvjPug@mail.gmail.com>
 <CALj2ACUmUb=CLEyfsQrW0WAkF6Y9fiBfG6pnPjepfOM7A1XReA@mail.gmail.com>
In-Reply-To: 
 <CALj2ACUmUb=CLEyfsQrW0WAkF6Y9fiBfG6pnPjepfOM7A1XReA@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 24 Mar 2026 23:50:13 -0700
Message-ID: 
 <CAD21AoAg6x57a8LoP2s+0vgizp9QGHcLGJL9bwh7kzEJq3arBg@mail.gmail.com>
Subject: Re: Introduce XID age based replication slot invalidation
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 John H <johnhyvr@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAD21AoAg6x57a8LoP2s%2B0vgizp9QGHcLGJL9bwh7kzEJq3arBg%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 24, 2026 at 2:42=E2=80=AFPM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> On Mon, Mar 23, 2026 at 4:36=E2=80=AFPM Masahiko Sawada <sawada.mshk@gmai=
l.com> wrote:
> >
> > I've studied the discussion on this thread and the patch. I understand
> > the purpose of this feature and agree that it's useful especially in
> > cases where orphaned (physical or logical) replication slots prevent
> > the xmin from advancing and inactive_since based slot invalidation
> > might not fit.
> >
> > And +1 for treating both the slot's xmin and catalog_xmin similarly
> > with the single GUC.
>
> Thanks for reviewing the patch.
>
> > > >> I made the following design choice: try invalidating only once per
> > > >> vacuum cycle, not per table. While this keeps the cost of checking
> > > >> (incl. the XidGenLock contention) for invalidation to a minimum wh=
en
> > > >> there are a large number of tables and replication slots, it can b=
e
> > > >> less effective when individual tables/indexes are large. Invalidat=
ing
> > > >> during checkpoints can help to some extent with the large table/in=
dex
> > > >> cases. But I'm open to thoughts on this.
> > > >
> > > > It may not solve the intent when the vacuum cycle is longer, which =
one can expect on a large database particularly when there is heavy bloat.
> > >
> > > This design choice boils down to the following: a database instance
> > > having either 1/ a large number of small tables or 2/ large tables.
> > > From my experience, I have seen both cases but mostly case 2 (others
> > > can correct me). In this context, having an XID age based slot
> > > invalidation check once per relation makes sense. However, I'm open t=
o
> > > more thoughts here.
> >
> > ISTM that checking the XID-based slot invalidation per table would be
> > more bullet-proof and cover many cases. How about checking the
> > XID-based slot invalidation opportunity only when the OldestXmin is
> > older than the new GUC? For example, we can do this check in
> > heap_vacuum_rel() based on the VacuumCutoffs returned by
> > vacuum_get_cutoffs(). If we invalidate at least one slot for its XID,
> > we can re-compute the OldestXmin.
>
> Agreed. Here's the patch that moves the XID-age based slot
> invalidation check to vacuum_get_cutoffs. This has some nice
> advantages: 1/ It makes the check once per table (to help with large
> tables). 2/ It makes the check less costly since we rely on already
> computed OldestXmin and nextXID values. 3/ It avoids the checkpointer
> to do XID-age based slot invalidation which keeps the usage of this
> GUC simple with no additional costs to the checkpointer - just the
> vacuum (both vacuum command and autovacuum) does the invalidation when
> needed.
>
> I moved the new tests to the existing TAP test file
> t/019_replslot_limit.pl alongside other invalidation tests.
>
> I added detailed comments around InvalidateXIDAgedReplicationSlots and
> slightly modified the docs.
>
> Please find the v3 patch for further review.

Thank you for updating the patch. I think the patch is reasonably
simple and can avoid unnecessary overheads well due to XID-based
checks. Here are some comments:

+   /*
+    * Try to invalidate XID-aged replication slots that may interfere with
+    * vacuum's ability to freeze and remove dead tuples. Since OldestXmin
+    * already covers the slot xmin/catalog_xmin values, pass it as a
+    * preliminary check to avoid additional iteration over all the slots.
+    *
+    * If at least one slot was invalidated, recompute OldestXmin so that t=
his
+    * vacuum benefits from the advanced horizon immediately.
+    */
+   if (InvalidateXIDAgedReplicationSlots(cutoffs->OldestXmin, nextXID))
+   {
+       cutoffs->OldestXmin =3D GetOldestNonRemovableTransactionId(rel);
+       Assert(TransactionIdIsNormal(cutoffs->OldestXmin));
+   }

vacuum_get_cutoff() is also called by VACUUM FULL, CLUSTER, and
REPACK. I'm not sure that users would expect the slot invalidation
also in these commands. I think it's better to leave
vacuum_get_cutoff() a pure cutoff computation function and we can try
to invalidate slots in heap_vacuum_rel(). It requires additional
ReadNextTransactionId() but we can live with it, or we can make
vacuum_get_cutoffs() return the nextXID as well (stored in *cutoffs).

---
+   /* ensure it's a "normal" XID, else TransactionIdPrecedes misbehaves */
+   /* this can cause the limit to go backwards by 3, but that's OK */
+   if (!TransactionIdIsNormal(cutoffXID))
+       cutoffXID =3D FirstNormalTransactionId;
+
+   if (TransactionIdPrecedes(oldestXmin, cutoffXID))
+   {
+       invalidated =3D InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE=
,
+                                                        0,
+                                                        InvalidOid,
+                                                        InvalidTransaction=
Id,
+                                                        nextXID);
+   }

I think it's better to check the procArray->replication_slot_xmin and
procArray->replication_slot_catalog_xmin before iterating over each
slot. Otherwise, we would end up checking every slot even when a long
running transaction holds the oldestxmin back.

---
+   if (cutoffXID < FirstNormalTransactionId)
+       cutoffXID -=3D FirstNormalTransactionId;

and

+   if (!TransactionIdIsNormal(cutoffXID))
+       cutoffXID =3D FirstNormalTransactionId;

These codes have the same comment but are doing a slightly different
thing. I guess the latter is missing '-'?

Regards,

--=20
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com