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

On Fri, Jul 8, 2022 at 6:45 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Jul 7, 2022 at 3:40 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Jul 7, 2022 at 8:21 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached the new version patch that incorporates the comments and
> the optimizations discussed above.
>

Thanks, few minor comments:
1.
In ReorderBufferGetCatalogChangesXacts(), isn't it better to use the
list length of 'catchange_txns' to allocate xids array? If we can do
so, then we will save the need to repalloc as well.

2.
/* ->committed manipulation */
static void SnapBuildPurgeCommittedTxn(SnapBuild *builder);

The above comment also needs to be changed.

3. As SnapBuildPurgeCommittedTxn() removes xacts both from committed
and catchange arrays, the function name no more remains appropriate.
We can either rename to something like SnapBuildPurgeOlderTxn() or
move the catchange logic to a different function and call it from
SnapBuildProcessRunningXacts.

4.
+ if (TransactionIdEquals(builder->catchange.xip[off],
+ builder->xmin) ||
+ NormalTransactionIdFollows(builder->catchange.xip[off],
+ builder->xmin))

Can we use TransactionIdFollowsOrEquals() instead of above?

5. Comment change suggestion:
/*
* Remove knowledge about transactions we treat as committed or
containing catalog
* changes that are smaller than ->xmin. Those won't ever get checked via
- * the ->committed array and ->catchange, respectively. The committed xids will
- * get checked via the clog machinery.
+ * the ->committed or ->catchange array, respectively. The committed xids will
+ * get checked via the clog machinery. We can ideally remove the transaction
+ * from catchange array once it is finished (committed/aborted) but that could
+ * be costly as we need to maintain the xids order in the array.
*/

Apart from the above, I think there are pending comments for the
back-branch patch and some performance testing of this work.

--
With Regards,
Amit Kapila.