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> <CALj2ACWLctoiH-pSjWnEpR54q4DED6rw_BRJm5pCx86_Y01MoQ@mail.gmail.com>
In-Reply-To: <CALj2ACWLctoiH-pSjWnEpR54q4DED6rw_BRJm5pCx86_Y01MoQ@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Tue, 26 Mar 2024 16:49:18 +0530
Message-ID: <CAJpy0uCy37xeb2L0T+PRcazqdanzx3Ezb9a8yht-acjoDmrC4w@mail.gmail.com>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
To: Bharath Rupireddy <bharath.rupireddyforpostgres@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>, 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/CAJpy0uCy37xeb2L0T%2BPRcazqdanzx3Ezb9a8yht-acjoDmrC4w%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 26, 2024 at 4:35=E2=80=AFPM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Mar 26, 2024 at 4:18=E2=80=AFPM shveta malik <shveta.malik@gmail.=
com> wrote:
> >
> > > What about another approach?: inactive_since gives data synced from p=
rimary for
> > > synced slots and another dedicated field (could be added later...) co=
uld
> > > 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?

If we update inactive_since  at synced slot's creation or during
restart (skipping setting it during sync), then this time reflects
actual 'inactive_since' for that particular synced slot.  Isn't that a
clear info for the user and in alignment of what the name
'inactive_since' actually suggests?

> We don't expose slot
> creation time, no?

No, we don't. But for synced slot, that is the time since that slot is
inactive  (unless promoted), so we are exposing inactive_since and not
creation time.

>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?

Yes, we can do that. But curious to know, do we see any additional
benefit of reflecting primary's inactive_since at standby which I
might be missing?

thanks
Shveta