Re: long-standing data loss bug in initial sync of logical replication

On Mon, Mar 17, 2025 at 6:53 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> OK, let me share patched for back branches. Mostly the same fix patched as master
> can be used for PG14-PG17, like attached.
>

A few comments:
==============
1.
+SnapBuildDistributeSnapshotAndInval(SnapBuild *builder, XLogRecPtr
lsn, TransactionId xid)
{
dlist_iter txn_i;
ReorderBufferTXN *txn;
+ ReorderBufferTXN *curr_txn;
+
+ curr_txn = ReorderBufferTXNByXid(builder->reorder, xid, false, NULL,
InvalidXLogRecPtr, false);

The above is used to access invalidations from curr_txn. I am thinking
about whether it would be better to expose a new function to get
invalidations for a txn based on xid instead of getting
ReorderBufferTXN. It would avoid any layering violation and misuse of
ReorderBufferTXN by other modules.

2. The patch has a lot of tests to verify the same points. Can't we
have one simple test using SQL API based on what Andres presented in
an email [1]?

3. I have made minor changes in the comments in the attached.

[1] - https://www.postgresql.org/message-id/20231119021830.d6t6aaxtrkpn743y%40awork3.anarazel.de

--
With Regards,
Amit Kapila.