Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit bb7f195

Browse files
MasahikoSawadaMasahikoSawada
committed
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
1 parent e03ec46 commit bb7f195

File tree

4 files changed

+148
-10
lines changed

4 files changed

+148
-10
lines changed

‎src/include/lib/radixtree.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1749,6 +1749,10 @@ RT_SET(RT_RADIX_TREE * tree, uint64 key, RT_VALUE_TYPE * value_p)
17491749

17501750
if (RT_VALUE_IS_EMBEDDABLE(value_p))
17511751
{
1752+
/* free the existing leaf */
1753+
if (found && !RT_CHILDPTR_IS_VALUE(*slot))
1754+
RT_FREE_LEAF(tree, *slot);
1755+
17521756
/* store value directly in child pointer slot */
17531757
memcpy(slot, value_p, value_sz);
17541758

@@ -1765,7 +1769,7 @@ RT_SET(RT_RADIX_TREE * tree, uint64 key, RT_VALUE_TYPE * value_p)
17651769
{
17661770
RT_CHILD_PTR leaf;
17671771

1768-
if (found)
1772+
if (found && !RT_CHILDPTR_IS_VALUE(*slot))
17691773
{
17701774
Assert(RT_PTR_ALLOC_IS_VALID(*slot));
17711775
leaf.alloc = *slot;

‎src/test/modules/test_tidstore/expected/test_tidstore.out

Lines changed: 112 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,3 @@
1-
-- Note: The test code use an array of TIDs for verification similar
2-
-- to vacuum's dead item array pre-PG17. To avoid adding duplicates,
3-
-- each call to do_set_block_offsets() should use different block
4-
-- numbers.
51
CREATE EXTENSION test_tidstore;
62
-- To hide the output of do_set_block_offsets()
73
CREATE TEMP TABLE hideblocks(blockno bigint);
@@ -79,6 +75,118 @@ SELECT test_destroy();
7975

8076
(1 row)
8177

78+
-- Test replacements crossing RT_CHILDPTR_IS_VALUE in both directions
79+
SELECT test_create(false);
80+
test_create
81+
-------------
82+
83+
(1 row)
84+
85+
SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT check_set_block_offsets();
86+
do_set_block_offsets
87+
----------------------
88+
1
89+
(1 row)
90+
91+
check_set_block_offsets
92+
-------------------------
93+
94+
(1 row)
95+
96+
SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT check_set_block_offsets();
97+
do_set_block_offsets
98+
----------------------
99+
1
100+
(1 row)
101+
102+
check_set_block_offsets
103+
-------------------------
104+
105+
(1 row)
106+
107+
SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT check_set_block_offsets();
108+
do_set_block_offsets
109+
----------------------
110+
1
111+
(1 row)
112+
113+
check_set_block_offsets
114+
-------------------------
115+
116+
(1 row)
117+
118+
SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT check_set_block_offsets();
119+
do_set_block_offsets
120+
----------------------
121+
1
122+
(1 row)
123+
124+
check_set_block_offsets
125+
-------------------------
126+
127+
(1 row)
128+
129+
SELECT do_set_block_offsets(1, array[1,2,3,4,100]::int2[]); SELECT check_set_block_offsets();
130+
do_set_block_offsets
131+
----------------------
132+
1
133+
(1 row)
134+
135+
check_set_block_offsets
136+
-------------------------
137+
138+
(1 row)
139+
140+
SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT check_set_block_offsets();
141+
do_set_block_offsets
142+
----------------------
143+
1
144+
(1 row)
145+
146+
check_set_block_offsets
147+
-------------------------
148+
149+
(1 row)
150+
151+
SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT check_set_block_offsets();
152+
do_set_block_offsets
153+
----------------------
154+
1
155+
(1 row)
156+
157+
check_set_block_offsets
158+
-------------------------
159+
160+
(1 row)
161+
162+
SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT check_set_block_offsets();
163+
do_set_block_offsets
164+
----------------------
165+
1
166+
(1 row)
167+
168+
check_set_block_offsets
169+
-------------------------
170+
171+
(1 row)
172+
173+
SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT check_set_block_offsets();
174+
do_set_block_offsets
175+
----------------------
176+
1
177+
(1 row)
178+
179+
check_set_block_offsets
180+
-------------------------
181+
182+
(1 row)
183+
184+
SELECT test_destroy();
185+
test_destroy
186+
--------------
187+
188+
(1 row)
189+
82190
-- Use shared memory this time. We can't do that in test_radixtree.sql,
83191
-- because unused static functions would raise warnings there.
84192
SELECT test_create(true);

‎src/test/modules/test_tidstore/sql/test_tidstore.sql

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,3 @@
1-
-- Note: The test code use an array of TIDs for verification similar
2-
-- to vacuum's dead item array pre-PG17. To avoid adding duplicates,
3-
-- each call to do_set_block_offsets() should use different block
4-
-- numbers.
5-
61
CREATE EXTENSION test_tidstore;
72

83
-- To hide the output of do_set_block_offsets()
@@ -50,6 +45,22 @@ SELECT test_is_full();
5045

5146
-- Re-create the TID store for randommized tests.
5247
SELECT test_destroy();
48+
49+
50+
-- Test replacements crossing RT_CHILDPTR_IS_VALUE in both directions
51+
SELECT test_create(false);
52+
SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT check_set_block_offsets();
53+
SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT check_set_block_offsets();
54+
SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT check_set_block_offsets();
55+
SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT check_set_block_offsets();
56+
SELECT do_set_block_offsets(1, array[1,2,3,4,100]::int2[]); SELECT check_set_block_offsets();
57+
SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT check_set_block_offsets();
58+
SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT check_set_block_offsets();
59+
SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT check_set_block_offsets();
60+
SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT check_set_block_offsets();
61+
SELECT test_destroy();
62+
63+
5364
-- Use shared memory this time. We can't do that in test_radixtree.sql,
5465
-- because unused static functions would raise warnings there.
5566
SELECT test_create(true);

‎src/test/modules/test_tidstore/test_tidstore.c

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,18 @@ sanity_check_array(ArrayType *ta)
146146
errmsg("argument must be empty or one-dimensional array")));
147147
}
148148

149+
static void
150+
purge_from_verification_array(BlockNumber blkno)
151+
{
152+
int dst = 0;
153+
154+
for (int src = 0; src < items.num_tids; src++)
155+
if (ItemPointerGetBlockNumber(&items.insert_tids[src]) != blkno)
156+
items.insert_tids[dst++] = items.insert_tids[src];
157+
items.num_tids = dst;
158+
}
159+
160+
149161
/* Set the given block and offsets pairs */
150162
Datum
151163
do_set_block_offsets(PG_FUNCTION_ARGS)
@@ -165,6 +177,9 @@ do_set_block_offsets(PG_FUNCTION_ARGS)
165177
TidStoreSetBlockOffsets(tidstore, blkno, offs, noffs);
166178
TidStoreUnlock(tidstore);
167179

180+
/* Remove the existing items of blkno from the verification array */
181+
purge_from_verification_array(blkno);
182+
168183
/* Set TIDs in verification array */
169184
for (int i = 0; i < noffs; i++)
170185
{

0 commit comments

Comments
 (0)