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

Dear Amit,

> 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.

Sounds reasonable. I introduced new function ReorderBufferGetInvalidations() which
obtains the number of invalidations and its list. ReorderBufferTXN() is not exported
anymore.

> 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]?

You meant that we need to test only the case reported by the Andres, right? New
version did like that. To make the test faster test was migrated to isolation tester
instead of the TAP test.

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

Thanks, I included.

PSA new version for PG 14-master. Special thanks for Hou to minimize the test code.

Best regards,
Hayato Kuroda
FUJITSU LIMITED