MIME-Version: 1.0
References: <CAD21AoAd0BX0L17YxSCbSNbj=3V7dvLVGY0WhwLHAt724rNCZA@mail.gmail.com>
 <ZgulmKzdqxuBuhCU@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAJpy0uBthb-RG=Xg+mM-G6Xy01JLcBsizJuz+xbti08qP_W7AQ@mail.gmail.com>
 <CALj2ACVfDBcbybgyga7ia_KiQPAfEi_3=VDOGf2VU8RkJZSQpw@mail.gmail.com>
 <Zg0QgarM9UlRutLw@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACUmp-F+irHYfNhmo_9yaCo7aGi7ZB20KQcDR3S9eT56hg@mail.gmail.com>
 <Zg1WqRS/gQrLG2vl@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACUBiRLHF7VYD-0nYVHSvVBSCQ_5OMbP4tE8Ys_rpvQ--A@mail.gmail.com>
 <Zg2DgfXorAXrC8lw@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACXbUzp-9tixeUK_dws5k+ARJaXxj8cj9W3adA0XNUS4Hg@mail.gmail.com>
 <Zg+rvmqjL3DfbJdx@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACW8SYiRL8vGAhez=FdrzxO0W5pVFvN8GF7yKCO8_c=QdQ@mail.gmail.com>
 <CAA4eK1+-ipXB+LuUiDNQ5L+sWJFpzK89k-Oc7xOG37AefrptoQ@mail.gmail.com>
 <CALj2ACWhTY9mRc7sOpuao=GGidRjCVXc0SNtJ_DNman4nh5CcQ@mail.gmail.com>
 <CALj2ACVY+Fd5vC0VjW=5VDK9mmt-Y+PDZxnBp8ngGAZc24Vv9g@mail.gmail.com>
 <CALj2ACX0FVrx9f0c2g19Rb-WAJhm3wdyttde3YQtkotL6bUveA@mail.gmail.com> <CALj2ACXQNGXokgx8APwdxrG4MHMF=cOz6XQtUL7EHua9oUfkgA@mail.gmail.com>
In-Reply-To: <CALj2ACXQNGXokgx8APwdxrG4MHMF=cOz6XQtUL7EHua9oUfkgA@mail.gmail.com>
From: Ajin Cherian <itsajin@gmail.com>
Date: Wed, 14 Aug 2024 13:50:38 +1000
Message-ID: <CAFPTHDZPOc7pY4E0zJ2om0MZLMt+UuiX-VLsJyRO_aSyR8DsnA@mail.gmail.com>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, 
	Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, shveta malik <shveta.malik@gmail.com>, 
	Masahiko Sawada <sawada.mshk@gmail.com>, Nathan Bossart <nathandbossart@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000c6d1f1061f9ca1c9"
Archived-At: <https://www.postgresql.org/message-id/CAFPTHDZPOc7pY4E0zJ2om0MZLMt%2BUuiX-VLsJyRO_aSyR8DsnA%40mail.gmail.com>
Precedence: bulk

--000000000000c6d1f1061f9ca1c9
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 24, 2024 at 4:01=E2=80=AFPM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

> Hi,
>
> On Mon, Jun 17, 2024 at 5:55=E2=80=AFPM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Please find the attached v41 patches implementing the idea of vacuum
> doing the invalidation.
>
> Thoughts?
>
>
>

Some minor comments on the patch:
1.
+ /*
+ * Release the lock if it's not yet to keep the cleanup path on
+ * error happy.
+ */

I suggest rephrasing to: " "Release the lock if it hasn't been already to
ensure smooth cleanup on error."


2.

elog(DEBUG1, "performing replication slot invalidation");

Probably change it to "performing replication slot invalidation checks" as
we might not actually invalidate any slot here.

3.

In CheckPointReplicationSlots()

+ invalidated =3D
InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT,
+ 0,
+ InvalidOid,
+ InvalidTransactionId);
+
+ if (invalidated)
+ {
+ /*
+ * If any slots have been invalidated, recalculate the resource
+ * limits.
+ */
+ ReplicationSlotsComputeRequiredXmin(false);
+ ReplicationSlotsComputeRequiredLSN();
+ }

Is this calculation of resource limits really required here when the same
is already done inside InvalidateObsoleteReplicationSlots()


regards,
Ajin Cherian
Fujitsu Australia

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

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">On Mon, Jun 24, 2024 at 4:01=E2=80=AF=
PM Bharath Rupireddy &lt;<a href=3D"mailto:bharath.rupireddyforpostgres@gma=
il.com">bharath.rupireddyforpostgres@gmail.com</a>&gt; wrote:<br></div><blo=
ckquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left=
:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Mon, Jun 17, 2024 at 5:55=E2=80=AFPM Bharath Rupireddy<br>
&lt;<a href=3D"mailto:bharath.rupireddyforpostgres@gmail.com" target=3D"_bl=
ank">bharath.rupireddyforpostgres@gmail.com</a>&gt; wrote:<br>
<br>
Please find the attached v41 patches implementing the idea of vacuum<br>
doing the invalidation.<br>
<br>
Thoughts?<br>
<br><br></blockquote><div><br></div><div><br></div><div>Some minor comments=
 on the patch:<br>1.<br>+			/*<br>+			 * Release the lock if it&#39;s not y=
et to keep the cleanup path on<br>+			 * error happy.<br>+			 */<br><br>I s=
uggest rephrasing to: &quot;

&quot;Release the lock if it hasn&#39;t been already to ensure smooth clean=
up on error.&quot;<br><br><br>2.<br><br>elog(DEBUG1, &quot;performing repli=
cation slot invalidation&quot;);<br><br>Probably change it to &quot;perform=
ing replication slot invalidation checks&quot; as we might not actually inv=
alidate any slot here.<br><br>3. <br><br>In CheckPointReplicationSlots()<br=
><br>+	invalidated =3D InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE=
_TIMEOUT,<br>+													 0,<br>+													 InvalidOid,<br>+										=
			 InvalidTransactionId);<br>+<br>+	if (invalidated)<br>+	{<br>+		/*<br>+	=
	 * If any slots have been invalidated, recalculate the resource<br>+		 * l=
imits.<br>+		 */<br>+		ReplicationSlotsComputeRequiredXmin(false);<br>+		Re=
plicationSlotsComputeRequiredLSN();<br>+	}<br><br>Is this calculation of re=
source limits really required here when the same is already done inside Inv=
alidateObsoleteReplicationSlots()<br><br><br></div><div>regards,</div><div>=
Ajin Cherian</div><div>Fujitsu Australia<br></div><div><br><br><br> </div><=
/div></div>

--000000000000c6d1f1061f9ca1c9--