MIME-Version: 1.0
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>
 <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>
In-Reply-To: <CAJpy0uB=caX5t6jJe6J+jGUFxnYoKPoAE+8Eaz8t+YXxm-esxw@mail.gmail.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 26 Mar 2024 16:35:16 +0530
Message-ID: <CALj2ACWLctoiH-pSjWnEpR54q4DED6rw_BRJm5pCx86_Y01MoQ@mail.gmail.com>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
To: shveta malik <shveta.malik@gmail.com>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Amit Kapila <amit.kapila16@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/CALj2ACWLctoiH-pSjWnEpR54q4DED6rw_BRJm5pCx86_Y01MoQ%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 26, 2024 at 4:18=E2=80=AFPM shveta malik <shveta.malik@gmail.co=
m> wrote:
>
> > What about another approach?: inactive_since gives data synced from pri=
mary for
> > synced slots and another dedicated field (could be added later...) coul=
d
> > represent what you suggest as the other option.
>
> Yes, okay with me. I think there is some confusion here as well. In my
> second approach above, I have not suggested anything related to
> sync-worker. We can think on that later if we really need another
> field which give us sync time.  In my second approach, I have tried to
> avoid updating inactive_since for synced slots during sync process. We
> update that field during creation of synced slot so that
> inactive_since reflects correct info even for synced slots (rather
> than copying from primary). Please have a look at my patch and let me
> know your thoughts. I am fine with copying it from primary as well and
> documenting this behaviour.

I took a look at your patch.

--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -628,6 +628,7 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid
remote_dbid)
         SpinLockAcquire(&slot->mutex);
         slot->effective_catalog_xmin =3D xmin_horizon;
         slot->data.catalog_xmin =3D xmin_horizon;
+        slot->inactive_since =3D GetCurrentTimestamp();
         SpinLockRelease(&slot->mutex);

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. Do you see any
issues with it?

--=20
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com