MIME-Version: 1.0
References: <CALj2ACW4aUe-_uFQOjdWCEN-xXoLGhmvRFnL8SNw_TZ5nJe+aw@mail.gmail.com>
 <ZcCuTqik6hT+HQKS@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACUbp55FXw2cp_noHexQXrSLh4Cgm0=3kxz0G5WShNTDwg@mail.gmail.com>
 <ZcXXffsYbISnlnsv@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACWgACB4opnbqi=x7Hc4aqcgkXoLsh1VB+gfidXaDQNu_Q@mail.gmail.com>
 <CALj2ACWE9asmvN1B18LqfHE8uBuWGsCEP7OO5trRCxPtTPeHVA@mail.gmail.com>
 <ZdXrtXLkjvIJMYvB@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACWbR9ck1PKyqT2agd2XCEq8L=D=X1Uw566yhA9SgXspAw@mail.gmail.com>
 <20240301221108.GB3024365@nathanxps13> <CALj2ACUprrvq=R9WBnCEtguBhSp0X+krTCGDvkHyBdawyex-kA@mail.gmail.com>
 <20240303214434.GA3036597@nathanxps13> <CAA4eK1+NUeb21bYa9K5mVsFHcw-xodGLkN9QoqJXym7sD3LtZA@mail.gmail.com>
 <CALj2ACVr8ra-yp8CwiBq=Pc9Kkq64z=zDvoFxMK3K4wSDhKMmg@mail.gmail.com>
 <CAA4eK1KxsMg4bnxRZ0U3dZ3irFoR2=TfUCOFig3ccHoRPp5iWg@mail.gmail.com>
 <CALj2ACU-sJdiLfAcWd+8hnt4HSojH0UWFGnaGr9gxb2bYzYYSA@mail.gmail.com>
 <CAA4eK1++=fOpmsQKugQQRtQr2wmiintjKEhpuOch9=phxirAfg@mail.gmail.com>
 <CALj2ACVd_dizYQiZwwUfsb+hG-fhGYo_kEDq0wn_vNwQvOrZHg@mail.gmail.com>
 <CAA4eK1+02XhKht3EB=RgzQ=2zyb9Sv4=9iC=yt1j4FfVy0-5kg@mail.gmail.com> <CALj2ACVRS2LZbycpM8rZT8QynR05AtvttOHfFOhczjyqU7zZSA@mail.gmail.com>
In-Reply-To: <CALj2ACVRS2LZbycpM8rZT8QynR05AtvttOHfFOhczjyqU7zZSA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Fri, 15 Mar 2024 12:49:07 +0530
Message-ID: <CAJpy0uChxSm7pod6CQwU4z4pSvV03b4iRCy_9zb0zVbSs__qOg@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>, Nathan Bossart <nathandbossart@gmail.com>, 
	Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAJpy0uChxSm7pod6CQwU4z4pSvV03b4iRCy_9zb0zVbSs__qOg%40mail.gmail.com>
Precedence: bulk

On Thu, Mar 14, 2024 at 7:58=E2=80=AFPM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> While we wait to hear from others on this, I'm attaching the v9 patch
> set implementing the above idea (check 0001 patch). Please have a
> look. I'll come back to the other review comments soon.
>

patch002:

1)
I would like to understand the purpose of 'inactive_count'? Is it only
for users for monitoring purposes? We are not using it anywhere
internally.

I shutdown the instance 5 times and found that 'inactive_count' became
5 for all the slots created on that instance. Is this intentional? I
mean we can not really use them if the instance is down.  I felt it
should increment the inactive_count only if during the span of
instance, they were actually inactive i.e. no streaming or replication
happening through them.


2)
slot.c:
+ case RS_INVAL_XID_AGE:
+ {
+ if (TransactionIdIsNormal(s->data.xmin))
+ {
+                          ..........
+ }
+ if (TransactionIdIsNormal(s->data.catalog_xmin))
+ {
+                          ..........
+ }
+ }

Can we optimize this code? It has duplicate code for processing
s->data.catalog_xmin and s->data.xmin. Can we create a sub-function
for this purpose and call it twice here?

thanks
Shveta