Date: Wed, 27 Mar 2024 05:48:46 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Nathan Bossart <nathandbossart@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Introduce XID age and inactive timeout based replication slot
 invalidation
Message-ID: <ZgOzPiP7NLgBuGL7@ip-10-97-1-34.eu-west-3.compute.internal>
References: <ZgJ9D0p28ijwsjsl@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA4eK1KrPGwfZV9LYGidjxHeW+rxJ=E2ThjXvwRGLO=iLNuo=Q@mail.gmail.com>
 <ZgKGIDC5lttWTdJH@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAJpy0uDkTW+t1k3oPkaipFBzZePfFNB5DmiA==pxRGcAdpF=Pg@mail.gmail.com>
 <ZgKhgtq+PN0IsURZ@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAJpy0uB=caX5t6jJe6J+jGUFxnYoKPoAE+8Eaz8t+YXxm-esxw@mail.gmail.com>
 <CALj2ACWLctoiH-pSjWnEpR54q4DED6rw_BRJm5pCx86_Y01MoQ@mail.gmail.com>
 <CALj2ACUXS0SfbHzsX8bqo+7CZhocsV52Kiu7OWGb5HVPAmJqnA@mail.gmail.com>
 <ZgMLTBD3xJIP9W93@ip-10-97-1-34.eu-west-3.compute.internal>
 <CALj2ACX-M+Lx6Qv4id7HtqqTKVKA_ti4whK9cgpTPHdHLi943w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CALj2ACX-M+Lx6Qv4id7HtqqTKVKA_ti4whK9cgpTPHdHLi943w@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZgOzPiP7NLgBuGL7%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Wed, Mar 27, 2024 at 10:08:33AM +0530, Bharath Rupireddy wrote:
> On Tue, Mar 26, 2024 at 11:22 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > -               if (!(RecoveryInProgress() && slot->data.synced))
> > +               if (!(InRecovery && slot->data.synced))
> >                         slot->inactive_since = GetCurrentTimestamp();
> >                 else
> >                         slot->inactive_since = 0;
> >
> > Not related to this change but more the way RestoreSlotFromDisk() behaves here:
> >
> > For a sync slot on standby it will be set to zero and then later will be
> > synchronized with the one coming from the primary. I think that's fine to have
> > it to zero for this window of time.
> 
> Right.
> 
> > Now, if the standby is down and one sets sync_replication_slots to off,
> > then inactive_since will be set to zero on the standby at startup and not
> > synchronized (unless one triggers a manual sync). I also think that's fine but
> > it might be worth to document this behavior (that after a standby startup
> > inactive_since is zero until the next sync...).
> 
> Isn't this behaviour applicable for other slot parameters that the
> slot syncs from the remote slot on the primary?

No they are persisted on disk. If not, we'd not know where to resume the decoding
from on the standby in case primary is down and/or sync is off.

> I've added the following note in the comments when we update
> inactive_since in RestoreSlotFromDisk.
> 
>          * Note that for synced slots after the standby starts up (i.e. after
>          * the slots are loaded from the disk), the inactive_since will remain
>          * zero until the next slot sync cycle.
>          */
>         if (!(InRecovery && slot->data.synced))
>             slot->inactive_since = GetCurrentTimestamp();
>         else
>             slot->inactive_since = 0;

I think we should add some words in the doc too and also about what the meaning
of inactive_since on the standby is (as suggested by Shveta in [1]).

[1]: https://www.postgresql.org/message-id/CAJpy0uDkTW%2Bt1k3oPkaipFBzZePfFNB5DmiA%3D%3DpxRGcAdpF%3DPg%40mail.gmail.com

> > 7 ===
> >
> > +# Capture and validate inactive_since of a given slot.
> > +sub capture_and_validate_slot_inactive_since
> > +{
> > +       my ($node, $slot_name, $slot_creation_time) = @_;
> > +       my $name = $node->name;
> >
> > We know have capture_and_validate_slot_inactive_since at 2 places:
> > 040_standby_failover_slots_sync.pl and 019_replslot_limit.pl.
> >
> > Worth to create a sub in Cluster.pm?
> 
> I'd second that thought for now. We might have to debate first if it's
> useful for all the nodes even without replication, and if yes, the
> naming stuff and all that. Historically, we've had such duplicated
> functions until recently, for instance advance_wal and log_contains.
> We
> moved them over to a common perl library Cluster.pm very recently. I'm
> sure we can come back later to move it to Cluster.pm.

I thought that would be the right time not to introduce duplicated code.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com