Re: Logical replication: lost updates/deletes and invalid log messages caused by SnapshotDirty + concurrent updates

Hi Mikhail,

> The patch [...] rewrites RelationFindReplTupleByIndex/
> RelationFindReplTupleSeq to use GetLatestSnapshot for each attempt to
> find the target row. Since it calls GetLatestSnapshot before
> table_tuple_lock anyway, no performance regression is expected.

I reviewed v20 and reproduced the bug end to end on current master
(85adc584e2b), built with --enable-injection-points.

With v20 applied, 039_apply_search_concurrent_update passes. To confirm
the fix is what makes it pass, I reverted only the execReplication.c hunks
(back to the dirty snapshot), keeping the injection points and the test.
039 then fails exactly as the bug predicts (6 of 7):

- the DELETE conflict is logged as 'delete_missing' instead of
'delete_origin_differs', and the row is not deleted on the subscriber:
the publisher's DELETE is lost.
- the UPDATE conflict is logged as 'update_missing' instead of
'update_origin_differs', and the row keeps 'frompub' instead of
'frompubnew': the publisher's UPDATE is lost.
- the same happens under retain_dead_tuples.

So the subscriber's dirty-snapshot index scan misses the concurrently
updated row, the publisher's DELETE/UPDATE is dropped, and the conflict is
logged as *_missing instead of *_origin_differs. Reverting just the
snapshot change brings the bug back, which isolates the snapshot as the
root cause and v20 as the minimal fix.

No regression: the full subscription suite passes with v20, 41 files and
621 tests.

The fix itself reads well. GetLatestSnapshot() was already taken before
table_tuple_lock() on the found path, so moving it to the start of each
attempt is not an extra snapshot, and a fresh latest snapshot per retry is
the right thing. Leaving check_exclusion_or_unique_constraint() on the
dirty snapshot (with the new comment) also seems right, since the ON
CONFLICT retry covers that case and there is no data loss there.

Looks good to me. Happy to keep an eye on the next rev.

Regards,
Manu