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
