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.
