On Fri, Jul 8, 2022 at 6:45 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Thu, Jul 7, 2022 at 3:40 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Thu, Jul 7, 2022 at 8:21 AM Masahiko Sawada <sawada.m...@gmail.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.