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>
 <CAD21AoAg6x57a8LoP2s+0vgizp9QGHcLGJL9bwh7kzEJq3arBg@mail.gmail.com>
 <CALj2ACWcaSkfMAQu3s6ZkTZuoFvVRD=DkxXbBwC33PL9+kzsqw@mail.gmail.com>
 <CAD21AoBEBqQONiZxvnUYOu814yB2tRPrmX=7KqvL+f3ae7250w@mail.gmail.com>
 <CALj2ACUenekLgjMr8x4DyuU=zKZ4eNqW9XF-1PovSctkY2VA0Q@mail.gmail.com>
 <CAFC+b6pO44=zGqwijzrcyGGTYCM51Y7zS5uQX0_nWjsxW9i3QQ@mail.gmail.com>
 <CALj2ACU=A31kqHELyaF-=2vnyed=cO2JNQt+vY92KtHLF-m8sg@mail.gmail.com>
In-Reply-To: 
 <CALj2ACU=A31kqHELyaF-=2vnyed=cO2JNQt+vY92KtHLF-m8sg@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 30 Mar 2026 17:13:08 -0700
Message-ID: 
 <CAD21AoB4MsTpG5JEkifght_tQ91VHJO_8kpsDCrG-z9qkkmN5g@mail.gmail.com>
Subject: Re: Introduce XID age based replication slot invalidation
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
	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/CAD21AoB4MsTpG5JEkifght_tQ91VHJO_8kpsDCrG-z9qkkmN5g%40mail.gmail.com>
Precedence: bulk

