Skip to content

Commit bae31e7

Browse files
tvondratvondra
committed
Properly detoast data in brin_form_tuple
brin_form_tuple failed to consider the values may be toasted, inserting the toast pointer into the index. This may easily result in index corruption, as the toast data may be deleted and cleaned up by vacuum. The cleanup however does not care about indexes, leaving invalid toast pointers behind, which triggers errors like this: ERROR: missing chunk number 0 for toast value 16433 in pg_toast_16426 A less severe consequence are inconsistent failures due to the index row being too large, depending on whether brin_form_tuple operated on plain or toasted version of the row. For example CREATE TABLE t (val TEXT); INSERT INTO t VALUES ('... long value ...') CREATE INDEX idx ON t USING brin (val); would likely succeed, as the row would likely include toast pointer. Switching the order of INSERT and CREATE INDEX would likely fail: ERROR: index row size 8712 exceeds maximum 8152 for index "idx" because this happens before the row values are toasted. The bug exists since PostgreSQL 9.5 where BRIN indexes were introduced. So backpatch all the way back. Author: Tomas Vondra Reviewed-by: Alvaro Herrera Backpatch-through: 9.5 Discussion: https://postgr.es/m/20201001184133.oq5uq75sb45pu3aw@development Discussion: https://postgr.es/m/20201104010544.zexj52mlldagzowv%40development
1 parent 9e55518 commit bae31e7

File tree

3 files changed

+172
-1
lines changed

3 files changed

+172
-1
lines changed

‎src/backend/access/brin/brin_tuple.c

Lines changed: 90 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "access/brin_tuple.h"
3636
#include "access/tupdesc.h"
3737
#include "access/tupmacs.h"
38+
#include "access/tuptoaster.h"
3839
#include "utils/datum.h"
3940
#include "utils/memutils.h"
4041

@@ -100,6 +101,12 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
100101
Size len,
101102
hoff,
102103
data_len;
104+
int i;
105+
106+
#ifdef TOAST_INDEX_HACK
107+
Datum *untoasted_values;
108+
int nuntoasted = 0;
109+
#endif
103110

104111
Assert(brdesc->bd_totalstored > 0);
105112

@@ -108,6 +115,10 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
108115
phony_nullbitmap = (bits8 *)
109116
palloc(sizeof(bits8) * BITMAPLEN(brdesc->bd_totalstored));
110117

118+
#ifdef TOAST_INDEX_HACK
119+
untoasted_values = (Datum *) palloc(sizeof(Datum) * brdesc->bd_totalstored);
120+
#endif
121+
111122
/*
112123
* Set up the values/nulls arrays for heap_fill_tuple
113124
*/
@@ -139,10 +150,83 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
139150
if (tuple->bt_columns[keyno].bv_hasnulls)
140151
anynulls = true;
141152

153+
/*
154+
* Now obtain the values of each stored datum. Note that some values
155+
* might be toasted, and we cannot rely on the original heap values
156+
* sticking around forever, so we must detoast them. Also try to
157+
* compress them.
158+
*/
142159
for (datumno = 0;
143160
datumno < brdesc->bd_info[keyno]->oi_nstored;
144161
datumno++)
145-
values[idxattno++] = tuple->bt_columns[keyno].bv_values[datumno];
162+
{
163+
Datum value = tuple->bt_columns[keyno].bv_values[datumno];
164+
165+
#ifdef TOAST_INDEX_HACK
166+
167+
/* We must look at the stored type, not at the index descriptor. */
168+
TypeCacheEntry *atttype = brdesc->bd_info[keyno]->oi_typcache[datumno];
169+
170+
/* Do we need to free the value at the end? */
171+
bool free_value = false;
172+
173+
/* For non-varlena types we don't need to do anything special */
174+
if (atttype->typlen != -1)
175+
{
176+
values[idxattno++] = value;
177+
continue;
178+
}
179+
180+
/*
181+
* Do nothing if value is not of varlena type. We don't need to
182+
* care about NULL values here, thanks to bv_allnulls above.
183+
*
184+
* If value is stored EXTERNAL, must fetch it so we are not
185+
* depending on outside storage.
186+
*
187+
* XXX Is this actually true? Could it be that the summary is
188+
* NULL even for range with non-NULL data? E.g. degenerate bloom
189+
* filter may be thrown away, etc.
190+
*/
191+
if (VARATT_IS_EXTERNAL(DatumGetPointer(value)))
192+
{
193+
value = PointerGetDatum(heap_tuple_fetch_attr((struct varlena *)
194+
DatumGetPointer(value)));
195+
free_value = true;
196+
}
197+
198+
/*
199+
* If value is above size target, and is of a compressible datatype,
200+
* try to compress it in-line.
201+
*/
202+
if (!VARATT_IS_EXTENDED(DatumGetPointer(value)) &&
203+
VARSIZE(DatumGetPointer(value)) > TOAST_INDEX_TARGET &&
204+
(atttype->typstorage == 'x' || atttype->typstorage == 'm'))
205+
{
206+
Datum cvalue = toast_compress_datum(value);
207+
208+
if (DatumGetPointer(cvalue) != NULL)
209+
{
210+
/* successful compression */
211+
if (free_value)
212+
pfree(DatumGetPointer(value));
213+
214+
value = cvalue;
215+
free_value = true;
216+
}
217+
}
218+
219+
/*
220+
* If we untoasted / compressed the value, we need to free it
221+
* after forming the index tuple.
222+
*/
223+
if (free_value)
224+
untoasted_values[nuntoasted++] = value;
225+
226+
#endif
227+
228+
values[idxattno++] = value;
229+
}
146230
}
147231

