On Tue, Jan 9, 2024 at 8:19 PM John Naylor <johncnaylo...@gmail.com> wrote: > > On Tue, Jan 9, 2024 at 9:40 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > In addition, I've made some changes and cleanups: > > These look good to me, although I have not tried dumping a node in a while. > > > 0011 - simplify the radix tree iteration code. I hope it makes the > > code clear and readable. Also I removed RT_UPDATE_ITER_STACK(). > > I'm very pleased with how much simpler it is now! > > > 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. > > Thanks for the pointer. ;-) > > > 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. > > That's been in the back of my mind as well. Maybe the common header > should be the new "base" member? At least, something other than "n".
Agreed. > > > 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. > > That makes sense. Do you want to have a go at it, or shall I? I've done in 0010 patch in v51 patch set. Whereas RT_NODE_4 and RT_NODE_16 structs declaration needs RT_FANOUT_4_HI and RT_FANOUT_16_HI respectively, RT_FANOUT_16_LO and RT_FANOUT_48 need RT_NODE_16 and RT_NODE_48 structs declaration. So fanout declarations are now spread before and after RT_NODE_XXX struct declaration. It's a bit less readable, but I'm not sure of a better way. The previous updates are merged into the main radix tree patch and tidstore patch. Nothing changes in other patches from v50. > > I think after that, the only big cleanup needed is putting things in a > more readable order. I can do that at a later date, and other > opportunities for beautification are pretty minor and localized. Agreed. > > Rationalizing locking is the only thing left that requires a bit of thought. Right, I'll send a reply soon. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
v51-ART.tar.gz
Description: application/gzip