Re: logical replication: restart_lsn can go backwards (and more), seems broken since 9.4

On 11/13/24 05:38, Ashutosh Bapat wrote:
> ...
> 
> Here's way we can fix SnapBuildProcessRunningXacts() similar to
> DecodeCommit(). DecodeCommit() uses SnapBuildXactNeedsSkip() to decide
> whether a given transaction should be decoded or not.
> /*
>  * Should the contents of transaction ending at 'ptr' be decoded?
>  */
> bool
> SnapBuildXactNeedsSkip(SnapBuild *builder, XLogRecPtr ptr)
> {
> return ptr < builder->start_decoding_at;
> }
> 
> Similar to SnapBuild::start_decoding_at we could maintain a field
> SnapBuild::start_reading_at to the LSN from which the WAL sender would
> start reading WAL. If candidate_restart_lsn produced by a running
> transactions WAL record is less than SnapBuild::start_reading_at,
> SnapBuildProcessRunningXacts() won't call
> LogicalIncreaseRestartDecodingForSlot() with that candiate LSN. We
> won't access the slot here and the solution will be inline with
> DecodeCommit() which skips the transactions.
> 

Could you maybe write a patch doing this? That would allow proper
testing etc.


regards

-- 
Tomas Vondra