Author: Noah Misch Commit: Noah Misch 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;