MIME-Version: 1.0
References: <CALj2ACXEwHxzuP9imWoJKc24HcSB1izPaot7U9XGzX0qGWq3DA@mail.gmail.com>
 <CAJpy0uAyG44xp4pzGd7X+xrJMuPfD0dYC0tA9Gep9zMuALzFGg@mail.gmail.com>
 <CALj2ACXh4ur3gk1O-yT+R4sOJtKP6KSYjMYm6d4R-AtVTMhikw@mail.gmail.com>
 <CAJpy0uDgKwk=hOE-Q5z4nA5kovHKhuspjXn2MhSmApbQW2ck9Q@mail.gmail.com>
 <CALj2ACVPaiTHokRzV135UycNozKsLwa-jvcjX7G7rFbHYJLjaA@mail.gmail.com>
 <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> <CAJpy0uBoK=d6Eoz03mrDK1+BK-mQmGre+Pn2GQcs3RFBTghk2g@mail.gmail.com>
In-Reply-To: <CAJpy0uBoK=d6Eoz03mrDK1+BK-mQmGre+Pn2GQcs3RFBTghk2g@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Wed, 3 Apr 2024 16:19:46 +0530
Message-ID: <CAA4eK1JuOwQh_FBdpAXUBgULTC2pKZy7PLG-WDaVQ5OSdozPUw@mail.gmail.com>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
To: shveta malik <shveta.malik@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1JuOwQh_FBdpAXUBgULTC2pKZy7PLG-WDaVQ5OSdozPUw%40mail.gmail.com>
Precedence: bulk

On Wed, Apr 3, 2024 at 2:58=E2=80=AFPM shveta malik <shveta.malik@gmail.com=
> wrote:
>
> On Wed, Apr 3, 2024 at 11:17=E2=80=AFAM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, Apr 3, 2024 at 8:38=E2=80=AFAM shveta malik <shveta.malik@gmail=
.com> wrote:
> > >
> > > > > Or a simple solution is that the slotsync worker updates
> > > > > inactive_since as it does for non-synced slots, and disables
> > > > > timeout-based slot invalidation for synced slots.
> > >
> > > I like this idea better, it takes care of such a case too when the
> > > user is relying on sync-function rather than worker and does not want
> > > to get the slots invalidated in between 2 sync function calls.
> >
> > Please find the attached v31 patches implementing the above idea:
> >
>
> Thanks for the patches, please find few comments:
>
> v31-001:
>
> 1)
> system-views.sgml:
> value will get updated  after every synchronization from the
> corresponding remote slot on the primary.
>
> --This is confusing. It will be good to rephrase it.
>
> 2)
> update_synced_slots_inactive_since()
>
> --May be, we should mention in the header that this function is called
> only during promotion.
>
> 3) 040_standby_failover_slots_sync.pl:
> We capture inactive_since_on_primary when we do this for the first time a=
t #175
> ALTER SUBSCRIPTION regress_mysub1 DISABLE"
>
> But we again recreate the sub and disable it at line #280.
> Do you think we shall get inactive_since_on_primary again here, to be
> compared with inactive_since_on_new_primary later?
>

I think so.

Few additional comments on tests:
1.
+is( $standby1->safe_psql(
+ 'postgres',
+ "SELECT '$inactive_since_on_primary'::timestamptz <
'$inactive_since_on_standby'::timestamptz AND
+ '$inactive_since_on_standby'::timestamptz < '$slot_sync_time'::timestampt=
z;"

Shall we do <=3D check as we are doing in the main function
get_slot_inactive_since_value as the time duration is less so it can
be the same as well? Similarly, please check other tests.

2.
+=3Ditem $node->get_slot_inactive_since_value(self, slot_name, reference_ti=
me)
+
+Get inactive_since column value for a given replication slot validating it
+against optional reference time.
+
+=3Dcut
+
+sub get_slot_inactive_since_value

I see that all callers validate against reference time. It is better
to name it validate_slot_inactive_since rather than using get_* as the
main purpose is to validate the passed value.

--=20
With Regards,
Amit Kapila.