From: | John Naylor <john(dot)naylor(at)enterprisedb(dot)com> |
---|---|
To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
Cc: | Nathan Bossart <nathandbossart(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com>, Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [PoC] Improve dead tuple storage for lazy vacuum |
Date: | 2023-01-09 08:59:04 |
Message-ID: | CAFBsxsFMuyUd48SXy1VMFvpVBHh6B6300_UfTSiPckKJtcwWMA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> [working on templating]
In the end, I decided to base my effort on v8, and not v12 (based on one of
my less-well-thought-out ideas). The latter was a good experiment, but it
did not lead to an increase in readability as I had hoped. The attached v17
is still rough, but it's in good enough shape to evaluate a mostly-complete
templating implementation.
Part of what I didn't like about v8 was distinctions like "node" vs
"nodep", which hinder readability. I've used "allocnode" for some cases
where it makes sense, which is translated to "newnode" for the local
pointer. Some places I just gave up and used "nodep" for parameters like in
v8, just to get it done. We can revisit naming later.
Not done yet:
- get_handle() is not implemented
- rt_attach is defined but unused
- grow_node_kind() was hackishly removed, but could be turned into a macro
(or function that writes to 2 pointers)
- node_update_inner() is back, now that we can share a template with
"search". Seems easier to read, and I suspect this is easier for the
compiler.
- the value type should really be a template macro, but is still hard-coded
to uint64
- I think it's okay if the key is hard coded for PG16: If some use case
needs more than uint64, we could consider "single-value leaves" with varlen
keys as a template option.
- benchmark tests not updated
v13-0007 had some changes to the regression tests, but I haven't included
those. The tests from v13-0003 do pass, both locally and shared. I quickly
hacked together changing shared/local tests by hand (need to recompile),
but it would be good for maintainability if tests could run once each with
local and shmem, but use the same "expected" test output.
Also, I didn't look to see if there were any changes in v14/15 that didn't
have to do with precise memory accounting.
At this point, Masahiko, I'd appreciate your feedback on whether this is an
improvement at all (or at least a good base for improvement), especially
for integrating with the TID store. I think there are some advantages to
the template approach. One possible disadvantage is needing separate
functions for each local and shared memory.
If we go this route, I do think the TID store should invoke the template as
static functions. I'm not quite comfortable with a global function that may
not fit well with future use cases.
One review point I'll mention: Somehow I didn't notice there is no use for
the "chunk" field in the rt_node type -- it's only set to zero and copied
when growing. What is the purpose? Removing it would allow the
smallest node to take up only 32 bytes with a fanout of 3, by eliminating
padding.
Also, v17-0005 has an optimization/simplification for growing into node125
(my version needs an assertion or fallback, but works well now), found by
another reading of Andres' prototype There is a lot of good engineering
there, we should try to preserve it.
--
John Naylor
EDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
v17-0005-Template-out-inner-and-leaf-nodes.patch | text/x-patch | 45.4 KB |
v17-0009-Implement-shared-memory.patch | text/x-patch | 38.6 KB |
v17-0008-Invent-specific-pointer-macros.patch | text/x-patch | 19.3 KB |
v17-0007-Convert-radixtree.h-into-a-template.patch | text/x-patch | 82.4 KB |
v17-0006-Convert-radixtree.c-into-a-header.patch | text/x-patch | 91.9 KB |
v17-0001-introduce-vector8_min-and-vector8_highbit_mask.patch | text/x-patch | 2.6 KB |
v17-0002-Move-some-bitmap-logic-out-of-bitmapset.c.patch | text/x-patch | 6.1 KB |
v17-0004-tool-for-measuring-radix-tree-performance.patch | text/x-patch | 20.0 KB |
v17-0003-Add-radix-implementation.patch | text/x-patch | 89.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | David Rowley | 2023-01-09 09:21:05 | Re: An oversight in ExecInitAgg for grouping sets |
Previous Message | Amit Kapila | 2023-01-09 08:51:03 | Re: Perform streaming logical transactions by background workers and parallel apply |