On Tue, Jul 12, 2022 at 10:28 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Tue, Jul 12, 2022 at 9:48 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Fri, Jul 8, 2022 at 8:20 PM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > On Fri, Jul 8, 2022 at 5:59 PM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > > > > > On Fri, Jul 8, 2022 at 12:46 PM Masahiko Sawada <sawada.m...@gmail.com> > > > > wrote: > > > > > > > > > > On Fri, Jul 8, 2022 at 3:27 PM Amit Kapila <amit.kapil...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > It is using the list "catchange_txns" to form xid array which > > > > shouldn't change for the duration of > > > > ReorderBufferGetCatalogChangesXacts(). Then the caller frees the xid > > > > array after its use. Next time in > > > > ReorderBufferGetCatalogChangesXacts(), the fresh allocation for xid > > > > array happens, so not sure why repalloc would be required? > > > > > > Oops, I mistook catchange_txns for catchange->xcnt. You're right. > > > Starting with the length of catchange_txns should be sufficient. > > > > > > > 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. > > > > I'm doing benchmark tests and will share the results. >
I've done benchmark tests to measure the overhead introduced by doing bsearch() every time when decoding a commit record. I've simulated a very intensified situation where we decode 1M commit records while keeping builder->catchange.xip array but the overhead is negilible: HEAD: 584 ms Patched: 614 ms I've attached the benchmark script I used. With increasing LOG_SNAPSHOT_INTERVAL_MS to 90000, the last decoding by pg_logicla_slot_get_changes() decodes 1M commit records while keeping catalog modifying transactions. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
bench.spec
Description: Binary data
