public inbox for [email protected]  

help / color / mirror / Atom feed

From: shveta malik <[email protected]>
To: Bharath Rupireddy <[email protected]>
Cc: Bertrand Drouvot <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
Date: Wed, 27 Mar 2024 09:01:50 +0530
Message-ID: <CAJpy0uBucqQaOSnodBdwCRfDsGTQ9YX9JvGG93igWDCtimp4SA@mail.gmail.com> (raw)
In-Reply-To: <CALj2ACUXS0SfbHzsX8bqo+7CZhocsV52Kiu7OWGb5HVPAmJqnA@mail.gmail.com>
References: <CALj2ACU6tV0bh_MiFBYU7-VK2cj8Gas73htcQjoOv0uvSEG_HA@mail.gmail.com>
	<CAJpy0uA-gJp=aoH+=1jYvU4mZYt4vJCe=cmNn50ZLGFUdjB34g@mail.gmail.com>
	<CAJpy0uAqBi+GbNn2ngJ-A_Z905CD3ss896bqY2ACUjGiF1Gkng@mail.gmail.com>
	<CAJpy0uCLu+mqAwAMum=pXE9YYsy0BE7hOSw_Wno5vjwpFY=63g@mail.gmail.com>
	<CAJpy0uB-yE+Riw7JQ4hW0+igJxvPc+rq+9c7WyTa1Jz7+2gAiA@mail.gmail.com>
	<CALj2ACXRFx9g7A9RFJZF7eBe=zxk7=apMRFuCgJJKYB7O=vgwg@mail.gmail.com>
	<[email protected]>
	<CAA4eK1KrPGwfZV9LYGidjxHeW+rxJ=E2ThjXvwRGLO=iLNuo=Q@mail.gmail.com>
	<[email protected]>
	<CAJpy0uDkTW+t1k3oPkaipFBzZePfFNB5DmiA==pxRGcAdpF=Pg@mail.gmail.com>
	<[email protected]>
	<CAJpy0uB=caX5t6jJe6J+jGUFxnYoKPoAE+8Eaz8t+YXxm-esxw@mail.gmail.com>
	<CALj2ACWLctoiH-pSjWnEpR54q4DED6rw_BRJm5pCx86_Y01MoQ@mail.gmail.com>
	<CALj2ACUXS0SfbHzsX8bqo+7CZhocsV52Kiu7OWGb5HVPAmJqnA@mail.gmail.com>

On Tue, Mar 26, 2024 at 9:59 PM Bharath Rupireddy
<[email protected]> wrote:
>
> On Tue, Mar 26, 2024 at 4:35 PM Bharath Rupireddy
> <[email protected]> wrote:
> >
> > If we just sync inactive_since value for synced slots while in
> > recovery from the primary, so be it. Why do we need to update it to
> > the current time when the slot is being created? We don't expose slot
> > creation time, no? Aren't we fine if we just sync the value from
> > primary and document that fact? After the promotion, we can reset it
> > to the current time so that it gets its own time.
>
> I'm attaching v24 patches. It implements the above idea proposed
> upthread for synced slots. I've now separated
> s/last_inactive_time/inactive_since and synced slots behaviour. Please
> have a look.

Thanks for the patches. Few trivial comments for v24-002:

1)
slot.c:
+ * data from the remote slot. We use InRecovery flag instead of
+ * RecoveryInProgress() as it always returns true even for normal
+ * server startup.

a) Not clear what 'it' refers to. Better to use 'the latter'
b) Is it better to mention the primary here:
 'as the latter always returns true even on the primary server during startup'.


2)
update_local_synced_slot():

- strcmp(remote_slot->plugin, NameStr(slot->data.plugin)) == 0)
+ strcmp(remote_slot->plugin, NameStr(slot->data.plugin)) == 0 &&
+ remote_slot->inactive_since == slot->inactive_since)

When this code was written initially, the intent was to do strcmp at
the end (only if absolutely needed). It will be good if we maintain
the same and add new checks before strcmp.

3)
update_synced_slots_inactive_time():

This assert is removed, is it intentional?
Assert(s->active_pid == 0);


4)
040_standby_failover_slots_sync.pl:

+# Capture the inactive_since of the slot from the standby the logical failover
+# slots are synced/created on the standby.

The comment is unclear, something seems missing.

thanks
Shveta

view thread (46+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
  In-Reply-To: <CAJpy0uBucqQaOSnodBdwCRfDsGTQ9YX9JvGG93igWDCtimp4SA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox