Horton

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

John Naylor <john.naylor@enterprisedb.com>

View thread View on postgr.es
From: John Naylor <john.naylor@enterprisedb.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>, Andres Freund <andres@anarazel.de>, Matthias van de Meent <boekewurm+postgres@gmail.com>, Yura Sokolov <y.sokolov@postgrespro.ru>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2023-01-17T11:05:59Z
Lists: pgsql-hackers

Attachments

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 radix tree 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 would be 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 CI so elsewhere where bmw_*
symbols resolve just fine on all platforms. I've worked around the error in
v19-0004 by using the general-purpose pg_popcount() function. We only need
to count bits in assert builds, so it doesn't matter a whole lot.

> +        /* 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 is capable of keeping track of that
if necessary. It's also misnamed, since it's concerned with the number of
keys. The vacuum case cares about the number of TIDs, and not number of
(encoded) keys. Even if we ever (say) changed the key to blocknumber and
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 change the name.

> > It sounds like we should at least make the delete functionality
optional. (Side note on optional functions: if an implementation didn'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.

I'll continue working on internals cleanup.

--
John Naylor
EDB: http://www.enterprisedb.com

Commits

  1. radixtree: Fix SIGSEGV at update of embeddable value to non-embeddable.

  2. Get rid of anonymous struct

  3. Teach radix tree to embed values at runtime

  4. Teach TID store to skip bitmap for small numbers of offsets

  5. Use bump context for TID bitmaps stored by vacuum

  6. Fix alignment of stack variable

  7. Use TidStore for dead tuple TIDs storage during lazy vacuum.

  8. Rethink create and attach APIs of shared TidStore.

  9. Fix inconsistent function prototypes with function definitions.

  10. Fix a calculation in TidStoreCreate().

  11. Fix potential integer handling issue in radixtree.h.

  12. Add TIDStore, to store sets of TIDs (ItemPointerData) efficiently.

  13. Fix link error for test_radixtree module on Windows

  14. Blind attempt to fix ODR violations

  15. Fix incorrect format specifier for int64

  16. Fix redefinition of typedefs

  17. Add template for adaptive radix tree

  18. Fix signedness error in 9f225e992 for gcc

  19. Introduce helper SIMD functions for small byte arrays

  20. Optimize vacuuming of relations with no indexes.

  21. Add bound check before bsearch() for performance

  22. Allocate consecutive blocks during parallel seqscans