On Fri, Jul 8, 2022 at 3:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 8, 2022 at 6:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Jul 7, 2022 at 3:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Jul 7, 2022 at 8:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached the new version patch that incorporates the comments and
> > the optimizations discussed above.
> >
>
> Thanks, few minor comments:
Thank you for the 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.
Since ReorderBufferGetcatalogChangesXacts() collects all ongoing
catalog modifying transactions, the length of the array could be
bigger than the one taken last time. We can start with the previous
length but I think we cannot remove the need for repalloc.
> 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.
> */
>
Agreed with the above comments.
> Apart from the above, I think there are pending comments for the
> back-branch patch and some performance testing of this work.
Right. I'll incorporate all comments I got so far into these patches
and submit them. Also, will do some benchmark tests.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/