v2-0001-radixtree-Fix-SIGSEGV-at-update-of-embeddable-val.patch

From 28a5d66682a89d22abf7cdc96777a112a9751db1 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 25 Apr 2024 10:55:20 +0900
Subject: [PATCH v2] 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.

Reported-by: Noah Misch
Author: Noah Misch
Reviewed-by: Masahiko Sawada, John Naylor
Discussion: https://postgr.es/m/20240424210319.4c.nmisch%40google.com
---
 src/include/lib/radixtree.h                   |   6 +-
 .../test_tidstore/expected/test_tidstore.out  | 116 +++++++++++++++++-
 .../test_tidstore/sql/test_tidstore.sql       |  21 +++-
 .../modules/test_tidstore/test_tidstore.c     |  15 +++
 4 files changed, 148 insertions(+), 10 deletions(-)

diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index d9f545d491..2896a6efc5 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -1749,6 +1749,10 @@ have_slot:
 
 	if (RT_VALUE_IS_EMBEDDABLE(value_p))
 	{
+		/* free the existing leaf */
+		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 +1769,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 06c610e84c..cbcacfd26e 100644
--- a/src/test/modules/test_tidstore/expected/test_tidstore.out
+++ b/src/test/modules/test_tidstore/expected/test_tidstore.out
@@ -1,7 +1,3 @@
--- Note: The test code use an array of TIDs for verification similar
--- to vacuum's dead item array pre-PG17. To avoid adding duplicates,
--- each call to do_set_block_offsets() should use different block
--- numbers.
 CREATE EXTENSION test_tidstore;
 -- To hide the output of do_set_block_offsets()
 CREATE TEMP TABLE hideblocks(blockno bigint);
@@ -79,6 +75,118 @@ 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,4,100]::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 bb31877b9a..a29e4ec1c5 100644
--- a/src/test/modules/test_tidstore/sql/test_tidstore.sql
+++ b/src/test/modules/test_tidstore/sql/test_tidstore.sql
@@ -1,8 +1,3 @@
--- Note: The test code use an array of TIDs for verification similar
--- to vacuum's dead item array pre-PG17. To avoid adding duplicates,
--- each call to do_set_block_offsets() should use different block
--- numbers.
-
 CREATE EXTENSION test_tidstore;
 
 -- To hide the output of do_set_block_offsets()
@@ -50,6 +45,22 @@ 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,4,100]::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 0a3a58722d..5417163407 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)
@@ -165,6 +177,9 @@ do_set_block_offsets(PG_FUNCTION_ARGS)
 	TidStoreSetBlockOffsets(tidstore, blkno, offs, noffs);
 	TidStoreUnlock(tidstore);
 
+	/* Remove the existing items of blkno from the verification array */
+	purge_from_verification_array(blkno);
+
 	/* Set TIDs in verification array */
 	for (int i = 0; i < noffs; i++)
 	{
-- 
2.39.3