On Tue, Jan 9, 2024 at 8:19 PM John Naylor <johncnaylorls@gmail.com> wrote:
>
> On Tue, Jan 9, 2024 at 9:40 AM Masahiko Sawada <sawada.mshk@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
