On Mon, Jan 16, 2023 at 3:18 PM Masahiko Sawada <
sawada.mshk@gmail.com> wrote:
>
> On Mon, Jan 16, 2023 at 2:02 PM John Naylor
> <
john.naylor@enterprisedb.com> wrote:
> >
> Thanks! cfbot complaints about some warnings but these are expected
> (due to unused delete routines etc). But one reported error[1] might
> be relevant with 0002 patch?
> [05:44:11.819] test_radixtree.c.obj : error LNK2001: unresolved
> external symbol pg_popcount64
> [05:44:11.819] src\test\modules\test_radixtree\test_radixtree.dll :
> fatal error LNK1120: 1 unresolved externals
Yeah, I'm not sure what's causing that. Since that comes from a debugging function, we could work around it, but it would be nice to understand why, so I'll probably have to experiment on my CI repo.
> ---
> +#ifndef RT_COMMON
> +#define RT_COMMON
>
> What are we using this macro RT_COMMON for?
It was a quick way to define some things only once, so they probably all showed up in the list of things you found not undefined. It's different from the style of simplehash.h, which is to have a local name and #undef for every single thing. simplehash.h is a precedent, so I'll change it to match. I'll take a look at your list, too.
> > + * Add Tids on a block to TidStore. The caller must ensure the offset numbers
> > + * in 'offsets' are ordered in ascending order.
> >
> > Must? What happens otherwise?
>
> It ends up missing TIDs by overwriting the same key with different
> values. Is it better to have a bool argument, say need_sort, to sort
> the given array if the caller wants?
> Since it seems you're working on another cleanup, I can address the
> above comments after your work is completed. But I'm also fine with
> including them into your cleanup work.
I think we can work mostly simultaneously, if you work on tid store and vacuum, and I work on the template. We can always submit a full patchset including each other's latest work. That will catch rebase issues sooner.
--
John Naylor
EDB:
http://www.enterprisedb.com