Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly

Dear Alexander, Amit

Alexander Korotkov wrote:
> Also, I've changed ReplicationSlotsComputeRequiredLSN() call to
> CheckPointReplicationSlots() to update required LSN after
> SaveSlotToPath() updated last_saved_restart_lsn.  This helps to pass
> checks in 001_stream_rep.pl without additional hacks.

Thank you for the improvement and patch preparation. I confirm the test is
passed without additional hacks now.

I sill do not understand why this solution is favored. It is, in my opinion,
a non backward-compatible solution. In any case, I'm ok to go with this patch.
If needed, I may prepare a backward-compatible solution where
last_saved_restart_lsn values will be in an another place of the shmem, rather
than in ReplicationSlot struct.

I still would like to add my 5 cents to the discussion.

The purpose of the xmin value is to prevent tuples from vacuuming. Slots'
restart_lsn values are used to calculate the oldest lsn to keep WAL segments
from removal in checkpoint. These processes are pretty independent.

The logical slots are advanced in 2 steps. At the first step, the logical
decoding stuff periodically sets consistent candidate values for catalog_xmin and
restart_lsn. At the second step, when LogicalConfirmReceivedLocation is called,
the candidate values are assigned on catalog_xmin and restart_lsn values based
on the confirmed lsn value. The slot is saved with these consistent values.
It is important, that the candidate values are consistent, decoding guarantees
it. In case of a crash, we should guarantee that the loaded from the disk
catalog_xmin and restart_lsn values are consistent and valid for logical slots.
LogicalConfirmReceivedLocation function keeps this consistency by updating them
from consistent candidate values in a single operation.

We have to guarantee that we use saved to disk values to calculate xmin horizon
and slots' oldest lsn. For this purpose, effective_catalog_xmin is used. We
update effective_catalog_xmin in LogicalConfirmReceivedLocation just
after saving slot to disk. Another place where we update effective_catalog_xmin
is when walsender receives hot standby feedback message.

Once, we have two independent processes (vacuuming, checkpoint), we can calculate
xmin horizon and oldest WAL lsn values independently (at different times) from
the saved to disk values. Note, these values are updated in a non atomic way.

The xmin value is set when the node receives hot standby feedback and it is used
to keep tuples from vacuuming as well as catalog_xmin for decoding stuff. Not
sure, xmin is applicable for logical replication.

The confirmed flush lsn is used as a startpoint when a peer node doesn't provide
the start lsn and to check that the start lsn is not older than the latest
confirmed flush lsn. The saving of the slot on disk at each call of
LogicalConfirmReceivedLocation doesn't help to avoid conflicts completely, but
it helps to decrease the probability of conflicts. So, i'm still not sure, we
need to save logical slots on each advance to avoid conflicts, because it
doesn't help in general. The conflicts should be resolved by other means.

Once, we truncate old wal segments in checkpoint only. I believe, it is ok if we
calculate the oldest lsn only at the beginning of the checkpoint, as it was in
the alternative solution. I think, we can update xmin horizon in checkpoint only
but the horizon advancing will be more lazy in this case.

Taking into account these thoughts, I can't see any problems with the alternative
patch where oldest wal lsn is calculated only in checkpoint.

With best regards,
Vitaly