Re: Physical replication slot advance is not persistent

At Tue, 24 Dec 2019 20:12:32 +0300, Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru> wrote in
> I dig into the code and it happens because of this if statement:
>
>     /* Update the on disk state when lsn was updated. */
>     if (XLogRecPtrIsInvalid(endlsn))
>     {
>         ReplicationSlotMarkDirty();
>         ReplicationSlotsComputeRequiredXmin(false);
>         ReplicationSlotsComputeRequiredLSN();
>         ReplicationSlotSave();
>     }

Yes, it seems just broken.

> Attached is a small patch, which fixes this bug. I have tried to
> stick to the same logic in this 'if (XLogRecPtrIsInvalid(endlsn))'
> and now pg_logical_replication_slot_advance and
> pg_physical_replication_slot_advance return InvalidXLogRecPtr if
> no-op.
>
> What do you think?

I think we shoudn't change the definition of
pg_*_replication_slot_advance since the result is user-facing.

The functions return a invalid value only when the slot had the
invalid value and failed to move the position. I think that happens
only for uninitalized slots.

Anyway what we should do there is dirtying the slot when the operation
can be assumed to have been succeeded.

As the result I think what is needed there is just checking if the
returned lsn is equal or larger than moveto. Doen't the following
change work?

- if (XLogRecPtrIsInvalid(endlsn))
+ if (moveto <= endlsn)

reagrds.

--
Kyotaro Horiguchi
NTT Open Source Software Center