On Tue, Jul 12, 2022 8:49 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > I've attached an updated patch. > > While trying this idea, I noticed there is no API to get the length of > dlist, as we discussed offlist. Alternative idea was to use List > (T_XidList) but I'm not sure it's a great idea since deleting an xid > from the list is O(N), we need to implement list_delete_xid, and we > need to make sure allocating list node in the reorder buffer context. > So in the patch, I added a variable, catchange_ntxns, to keep track of > the length of the dlist. Please review it. >
Thanks for your patch. Here are some comments on the master patch. 1. In catalog_change_snapshot.spec, should we use "RUNNING_XACTS record" instead of "RUNNING_XACT record" / "XACT_RUNNING record" in the comment? 2. + * Since catchange.xip is sorted, we find the lower bound of + * xids that sill are interesting. Typo? "sill" -> "still" 3. + * This array is set once when restoring the snapshot, xids are removed + * from the array when decoding xl_running_xacts record, and then eventually + * becomes an empty. + /* catchange list becomes an empty */ + pfree(builder->catchange.xip); + builder->catchange.xip = NULL; Should "becomes an empty" be modified to "becomes empty"? 4. + * changes that are smaller than ->xmin. Those won't ever get checked via + * the ->committed array and ->catchange, respectively. The committed xids will Should we change "the ->committed array and ->catchange" to "the ->committed or ->catchange array" ? Regards, Shi yu
