Re: persist logical slots to disk during shutdown checkpoint

On Mon, Sep 11, 2023 at 02:49:49PM +0530, Amit Kapila wrote:
> I don't know if the difference is worth inventing a new BACKEND_TYPE_*
> but if you think so then we can probably discuss this in a new thread.
> I think we may want to improve some comments as a separate patch to
> make this evident.

The comments in postmaster.c could be improved, at least.  There is no
need to discuss that here.

> This point is not very clear to me. Can you please quote the exact
> comment if you think something needs to be changed?

Hmm.  Don't think that's it yet..

Please see the v11 attached, that rewords all the places of the patch
that need clarifications IMO.  I've found that the comment additions
in CheckPointReplicationSlots() to be overcomplicated:
- The key point to force a flush of a slot if its confirmed_lsn has
moved ahead of the last LSN where it was saved is to make the follow
up restart more responsive.
- Not sure that there is any point to mention the other code paths in
the tree where ReplicationSlotSave() can be called, and a slot can be
saved in other processes than just WAL senders (like slot
manipulations in normal backends, for one).  This was the last
sentence in v10.
- Persist is incorrect in this context in the tests, slot.c and
slot.h, as it should refer to the slot's data being flushed, saved or
just "made durable" because this is what the new last saved LSN is
here for.  Persistence is a slot property, and does not refer to the
fact of flushing the data IMO.

+           if (s->data.invalidated == RS_INVAL_NONE &&
+               s->data.confirmed_flush != s->last_saved_confirmed_flush)

Actually this is incorrect, no?  Shouldn't we make sure that the
confirmed_flush is strictly higher than the last saved LSN?
--
Michael