Re: confirmed flush lsn seems to be move backward in certain error cases

On Tue, Feb 20, 2024 at 12:35 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Sat, 17 Feb 2024 at 12:03, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> >
> > @@ -1839,7 +1839,8 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
> >
> > SpinLockAcquire(&MyReplicationSlot->mutex);
> >
> > - MyReplicationSlot->data.confirmed_flush = lsn;
> > + if (lsn > MyReplicationSlot->data.confirmed_flush)
> > + MyReplicationSlot->data.confirmed_flush = lsn;
> >
> > /* if we're past the location required for bumping xmin, do so */
> > if (MyReplicationSlot->candidate_xmin_lsn != InvalidXLogRecPtr &&
> > @@ -1904,7 +1905,8 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
> > else
> > {
> > SpinLockAcquire(&MyReplicationSlot->mutex);
> > - MyReplicationSlot->data.confirmed_flush = lsn;
> > + if (lsn > MyReplicationSlot->data.confirmed_flush)
> > + MyReplicationSlot->data.confirmed_flush = lsn;
> >
> > BTW, from which code path does it update the prior value of
> > confirmed_flush?
>
> The confirmed_flush is getting set in the else condition for this scenario.
>
> If it is through the else check, then can we see if
> > it may change the confirm_flush to the prior position via the first
> > code path? I am asking because in the first code path, we can even
> > flush the re-treated value of confirm_flush LSN.
>
> I was not able to find any scenario to set a prior position with the
> first code path. I tried various scenarios like adding delay in
> walsender, add delay in apply worker, restart the instances and with
> various DML operations. It was always setting it to either to the same
> value as previous or greater value.
>

Fair enough. This means that in the prior versions, it was never
possible to move confirmed_flush LSN in the slot to a backward
position on the disk. So, moving it backward temporarily (in the
memory) shouldn't create any problem. I would prefer your
Assert_confirmed_flush_will_always_not_be_less_than_last_saved_confirmed_flush.patch
to fix this issue.

Thoughts?

--
With Regards,
Amit Kapila.