tidstore-overwrite-v1.patch

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@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;