RE: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns

On Mon, Jul 11, 2022 9:54 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached an updated patch, please review it.
>

Thanks for your patch. Here are some comments for the REL14-v1 patch.

1.
+ Size sz = sizeof(TransactionId) * nxacts;;

There is a redundant semicolon at the end.

2.
+ workspace = MemoryContextAlloc(rb->context, rb->n_initial_running_xacts);

Should it be:
+ workspace = MemoryContextAlloc(rb->context, sizeof(TransactionId) * rb->n_initial_running_xacts);

3.
+ /* bound check if there is at least one transaction to be removed */
+ if (NormalTransactionIdPrecedes(rb->initial_running_xacts[0],
+ running->oldestRunningXid))
+ return;
+

Here, I think it should return if rb->initial_running_xacts[0] is older than
oldestRunningXid, right? Should it be changed to:

+ if (!NormalTransactionIdPrecedes(rb->initial_running_xacts[0],
+ running->oldestRunningXid))
+ return;

4.
+ if ((parsed->xinfo & XACT_XINFO_HAS_INVALS) != 0)

Maybe we can change it like the following, to be consistent with other places in
this file. It's also fine if you don't change it.

+ if (parsed->xinfo & XACT_XINFO_HAS_INVALS)

Regards,
Shi yu