MIME-Version: 1.0
References: 
 <CA+-JvFsMHckBMzsu5Ov9HCG3AFbMh056hHy1FiXazBRtZ9pFBg@mail.gmail.com>
 <OSCPR01MB149667506BCFD684FEFDCB919F511A@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <CALj2ACUmPbkcj4y4oeXvzUkBejG68QDtrFF7QHDC_qz2vQcTCg@mail.gmail.com>
In-Reply-To: 
 <CALj2ACUmPbkcj4y4oeXvzUkBejG68QDtrFF7QHDC_qz2vQcTCg@mail.gmail.com>
From: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Date: Fri, 20 Mar 2026 23:28:57 -0700
Message-ID: 
 <CAHg+QDfnK7tQxsEZox=kOkYfqANmL70mwn0N=eRrJxE1Z+1ygg@mail.gmail.com>
Subject: Re: Introduce XID age based replication slot invalidation
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 John H <johnhyvr@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000003ec3b4064d82eb63"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHg%2BQDfnK7tQxsEZox%3DkOkYfqANmL70mwn0N%3DeRrJxE1Z%2B1ygg%40mail.gmail.com>
Precedence: bulk

--0000000000003ec3b4064d82eb63
Content-Type: text/plain; charset="UTF-8"

Hi Bharath,

Do you think we need different GUCs for catalog_xmin and xmin? If table
bloat is a concern (not catalog bloat), then logical slots are not required
to invalidate unless the cluster is close to wraparound.


> 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 when
> there are a large number of tables and replication slots, it can be
> less effective when individual tables/indexes are large. Invalidating
> during checkpoints can help to some extent with the large table/index
> 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.


> Please find the attached patch for further review. I fixed the XID age
> calculation in ReplicationSlotIsXIDAged and adjusted the code
> comments.
>

I applied the patch and all the tests passed. A few comments:

@@ -495,7 +525,7 @@ vacuum(List *relations, const VacuumParams params,
BufferAccessStrategy bstrateg
    MemoryContext vac_context, bool isTopLevel)
 {
  static bool in_vacuum = false;
-
+ static bool first_time = true;

first_time variable is not self explanatory, maybe something like
try_replication_slot_invalidation and add comments that it will be set to
false after the first check?


+ if (TransactionIdIsValid(xmin))
+ appendStringInfo(&err_detail, _("The slot's xmin %u exceeds the maximum
xid age %d specified by \"max_slot_xid_age\"."),
+ xmin,
+ max_slot_xid_age);

Slot invalidates even when the age is max_slot_xid_age, isn't it?


Thanks,
Satya

--0000000000003ec3b4064d82eb63
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div>Hi Bharath,</div><div><br></div><div>Do you think we =
need different GUCs for catalog_xmin and xmin? If table bloat is a concern =
(not catalog bloat), then logical slots are not required to invalidate unle=
ss the cluster is close to wraparound.=C2=A0</div><div class=3D"gmail_quote=
 gmail_quote_container"><div><br></div><div>=C2=A0</div><blockquote class=
=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg=
b(204,204,204);padding-left:1ex">I made the following design choice: try in=
validating only once per<br>
vacuum cycle, not per table. While this keeps the cost of checking<br>
(incl. the XidGenLock contention) for invalidation to a minimum when<br>
there are a large number of tables and replication slots, it can be<br>
less effective when individual tables/indexes are large. Invalidating<br>
during checkpoints can help to some extent with the large table/index<br>
cases. But I&#39;m open to thoughts on this.<br></blockquote><div><br></div=
><div>It may not solve the intent when the vacuum cycle is longer, which on=
e can expect on a large database particularly when there is heavy bloat.</d=
iv><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0=
px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Please find the attached patch for further review. I fixed the XID age<br>
calculation in ReplicationSlotIsXIDAged and adjusted the code<br>
comments.<br></blockquote><div><br></div><div>I applied the patch and all t=
he tests passed. A few comments:=C2=A0</div><div><br></div><div>@@ -495,7 +=
525,7 @@ vacuum(List *relations, const VacuumParams params, BufferAccessStr=
ategy bstrateg<br>=C2=A0	 =C2=A0 MemoryContext vac_context, bool isTopLevel=
)<br>=C2=A0{<br>=C2=A0	static bool in_vacuum =3D false;<br>-<br>+	static bo=
ol first_time =3D true;<br></div><div><br></div><div>first_time variable is=
 not self explanatory, maybe something like try_replication_slot_invalidati=
on and add comments that=C2=A0it will be set to false after the first check=
?</div><div><br></div><div><br></div><div>+				if (TransactionIdIsValid(xmi=
n))<br>+					appendStringInfo(&amp;err_detail, _(&quot;The slot&#39;s xmin =
%u exceeds the maximum xid age %d specified by \&quot;max_slot_xid_age\&quo=
t;.&quot;),<br>+									 xmin,<br>+									 max_slot_xid_age);</div><div>=
<br></div><div>Slot invalidates even when the age is max_slot_xid_age, isn&=
#39;t it?</div><div><br></div><div><br></div><div><br></div><div>Thanks,</d=
iv><div>Satya=C2=A0</div></div></div>

--0000000000003ec3b4064d82eb63--