On Sun, Mar 29, 2026 at 6:35=E2=80=AFPM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> On Sun, Mar 29, 2026 at 1:16=E2=80=AFPM Srinath Reddy Sadipiralla
> <srinath2133@gmail.com> wrote:
> >
> > Hello,
> >
> > Thanks for the v5 patch set, I have reviewed and did initial testing on
> > v5 patch set, and it LGTM, except these
>
> Thank you for reviewing and testing. I appreciate it.
>
> > diff --git a/src/backend/replication/slot.c b/src/backend/replication/s=
lot.c
> > index 286f0f46341..c2ff7e464f0 100644
> > --- a/src/backend/replication/slot.c
> > +++ b/src/backend/replication/slot.c
> > @@ -1849,7 +1849,7 @@ ReportSlotInvalidation(ReplicationSlotInvalidatio=
nCause cause,
> >                                 else
> >                                 {
> >                                         /* translator: %s is a GUC vari=
able name */
> > -                                       appendStringInfo(&err_detail, _=
("The slot's xmin %u is %d transactions old, which exceeds the configured \=
"%s\" value of %d."),
> > +                                       appendStringInfo(&err_detail, _=
("The slot's catalog_xmin %u is %d transactions old, which exceeds the conf=
igured \"%s\" value of %d."),
> >                                                                        =
  catalog_xmin, (int32) (recentXid - catalog_xmin), "max_slot_xid_age", max=
_slot_xid_age);
> >                                 }
>
> Fixed the typo.
>
> > while testing the active slot XID age invalidation (SIGTERM path) , i
> > observed that slot got invalidated , walsender was killed because of
> > SIGTERM , then starts the infinite-retry-cycle problem where
> > walreceiver starts walsender and walsender will try to use an invalidat=
ed
> > slot and dies, will think more on this.
>
> I would like to clarify that once a slot is invalidated due to any of
> the reasons (ReplicationSlotInvalidationCause), it becomes unusable;
> the sender will error out if the receiver tries to use it. This is
> consistent with all existing slot invalidation mechanisms.
>
> Please find the attached v6 patches fixing the typo for further review.
>

I've reviewed the v6 patch. Here are some comments.

 bool
 vacuum_get_cutoffs(Relation rel, const VacuumParams params,
-                  struct VacuumCutoffs *cutoffs)
+                  struct VacuumCutoffs *cutoffs,
+                  TransactionId *slot_xmin,
+                  TransactionId *slot_catalog_xmin)

How about storing both slot_xmin and catalog_xmin into VacuumCutoffs?

---
-   if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_REMOVED |
RS_INVAL_IDLE_TIMEOUT,
+   possibleInvalidationCauses =3D RS_INVAL_WAL_REMOVED | RS_INVAL_IDLE_TIM=
EOUT |
+       RS_INVAL_XID_AGE;
+
+   if (InvalidateObsoleteReplicationSlots(possibleInvalidationCauses,
                                           _logSegNo, InvalidOid,
+                                          InvalidTransactionId,
+                                          max_slot_xid_age > 0 ?
+                                          ReadNextTransactionId() :
                                           InvalidTransactionId))

It's odd to me that we specify RS_INVAL_XID_AGE while passing
InvalidTransactionId. I think we can specify RS_INVAL_XID_AGE along
with a valid recentXId only when we'd like to check the slots based on
their XIDs.

---
+   /* Check if the slot needs to be invalidated due to max_slot_xid_age GU=
C */
+   if ((possible_causes & RS_INVAL_XID_AGE) && CanInvalidateXidAgedSlot(s)=
)
+   {
+       TransactionId xidLimit;
+
+       Assert(TransactionIdIsValid(recentXid));
+
+       xidLimit =3D TransactionIdRetreatedBy(recentXid, max_slot_xid_age);
+

I think we can avoid calculating xidLimit for every slot by
calculating it in InvalidatePossiblyObsoleteSlot() and passing it to
DetermineSlotInvalidationCause().

---
  */
 TransactionId
 GetOldestNonRemovableTransactionId(Relation rel)
+{
+   return GetOldestNonRemovableTransactionIdExt(rel, NULL, NULL);
+}
+
+/*
+ * Same as GetOldestNonRemovableTransactionId(), but also returns the
+ * replication slot xmin and catalog_xmin from the same ComputeXidHorizons=
()
+ * call.  This avoids a separate ProcArrayLock acquisition when the caller
+ * needs both values.
+ */
+TransactionId
+GetOldestNonRemovableTransactionIdExt(Relation rel,
+                                     TransactionId *slot_xmin,
+                                     TransactionId *slot_catalog_xmin)
 {

I understand that the primary reason why the patch introduces another
variant of GetOldestNonRemovableTransactionId() is to avoid extra
ProcArrayLock acquision to get replication slot xmin and catalog_xmin.
While it's not very elegant, I find that it would not be bad because
otherwise autovacuum takes extra ProcArrayLock (in shared mode) for
every table to vacuum. The ProcArrayLock is already known
high-contented lock it would be better to avoid taking it once more.
If others think differently, we can just call
ProcArrayGetReplicationSlotXmin() separately and compare them to the
limit of XID-age based slot invalidation.

Having said that, I personally don't want to add new instructions to
the existing GetOldestNonRemovableTransactionId(). I guess we might
want to make both the existing function and new function call a common
(inline) function that takes ComputeXidHorizonsResult and returns
appropriate transaction id based on the given relation .

---
+   # Do some work to advance xids
+   $node->safe_psql(
+       'postgres', qq[
+       do \$\$
+       begin
+       for i in 1..$nxids loop
+           -- use an exception block so that each iteration eats an XID
+           begin
+           insert into $table_name values (i);
+           exception
+           when division_by_zero then null;
+           end;
+       end loop;
+       end\$\$;
+   ]);

I think it's fater to use pg_current_xact_id() instead.

---
+   else
+   {
+       $node->safe_psql('postgres', "VACUUM");
+   }

We don't need to vacuum all tables here.

---
+# Configure primary with XID age settings. Set autovacuum_naptime high so
+# that the checkpointer (not vacuum) triggers the invalidation.
+my $max_slot_xid_age =3D 500;
+$primary5->append_conf(
+   'postgresql.conf', qq{
+max_slot_xid_age =3D $max_slot_xid_age
+autovacuum_naptime =3D '1h'
+});

I think that it's better to disable autovacuum than setting a large number.

---
+# Testcase end: Invalidate streaming standby's slot due to max_slot_xid_ag=
e
+# GUC (via checkpoint).

I think that we can say "physical slot" instead of standby's slot to
avoid confusion as I thought standby's slot is a slot created on the
standby at the first glance.

---
Do we have tests for invalidating slots on the standbys?

Regards,

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