On Wed, Jan 3, 2024 at 11:10 PM John Naylor <johncnaylo...@gmail.com> wrote: > > On Tue, Jan 2, 2024 at 8:01 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > I agree that we expose RT_LOCK_* functions and have tidstore use them, > > but am not sure the if (TidStoreIsShared(ts) LWLockAcquire(..., ...)" > > calls part. I think that even if we expose them, we will still need to > > do something like "if (TidStoreIsShared(ts)) > > shared_rt_lock_share(ts->tree.shared)", no? > > I'll come back to this topic separately. > > > I've attached a new patch set. From v47 patch, I've merged your > > changes for radix tree, and split the vacuum integration patch into 3 > > patches: simply replaces VacDeadItems with TidsTore (0007 patch), and > > use a simple TID array for one-pass strategy (0008 patch), and replace > > has_lpdead_items with "num_offsets > 0" (0009 patch), while > > incorporating your review comments on the vacuum integration patch > > Nice! > > > (sorry for making it difficult to see the changes from v47 patch). > > It's actually pretty clear. I just have a couple comments before > sharing my latest cleanups: > > (diff'ing between v47 and v48): > > -- /* > - * In the shared case, TidStoreControl and radix_tree are backed by > the > - * same DSA area and rt_memory_usage() returns the value including > both. > - * So we don't need to add the size of TidStoreControl separately. > - */ > if (TidStoreIsShared(ts)) > - return sizeof(TidStore) + > shared_rt_memory_usage(ts->tree.shared); > + rt_mem = shared_rt_memory_usage(ts->tree.shared); > + else > + rt_mem = local_rt_memory_usage(ts->tree.local); > > - return sizeof(TidStore) + sizeof(TidStore) + > local_rt_memory_usage(ts->tree.local); > + return sizeof(TidStore) + sizeof(TidStoreControl) + rt_mem; > > Upthread, I meant that I don't see the need to include the size of > these structs *at all*. They're tiny, and the blocks/segments will > almost certainly have some empty space counted in the total anyway. > The returned size is already overestimated, so this extra code is just > a distraction.
Agreed. > > - if (result->num_offsets + bmw_popcount(w) > result->max_offset) > + if (result->num_offsets + (sizeof(bitmapword) * BITS_PER_BITMAPWORD) > >= result->max_offset) > > I believe this math is wrong. We care about "result->num_offsets + > BITS_PER_BITMAPWORD", right? > Also, it seems if the condition evaluates to equal, we still have > enough space, in which case ">" the max is the right condition. Oops, you're right. Fixed. > > - if (off < 1 || off > MAX_TUPLES_PER_PAGE) > + if (off < 1 || off > MaxOffsetNumber) > > This can now use OffsetNumberIsValid(). Fixed. > > > 0013 to 0015 patches are also updates from v47 patch. > > > I'm thinking that we should change the order of the patches so that > > tidstore patch requires the patch for changing DSA segment sizes. That > > way, we can remove the complex max memory calculation part that we no > > longer use from the tidstore patch. > > I don't think there is any reason to have those calculations at all at > this point. Every patch in every version should at least *work > correctly*, without kludging m_w_m and without constraining max > segment size. I'm fine with the latter remaining in its own thread, > and I hope we can consider it an enhancement that respects the admin's > configured limits more effectively, and not a pre-requisite for not > breaking. I *think* we're there now, but it's hard to tell since 0015 > was at the very end. As I said recently, if something still fails, I'd > like to know why. So for v49, I took the liberty of removing the DSA > max segment patches for now, and squashing v48-0015. Fair enough. > > In addition for v49, I have quite a few cleanups: > > 0001 - This hasn't been touched in a very long time, but I ran > pgindent and clarified a comment > 0002 - We no longer need to isolate the rightmost bit anywhere, so > removed that part and revised the commit message accordingly. Thanks. > > radix tree: > 0003 - v48 plus squashed v48-0013 > 0004 - Removed or adjusted WIP, FIXME, TODO items. Some were outdated, > and I fixed most of the rest. > 0005 - Remove the RT_PTR_LOCAL macro, since it's not really useful anymore. > 0006 - RT_FREE_LEAF only needs the allocated pointer, so pass that. A > bit simpler. > 0007 - Uses the same idea from a previous cleanup of RT_SET, for RT_DELETE. > 0008 - Removes a holdover from the multi-value leaves era. > 0009 - It occurred to me that we need to have unique names for memory > contexts for different instantiations of the template. This is one way > to do it, by using the configured RT_PREFIX in the context name. I > also took an extra step to make the size class fanout show up > correctly on different platforms, but that's probably overkill and > undesirable, and I'll probably use only the class name next time. > 0010/11 - Make the array functions less surprising and with more > informative names. > 0012 - Restore a useful technique from Andres's prototype. This part > has been slow for a long time, so much that it showed up in a profile > where this path wasn't even taken much. These changes look good to me. I've squashed them. In addition, I've made some changes and cleanups: 0010 - address the above review comments. 0011 - simplify the radix tree iteration code. I hope it makes the code clear and readable. Also I removed RT_UPDATE_ITER_STACK(). 0012 - fix a typo 0013 - In RT_SHMEM case, we use SIZEOF_VOID_P for RT_VALUE_IS_EMBEDDABLE check, but I think it's not correct. Because DSA has its own pointer size, SIZEOF_DSA_POINTER, it could be 4 bytes even if SIZEOF_VOID_P is 8 bytes, for example in a case where !defined(PG_HAVE_ATOMIC_U64_SUPPORT). Please refer to dsa.h for details. 0014 - cleanup RT_VERIFY code. 0015 - change and cleanup RT_DUMP_NODE(). Now it dumps only one node and no longer supports dumping nodes recursively. 0016 - remove RT_DUMP_SEARCH() and RT_DUMP(). These seem no longer necessary. 0017 - MOve RT_DUMP_NODE to the debug function section, close to RT_STATS. 0018 - Fix a printf format in RT_STATS(). BTW, now that the inner and leaf nodes use the same structure, do we still need RT_NODE_BASE_XXX types? Most places where we use RT_NODE_BASE_XXX types can be replaced with RT_NODE_XXX types. Exceptions are RT_FANOUT_XX calculations: #if SIZEOF_VOID_P < 8 #define RT_FANOUT_16_LO ((96 - sizeof(RT_NODE_BASE_16)) / sizeof(RT_PTR_ALLOC)) #define RT_FANOUT_48 ((512 - sizeof(RT_NODE_BASE_48)) / sizeof(RT_PTR_ALLOC)) #else #define RT_FANOUT_16_LO ((160 - sizeof(RT_NODE_BASE_16)) / sizeof(RT_PTR_ALLOC)) #define RT_FANOUT_48 ((768 - sizeof(RT_NODE_BASE_48)) / sizeof(RT_PTR_ALLOC)) #endif /* SIZEOF_VOID_P < 8 */ But I think we can replace them with offsetof(RT_NODE_16, children) etc. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
v50-ART.tar.gz
Description: GNU Zip compressed data
