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

Hello!

Hayato, Zsolt, thanks for the review!

> Can you put a comment why we need to get the GetLatestSnapshot()? Per my
> understanding it's for observing tuples which are updated by concurrent transactions.

Done. BTW, it is also GetLatestSnapshot in the current upstream.

> You told that this patch does not cause the performance isuse because
> GetLatestSnapshot() had already been eventually called, but what if the
> update_missing happens? Can the performance decrease in the extreme case which
> the lots of XXX_missing happens?

With the current snapshot-caching machinery, I think the impact is not
noticeable when compared to table fetches, logging, etc., done in that
path.
The whole scenario is non-normal also.

> I initially wondered it may break the transaction isolation level REPEATABLE READ
> and the function should return false: but this can lose changes from the publisher.
> See the attached claude-generated test case.

Nice catch, thanks.

I think we can fix it in a way similar to
d774576f6f05f65e3a944eb509ef0620ea6b107e: just force READ COMMITTED
for the worker.
I don't think any other value makes sense per se.

Zsolt, I have added your test into the patch itself (a bit reworked).

Mikhail.