Re: [PoC] Improve dead tuple storage for lazy vacuum

On Mon, Jan 16, 2023 at 3:18 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Mon, Jan 16, 2023 at 2:02 PM John Naylor
> <john(dot)naylor(at)enterprisedb(dot)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