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-02-09T12:56:27Z
Lists: pgsql-hackers 
On Thu, Feb 9, 2023 at 2:08 PM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:
> I think it's still important for lazy vacuum that an iteration over a
> TID store returns TIDs in ascending order, because otherwise a heap
> vacuum does random writes. That being said, we can have
> RT_ITERATE_NEXT() return key-value pairs in an order regardless of how
> the key chunks are stored in a node.

Okay, we can keep that possibility in mind if we need to go there.

> > Note: The regression seems to have started in v17, which is the first
with a full template.

> > 0007 removes some dead code. RT_NODE_INSERT_INNER is only called during
RT_SET_EXTEND, so it can't possibly find an existing key. This kind of
change is much easier with the inner/node cases handled together in a
template, as far as being sure of how those cases are different. I thought
about trying the search in assert builds and verifying it doesn't exist,
but thought yet another #ifdef would be too messy.

It just occurred to me that these facts might be related. v17 was the first
use of the full template, and I decided then I liked one of your earlier
patches where replace_node() calls node_update_inner() better than calling
node_insert_inner() with a NULL parent, which was a bit hard to understand.
That now-dead code was actually used in the latter case for updating the
(original) parent. It's possible that trying to use separate paths
contributed to the regression. I'll try the other way and report back.

> I've attached the patch that adds a simple benchmark test using
> TidStore. With this test, I got similar trends of results to yours
> with gcc, but I've not analyzed them in depth yet.

Thanks for that! I'll take a look.

> BTW it would be better to remove the RT_DEBUG macro from
bench_radix_tree.c.

Absolutely.

--
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