MIME-Version: 1.0
References: <ZgQeJDcxB9zYJKIt@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACX1bdaX2gf5ttwk3buiDrgZVV=vM6S1SQKf=Sxhj1HkzQ@mail.gmail.com>
 <CAA4eK1JU14-Fz3Ttx_wy=vzDaeTSv=umt7sC3N+4k2aYcdk4=w@mail.gmail.com>
 <CALj2ACWOz0tEY2vi3OneittGj9+yrQ9A6+6YYb1Cj7L_QrdMVg@mail.gmail.com>
 <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>
 <CAA4eK1+0uiKrLMNT9C47fEW8=M8-BLFygsFRCOWcjEiEbayXeQ@mail.gmail.com>
 <CALj2ACUs4k2CUtCt8NyrMx4kOzEh5gY=m_58Wy=aU2k3Ro-33w@mail.gmail.com>
 <CAA4eK1JC45R0P3SsdyBmGv-H9vXO1NgU_DaCvA8Zv0x+gapdXw@mail.gmail.com> <CALj2ACXZoxXQsV44vE5hLxkhr0-v8KdL=VWq_BqbL+HGRpDogQ@mail.gmail.com>
In-Reply-To: <CALj2ACXZoxXQsV44vE5hLxkhr0-v8KdL=VWq_BqbL+HGRpDogQ@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Fri, 5 Apr 2024 08:39:13 +0530
Message-ID: <CAJpy0uC1sEmUmfrENdBLQWYcSttDTBKkfbKa41obPn1f6YCigA@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>, Masahiko Sawada <sawada.mshk@gmail.com>, 
	Nathan Bossart <nathandbossart@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/CAJpy0uC1sEmUmfrENdBLQWYcSttDTBKkfbKa41obPn1f6YCigA%40mail.gmail.com>
Precedence: bulk

On Thu, Apr 4, 2024 at 5:53=E2=80=AFPM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Apr 4, 2024 at 4:35=E2=80=AFPM Amit Kapila <amit.kapila16@gmail.c=
om> wrote:
> >
> > > Thanks for the changes. v34-0001 LGTM.
> >
> > I was doing a final review before pushing 0001 and found that
> > 'inactive_since' could be set twice during startup after promotion,
> > once while restoring slots and then via ShutDownSlotSync(). The reason
> > is that ShutDownSlotSync() will be invoked in normal startup on
> > primary though it won't do anything apart from setting inactive_since
> > if we have synced slots. I think you need to check 'StandbyMode' in
> > update_synced_slots_inactive_since() and return if the same is not
> > set. We can't use 'InRecovery' flag as that will be set even during
> > crash recovery.
> >
> > Can you please test this once unless you don't agree with the above the=
ory?
>
> Nice catch. I've verified that update_synced_slots_inactive_since is
> called even for normal server startups/crash recovery. I've added a
> check to exit if the StandbyMode isn't set.
>
> Please find the attached v35 patch.

Thanks for the patch. Tested it , works well. Few cosmetic changes needed:

in 040 test file:
1)
# Capture the inactive_since of the slot from the primary. Note that the sl=
ot
# will be inactive since the corresponding subscription is disabled..

2 .. at the end. Replace with one.

2)
# Synced slot on the standby must get its own inactive_since.

. not needed in single line comment (to be consistent with
neighbouring comments)


3)
update_synced_slots_inactive_since():

if (!StandbyMode)
return;

It will be good to add comments here.

thanks
Shveta