Re: Physical replication slot advance is not persistent

Hi,

On 2020-01-28 17:01:14 +0900, Michael Paquier wrote:
> So attached is an updated patch which addresses the problem just by
> marking a physical slot as dirty if any advancing is done. Some
> documentation is added about the fact that an advance is persistent
> only at the follow-up checkpoint. And the tests are fixed to not use
> a fake LSN but instead advance to the latest LSN position produced.

>
> - /* Update the on disk state when lsn was updated. */
> - if (XLogRecPtrIsInvalid(endlsn))
> - {
> - ReplicationSlotMarkDirty();
> - ReplicationSlotsComputeRequiredXmin(false);
> - ReplicationSlotsComputeRequiredLSN();
> - ReplicationSlotSave();
> - }
> -

I am quite confused by the wholesale removal of these lines. That wasn't
in previous versions of the patch. As far as I can tell not calling
ReplicationSlotsComputeRequiredLSN() for the physical slot leads to the
global minimum LSN never beeing advanced, and thus WAL reserved by the
slot not being removable. Only if there's some independent call to
ReplicationSlotsComputeRequiredLSN() XLogSetReplicationSlotMinimumLSN()
will be called, allowing for slots to advance.

I realize this stuff has been broken since the introduction in
9c7d06d6068 (due to the above being if (XLogRecPtrIsInvalid()) rather
than if (!XLogRecPtrIsInvalid()) , but this seems to make it even worse?

I find it really depressing how much obviously untested stuff gets
added in this area.

Greetings,

Andres Freund