148232
/* Assert we did not overrun temp arrays */
@@ -194,6 +278,11 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
194278
pfree(nulls);
195279
pfree(phony_nullbitmap);
196280

281+
#ifdef TOAST_INDEX_HACK
282+
for (i = 0; i < nuntoasted; i++)
283+
pfree(DatumGetPointer(untoasted_values[i]));
284+
#endif
285+
197286
/*
198287
* Now fill in the real null bitmasks. allnulls first.
199288
*/

‎src/test/regress/expected/brin.out

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,3 +418,45 @@ SELECT brin_summarize_new_values('brinidx'); -- ok, no change expected
418418
0
419419
(1 row)
420420

421+
-- make sure data are properly de-toasted in BRIN index
422+
CREATE TABLE brintest_3 (a text, b text, c text, d text);
423+
-- long random strings (~2000 chars each, so ~6kB for min/max on two
424+
-- columns) to trigger toasting
425+
WITH rand_value AS (SELECT string_agg(md5(i::text),'') AS val FROM generate_series(1,60) s(i))
426+
INSERT INTO brintest_3
427+
SELECT val, val, val, val FROM rand_value;
428+
CREATE INDEX brin_test_toast_idx ON brintest_3 USING brin (b, c);
429+
DELETE FROM brintest_3;
430+
-- We need to wait a bit for all transactions to complete, so that the
431+
-- vacuum actually removes the TOAST rows. Creating an index concurrently
432+
-- is a one way to achieve that, because it does exactly such wait.
433+
CREATE INDEX CONCURRENTLY brin_test_temp_idx ON brintest_3(a);
434+
DROP INDEX brin_test_temp_idx;
435+
-- vacuum the table, to discard TOAST data
436+
VACUUM brintest_3;
437+
-- retry insert with a different random-looking (but deterministic) value
438+
-- the value is different, and so should replace either min or max in the
439+
-- brin summary
440+
WITH rand_value AS (SELECT string_agg(md5((-i)::text),'') AS val FROM generate_series(1,60) s(i))
441+
INSERT INTO brintest_3
442+
SELECT val, val, val, val FROM rand_value;
443+
-- now try some queries, accessing the brin index
444+
SET enable_seqscan = off;
445+
SET enable_bitmapscan = on;
446+
EXPLAIN (COSTS OFF)
447+
SELECT * FROM brintest_3 WHERE b < '0';
448+
QUERY PLAN
449+
------------------------------------------------
450+
Bitmap Heap Scan on brintest_3
451+
Recheck Cond: (b < '0'::text)
452+
-> Bitmap Index Scan on brin_test_toast_idx
453+
Index Cond: (b < '0'::text)
454+
(4 rows)
455+
456+
SELECT * FROM brintest_3 WHERE b < '0';
457+
a | b | c | d
458+
---+---+---+---
459+
(0 rows)
460+
461+
DROP TABLE brintest_3;
462+
RESET enable_seqscan;

‎src/test/regress/sql/brin.sql

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,3 +421,43 @@ UPDATE brintest SET textcol = '' WHERE textcol IS NOT NULL;
421421
SELECT brin_summarize_new_values('brintest'); -- error, not an index
422422
SELECT brin_summarize_new_values('tenk1_unique1'); -- error, not a BRIN index
423423
SELECT brin_summarize_new_values('brinidx'); -- ok, no change expected
424+
425+
-- make sure data are properly de-toasted in BRIN index
426+
CREATE TABLE brintest_3 (a text, b text, c text, d text);
427+
428+
-- long random strings (~2000 chars each, so ~6kB for min/max on two
429+
-- columns) to trigger toasting
430+
WITH rand_value AS (SELECT string_agg(md5(i::text),'') AS val FROM generate_series(1,60) s(i))
431+
INSERT INTO brintest_3
432+
SELECT val, val, val, val FROM rand_value;
433+
434+
CREATE INDEX brin_test_toast_idx ON brintest_3 USING brin (b, c);
435+
DELETE FROM brintest_3;
436+
437+
-- We need to wait a bit for all transactions to complete, so that the
438+
-- vacuum actually removes the TOAST rows. Creating an index concurrently
439+
-- is a one way to achieve that, because it does exactly such wait.
440+
CREATE INDEX CONCURRENTLY brin_test_temp_idx ON brintest_3(a);
441+
DROP INDEX brin_test_temp_idx;
442+
443+
-- vacuum the table, to discard TOAST data
444+
VACUUM brintest_3;
445+
446+
-- retry insert with a different random-looking (but deterministic) value
447+
-- the value is different, and so should replace either min or max in the
448+
-- brin summary
449+
WITH rand_value AS (SELECT string_agg(md5((-i)::text),'') AS val FROM generate_series(1,60) s(i))
450+
INSERT INTO brintest_3
451+
SELECT val, val, val, val FROM rand_value;
452+
453+
-- now try some queries, accessing the brin index
454+
SET enable_seqscan = off;
455+
SET enable_bitmapscan = on;
456+
457+
EXPLAIN (COSTS OFF)
458+
SELECT * FROM brintest_3 WHERE b < '0';
459+
460+
SELECT * FROM brintest_3 WHERE b < '0';
461+
462+
DROP TABLE brintest_3;
463+
RESET enable_seqscan;

0 commit comments

Comments
 (0)