On Thu, Apr 25, 2024 at 8:36 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Mon, Apr 15, 2024 at 6:12 PM John Naylor <johncnaylo...@gmail.com> wrote:
> > - RT_KEY_GET_SHIFT is not covered for key=0: > > > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L803 > > > > That should be fairly simple to add to the tests. > > There are two paths to call RT_KEY_GET_SHIFT(): > > 1. RT_SET() -> RT_KEY_GET_SHIFT() > 2. RT_SET() -> RT_EXTEND_UP() -> RT_KEY_GET_SHIFT() > > In both cases, it's called when key > tree->ctl->max_val. Since the > minimum value of max_val is 255, RT_KEY_GET_SHIFT() is never called > when key=0. Ah, right, so it is dead code. Nothing to worry about, but it does point the way to some simplifications, which I've put together in the attached. > > - RT_DELETE: "if (key > tree->ctl->max_val)" is not covered: > > > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L2644 > > > > That should be easy to add. > > Agreed. The patch is attached. LGTM > > - TidStoreCreate* has some memory clamps that are not covered: > > > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/access/common/tidstore.c.gcov.html#L179 > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/access/common/tidstore.c.gcov.html#L234 > > > > Maybe we could experiment with using 1MB for shared, and something > > smaller for local. > > I've confirmed that the local and shared tidstore with small max sizes > such as 4kB and 1MB worked. Currently the max size is hard-coded in > test_tidstore.c but if we use work_mem as the max size, we can pass > different max sizes for local and shared in the test script. Seems okay, do you want to try that and see how it looks?
diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h index 2896a6efc5..fdac103763 100644 --- a/src/include/lib/radixtree.h +++ b/src/include/lib/radixtree.h @@ -217,7 +217,6 @@ #define RT_NODE_48_GET_CHILD RT_MAKE_NAME(node_48_get_child) #define RT_NODE_256_IS_CHUNK_USED RT_MAKE_NAME(node_256_is_chunk_used) #define RT_NODE_256_GET_CHILD RT_MAKE_NAME(node_256_get_child) -#define RT_KEY_GET_SHIFT RT_MAKE_NAME(key_get_shift) #define RT_SHIFT_GET_MAX_VAL RT_MAKE_NAME(shift_get_max_val) #define RT_NODE_SEARCH RT_MAKE_NAME(node_search) #define RT_NODE_DELETE RT_MAKE_NAME(node_delete) @@ -320,9 +319,6 @@ RT_SCOPE void RT_STATS(RT_RADIX_TREE * tree); /* Mask for extracting a chunk from a key */ #define RT_CHUNK_MASK ((1 << RT_SPAN) - 1) -/* Maximum shift needed to extract a chunk from a key */ -#define RT_MAX_SHIFT RT_KEY_GET_SHIFT(UINT64_MAX) - /* Maximum level a tree can reach for a key */ #define RT_MAX_LEVEL ((sizeof(uint64) * BITS_PER_BYTE) / RT_SPAN) @@ -797,28 +793,15 @@ RT_NODE_256_GET_CHILD(RT_NODE_256 * node, uint8 chunk) return &node->children[chunk]; } -/* - * Return the smallest shift that will allowing storing the given key. - */ -static inline int -RT_KEY_GET_SHIFT(uint64 key) -{ - if (key == 0) - return 0; - else - return (pg_leftmost_one_pos64(key) / RT_SPAN) * RT_SPAN; -} - /* * Return the max value that can be stored in the tree with the given shift. */ static uint64 RT_SHIFT_GET_MAX_VAL(int shift) { - if (shift == RT_MAX_SHIFT) - return UINT64_MAX; - else - return (UINT64CONST(1) << (shift + RT_SPAN)) - 1; + int max_shift = (sizeof(uint64) - 1) * BITS_PER_BYTE; + + return UINT64_MAX >> (max_shift - shift); } /* @@ -1574,9 +1557,8 @@ RT_NODE_INSERT(RT_RADIX_TREE * tree, RT_PTR_ALLOC * parent_slot, RT_CHILD_PTR no * and move the old node below it. */ static pg_noinline void -RT_EXTEND_UP(RT_RADIX_TREE * tree, uint64 key) +RT_EXTEND_UP(RT_RADIX_TREE * tree, uint64 key, int target_shift) { - int target_shift = RT_KEY_GET_SHIFT(key); int shift = tree->ctl->start_shift; Assert(shift < target_shift); @@ -1713,11 +1695,15 @@ RT_SET(RT_RADIX_TREE * tree, uint64 key, RT_VALUE_TYPE * value_p) /* Extend the tree if necessary */ if (unlikely(key > tree->ctl->max_val)) { + int start_shift; + + /* compute the smallest shift that will allowing storing the key */ + start_shift = pg_leftmost_one_pos64(key) / RT_SPAN * RT_SPAN; + if (tree->ctl->num_keys == 0) { RT_CHILD_PTR node; RT_NODE_4 *n4; - int start_shift = RT_KEY_GET_SHIFT(key); /* * With an empty root node, we don't extend the tree upwards, @@ -1738,7 +1724,7 @@ RT_SET(RT_RADIX_TREE * tree, uint64 key, RT_VALUE_TYPE * value_p) goto have_slot; } else - RT_EXTEND_UP(tree, key); + RT_EXTEND_UP(tree, key, start_shift); } slot = RT_GET_SLOT_RECURSIVE(tree, &tree->ctl->root, @@ -2937,7 +2923,6 @@ RT_DUMP_NODE(RT_NODE * node) #undef RT_SPAN #undef RT_NODE_MAX_SLOTS #undef RT_CHUNK_MASK -#undef RT_MAX_SHIFT #undef RT_MAX_LEVEL #undef RT_GET_KEY_CHUNK #undef RT_BM_IDX @@ -3032,7 +3017,6 @@ RT_DUMP_NODE(RT_NODE * node) #undef RT_NODE_48_GET_CHILD #undef RT_NODE_256_IS_CHUNK_USED #undef RT_NODE_256_GET_CHILD -#undef RT_KEY_GET_SHIFT #undef RT_SHIFT_GET_MAX_VAL #undef RT_NODE_SEARCH #undef RT_ADD_CHILD_4