On Tue, Jan 17, 2023 at 8:06 PM John Naylor
<john.naylor@enterprisedb.com> wrote:
>
> 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:
>
> Attached is an update that mostly has the modest goal of getting CI green again. v19-0003 has squashed the entire
radixtree template from previously. I've kept out the perf test module for now -- still needs updating.
>
> > > [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
wouldbe nice to understand why, so I'll probably have to experiment on my CI repo.
>
> I'm still confused by this error, because it only occurs in the test module. I successfully built with just 0002 in
CIso elsewhere where bmw_* symbols resolve just fine on all platforms. I've worked around the error in v19-0004 by
usingthe general-purpose pg_popcount() function. We only need to count bits in assert builds, so it doesn't matter a
wholelot.
>
> > + /* XXX: do we need to set a callback on exit to detach dsa? */
> >
> > In the current shared radix tree design, it's a caller responsible
> > that they create (or attach to) a DSA area and pass it to RT_CREATE()
> > or RT_ATTACH(). It enables us to use one DSA not only for the radix
> > tree but also other data. Which is more flexible. So the caller needs
> > to detach from the DSA somehow, so I think we don't need to set a
> > callback here for that.
> >
> > ---
> > + dsa_free(tree->dsa, tree->ctl->handle); // XXX
> > + //dsa_detach(tree->dsa);
> >
> > Similar to above, I think we should not detach from the DSA area here.
> >
> > Given that the DSA area used by the radix tree could be used also by
> > other data, I think that in RT_FREE() we need to free each radix tree
> > node allocated in DSA. In lazy vacuum, we check the memory usage
> > instead of the number of TIDs and need to reset the TidScan after an
> > index scan. So it does RT_FREE() and dsa_trim() to return DSM segments
> > to the OS. I've implemented rt_free_recurse() for this purpose in the
> > v15 version patch.
> >
> > --
> > - Assert(tree->root);
> > + //Assert(tree->ctl->root);
> >
> > I think we don't need this assertion in the first place. We check it
> > at the beginning of the function.
>
> I've removed these in v19-0006.
>
> > > That sounds like a good idea. It's also worth wondering if we even need RT_NUM_ENTRIES at all, since the caller
iscapable of keeping track of that if necessary. It's also misnamed, since it's concerned with the number of keys. The
vacuumcase cares about the number of TIDs, and not number of (encoded) keys. Even if we ever (say) changed the key to
blocknumberand value to Bitmapset, the number of keys might not be interesting.
> >
> > Right. In fact, TIdStore doesn't use RT_NUM_ENTRIES.
>
> I've moved it to the test module, which uses it extensively. There, the name is more clear what it's for, so I didn't
changethe name.
>
> > > It sounds like we should at least make the delete functionality optional. (Side note on optional functions: if an
implementationdidn't care about iteration or its order, we could optimize insertion into linear nodes)
> >
> > Agreed.
>
> Done in v19-0007.
>
> v19-0009 is just a rebase over some more vacuum cleanups.
Thank you for updating the patches!
I've attached new version patches. There is no change from v19 patch
for 0001 through 0006. And 0004, 0005 and 0006 patches look good to
me. We can merge them into 0003 patch.
0007 patch fixes functions that are defined when RT_DEBUG. These
functions might be removed before the commit but this is useful at
least under development. 0008 patch fixes a bug in
RT_CHUNK_VALUES_ARRAY_SHIFT() and adds tests for that. 0009 patch
fixes the cfbot issue by linking pgport_srv. 0010 patch adds
RT_FREE_RECURSE() to free all radix tree nodes allocated in DSA. 0011
patch updates copyright etc. 0012 and 0013 patches are updated patches
that incorporate all comments I got so far.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com