I tried to address Andres's entirely valid complaints about that comment I added in this patch.
I was going to add a description to restart_lsn in slot.h instead, but it's proving harder to accurately describe restart_lsn than I'd like.
It's a point at which no transactions that commit after confirmed_lsn are yet in progress. Easy enough.
But it's also a point at which visibility information can be reconstructed. It's updated lazily by LogicalIncreaseRestartDecodingForSlot when the client sends confirmation. Looking at SnapBuildProcessRunningXacts that can be the most recent serialized snapshot from the snapshot builder (if there's no xact running at confirmed_lsn) or the restart_decoding_lsn of the oldest xact in progress at the confirmed_lsn. If I've read it correctly the xact's restart_decoding_lsn is the ReorderBuffer's restart_decoding_lsn at the time the xact started, which is in turn the last serialized snapshot at that time.
So I'd describe restart_lsn as something like:
"
The oldest LSN that might be required by this replication slot.
Points to the LSN of the most recent xl_running_xacts record where no transaction that commits after confirmed_flush is already in progress. At this point all xacts committing after confirmed_flush can be read entirely into reorder buffers and all visibility information can be reconstructed.
"
Is that accurate?
I want to get rid of the incorrect comment and fix this up.