Re: Synchronizing slots from primary to standby

On Thu, Jan 11, 2024 at 3:42 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Jan 10, 2024 at 5:53 PM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
>
> > > IIUC on the standby we just want to overwrite what we get from primary no? If
> > > so why we are using those APIs that are meant for the actual decoding slots
> > > where it needs to take certain logical decisions instead of mere overwriting?
> >
> > I think we don't have a strong reason to use these APIs, but it was convenient to
> > use these APIs as they can take care of updating the slots info and will call
> > functions like, ReplicationSlotsComputeRequiredXmin,
> > ReplicationSlotsComputeRequiredLSN internally. Or do you prefer directly overwriting
> > the fields and call these manually ?
>
> I might be missing something but do you want to call
> ReplicationSlotsComputeRequiredXmin() kind of functions in standby? I
> mean those will ultimately update the catalog xmin and replication
> xmin in Procarray and that prevents Vacuum from cleaning up some of
> the required xids. But on standby, those shared memory parameters are
> not used IIUC.
>

These xmins are required for logical slots as we allow logical
decoding on standby (see GetOldestSafeDecodingTransactionId()). We
also invalidate such slots if the required rows are removed on
standby. Similarly, we need ReplicationSlotsComputeRequiredLSN() to
avoid getting the required WAL removed.

> In my opinion on standby, we just need to update the values in the
> local slots and whatever we get from remote slots without taking all
> the logical decisions in the hope that they will all fall into a
> particular path, for example, if you see LogicalIncreaseXminForSlot(),
> it is doing following steps of operations as shown below[1]. These
> all make sense when you are doing candidate-based updation where we
> first mark the candidates and then update the candidate to real value
> once you get the confirmation for the LSN. Now following all this
> logic looks completely weird unless this can fall in a different path
> I feel it will do some duplicate steps as well. For example in
> local_slot_update(), first you call LogicalConfirmReceivedLocation()
> which will set the 'data.confirmed_flush' and then you will call
> LogicalIncreaseXminForSlot() which will set the 'updated_xmin = true;'
> and will again call LogicalConfirmReceivedLocation().
>

In case (else if (slot->candidate_xmin_lsn == InvalidXLogRecPtr)),
even the updated_xmin is not getting set to true which means there is
a chance that we will never update the required xmin values.

I don't think
> this is the correct way of reusing the function unless you need to go
> through those paths and I am missing something.
>

I agree with this conclusion and also think that we should directly
update the required fields and call functions like
ReplicationSlotsComputeRequiredLSN() wherever required.

--
With Regards,
Amit Kapila.