On Mon, Apr 15, 2024 at 04:12:38PM +0700, John Naylor wrote:
> - Some paths for single-value leaves are not covered:
>
> https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L904
> https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L954
> https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L2606
>
> However, these paths do get regression test coverage on 32-bit
> machines. 64-bit builds only have leaves in the TID store, which
> doesn't (currently) delete entries, and doesn't instantiate the tree
> with the debug option.
>
> - In RT_SET "if (found)" is not covered:
>
> https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L1768
>
> That's because we don't yet have code that replaces an existing value
> with a value of a different length.
I saw a SIGSEGV there when using tidstore to write a fix for something else.
Patch attached.
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
radixtree: Fix SIGSEGV at update of embeddable value to non-embeddable.
Also, fix a memory leak when updating from non-embeddable to embeddable.
Both were unreachable without adding C code.
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index dc4c00d..aa8f44c 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -1749,6 +1749,9 @@ have_slot:
if (RT_VALUE_IS_EMBEDDABLE(value_p))
{
+ if (found && !RT_CHILDPTR_IS_VALUE(*slot))
+ RT_FREE_LEAF(tree, *slot);
+
/* store value directly in child pointer slot */
memcpy(slot, value_p, value_sz);
@@ -1765,7 +1768,7 @@ have_slot:
{
RT_CHILD_PTR leaf;
- if (found)
+ if (found && !RT_CHILDPTR_IS_VALUE(*slot))
{
Assert(RT_PTR_ALLOC_IS_VALID(*slot));
leaf.alloc = *slot;
diff --git a/src/test/modules/test_tidstore/expected/test_tidstore.out
b/src/test/modules/test_tidstore/expected/test_tidstore.out
index 06c610e..d116927 100644
--- a/src/test/modules/test_tidstore/expected/test_tidstore.out
+++ b/src/test/modules/test_tidstore/expected/test_tidstore.out
@@ -79,6 +79,96 @@ SELECT test_destroy();
(1 row)
+-- Test replacements crossing RT_CHILDPTR_IS_VALUE in both directions
+SELECT test_create(false);
+ test_create
+-------------
+
+(1 row)
+
+SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT
check_set_block_offsets();
+ do_set_block_offsets
+----------------------
+ 1
+(1 row)
+
+ check_set_block_offsets
+-------------------------
+
+(1 row)
+
+SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT
check_set_block_offsets();
+ do_set_block_offsets
+----------------------
+ 1
+(1 row)
+
+ check_set_block_offsets
+-------------------------
+
+(1 row)
+
+SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT
check_set_block_offsets();
+ do_set_block_offsets
+----------------------
+ 1
+(1 row)
+
+ check_set_block_offsets
+-------------------------
+
+(1 row)
+
+SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT
check_set_block_offsets();
+ do_set_block_offsets
+----------------------
+ 1
+(1 row)
+
+ check_set_block_offsets
+-------------------------
+
+(1 row)
+
+SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT
check_set_block_offsets();
+ do_set_block_offsets
+----------------------
+ 1
+(1 row)
+
+ check_set_block_offsets
+-------------------------
+
+(1 row)
+
+SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT
check_set_block_offsets();
+ do_set_block_offsets
+----------------------
+ 1
+(1 row)
+
+ check_set_block_offsets
+-------------------------
+
+(1 row)
+
+SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT
check_set_block_offsets();
+ do_set_block_offsets
+----------------------
+ 1
+(1 row)
+
+ check_set_block_offsets
+-------------------------
+
+(1 row)
+
+SELECT test_destroy();
+ test_destroy
+--------------
+
+(1 row)
+
-- Use shared memory this time. We can't do that in test_radixtree.sql,
-- because unused static functions would raise warnings there.
SELECT test_create(true);
diff --git a/src/test/modules/test_tidstore/sql/test_tidstore.sql
b/src/test/modules/test_tidstore/sql/test_tidstore.sql
index bb31877..704d869 100644
--- a/src/test/modules/test_tidstore/sql/test_tidstore.sql
+++ b/src/test/modules/test_tidstore/sql/test_tidstore.sql
@@ -14,6 +14,7 @@ CREATE TEMP TABLE hideblocks(blockno bigint);
-- We use a higher number to test tidstore.
\set maxoffset 512
+
SELECT test_create(false);
-- Test on empty tidstore.
@@ -50,6 +51,20 @@ SELECT test_is_full();
-- Re-create the TID store for randommized tests.
SELECT test_destroy();
+
+
+-- Test replacements crossing RT_CHILDPTR_IS_VALUE in both directions
+SELECT test_create(false);
+SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT
check_set_block_offsets();
+SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT
check_set_block_offsets();
+SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT
check_set_block_offsets();
+SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT
check_set_block_offsets();
+SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT
check_set_block_offsets();
+SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT
check_set_block_offsets();
+SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT
check_set_block_offsets();
+SELECT test_destroy();
+
+
-- Use shared memory this time. We can't do that in test_radixtree.sql,
-- because unused static functions would raise warnings there.
SELECT test_create(true);
diff --git a/src/test/modules/test_tidstore/test_tidstore.c
b/src/test/modules/test_tidstore/test_tidstore.c
index 0a3a587..dac3b97 100644
--- a/src/test/modules/test_tidstore/test_tidstore.c
+++ b/src/test/modules/test_tidstore/test_tidstore.c
@@ -146,6 +146,18 @@ sanity_check_array(ArrayType *ta)
errmsg("argument must be empty or
one-dimensional array")));
}
+static void
+purge_from_verification_array(BlockNumber blkno)
+{
+ int dst = 0;
+
+ for (int src = 0; src < items.num_tids; src++)
+ if (ItemPointerGetBlockNumber(&items.insert_tids[src]) != blkno)
+ items.insert_tids[dst++] = items.insert_tids[src];
+ items.num_tids = dst;
+}
+
+
/* Set the given block and offsets pairs */
Datum
do_set_block_offsets(PG_FUNCTION_ARGS)
@@ -166,6 +178,7 @@ do_set_block_offsets(PG_FUNCTION_ARGS)
TidStoreUnlock(tidstore);
/* Set TIDs in verification array */
+ purge_from_verification_array(blkno);
for (int i = 0; i < noffs; i++)
{
ItemPointer tid;
