public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrey M. Borodin <[email protected]>
To: Alexander Lakhin <[email protected]>
Cc: Michael Zhilin <[email protected]>
Cc: [email protected]
Cc: Yura Sokolov <[email protected]>
Subject: Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum
Date: Tue, 23 Jan 2024 23:09:43 +0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
> On 20 Jan 2024, at 09:00, Alexander Lakhin <[email protected]> wrote:
>
>> Alexander, do you plan to provide fixes for bugs you discovered?
>
> No, I don't have a concrete proposal how to fix those bugs. I'd thought
> that fixing the whole class of such anomalies, not only one case, is a good
> thing to do, but if it's too complicated, maybe other similar bugs could be
> put aside.
PFA draft fixes for both this errors. Alexander, Michael, Jian, what do you think?
I did not touch anything in first step - fix for original bug in this thread. However, I think that comments from Jian He worth incorporating into the fix.
Best regards, Andrey Borodin.
Attachments:
[application/octet-stream] v3-0001-contrib-amcheck-must-support-different-header-siz.patch (6.2K, 2-v3-0001-contrib-amcheck-must-support-different-header-siz.patch)
download | inline diff:
From b514df4afa0c471cea90c84c3977cbc18c13fae4 Mon Sep 17 00:00:00 2001
From: Michael Zhilin <[email protected]>
Date: Thu, 14 Dec 2023 16:08:15 +0300
Subject: [PATCH v3 1/3] contrib/amcheck: must support different header size of
short varlena datum
---
contrib/amcheck/expected/check_btree.out | 19 +++++++++++++
contrib/amcheck/sql/check_btree.sql | 17 +++++++++++
contrib/amcheck/verify_nbtree.c | 36 ++++++++++++++++++++----
3 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index 86b38d93f4..e37901c402 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -1,3 +1,5 @@
+-- directory paths are passed to us in environment variables
+\getenv abs_builddir PG_ABS_BUILDDIR
CREATE TABLE bttest_a(id int8);
CREATE TABLE bttest_b(id int8);
CREATE TABLE bttest_multi(id int8, data int8);
@@ -240,6 +242,22 @@ SELECT bt_index_check('bttest_unique_nulls_b_c_idx', heapallindexed => true, che
(1 row)
+--
+-- BUG: must support different header size of short varlena datum
+--
+CREATE TABLE varlena_bug (v text);
+ALTER TABLE varlena_bug ALTER column v SET storage plain;
+INSERT INTO varlena_bug VALUES ('x');
+\set filename :abs_builddir '/results/varlena_bug.dmp'
+COPY varlena_bug TO :'filename';
+COPY varlena_bug FROM :'filename';
+CREATE INDEX varlena_bug_idx on varlena_bug(v);
+SELECT bt_index_check('varlena_bug_idx', true);
+ bt_index_check
+----------------
+
+(1 row)
+
-- cleanup
DROP TABLE bttest_a;
DROP TABLE bttest_b;
@@ -250,3 +268,4 @@ DROP FUNCTION ifun(int8);
DROP TABLE bttest_unique_nulls;
DROP OWNED BY regress_bttest_role; -- permissions
DROP ROLE regress_bttest_role;
+DROP TABLE varlena_bug;
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index aa461f7fb9..e67eb6352f 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -1,3 +1,6 @@
+-- directory paths are passed to us in environment variables
+\getenv abs_builddir PG_ABS_BUILDDIR
+
CREATE TABLE bttest_a(id int8);
CREATE TABLE bttest_b(id int8);
CREATE TABLE bttest_multi(id int8, data int8);
@@ -148,6 +151,19 @@ SELECT bt_index_check('bttest_unique_nulls_c_key', heapallindexed => true, check
CREATE INDEX on bttest_unique_nulls (b,c);
SELECT bt_index_check('bttest_unique_nulls_b_c_idx', heapallindexed => true, checkunique => true);
+--
+-- BUG: must support different header size of short varlena datum
+--
+
+CREATE TABLE varlena_bug (v text);
+ALTER TABLE varlena_bug ALTER column v SET storage plain;
+INSERT INTO varlena_bug VALUES ('x');
+\set filename :abs_builddir '/results/varlena_bug.dmp'
+COPY varlena_bug TO :'filename';
+COPY varlena_bug FROM :'filename';
+CREATE INDEX varlena_bug_idx on varlena_bug(v);
+SELECT bt_index_check('varlena_bug_idx', true);
+
-- cleanup
DROP TABLE bttest_a;
DROP TABLE bttest_b;
@@ -158,3 +174,4 @@ DROP FUNCTION ifun(int8);
DROP TABLE bttest_unique_nulls;
DROP OWNED BY regress_bttest_role; -- permissions
DROP ROLE regress_bttest_role;
+DROP TABLE varlena_bug;
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 91caa53dd8..e7f01c2add 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -2942,7 +2942,7 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
TupleDesc tupleDescriptor = RelationGetDescr(state->rel);
Datum normalized[INDEX_MAX_KEYS];
bool isnull[INDEX_MAX_KEYS];
- bool toast_free[INDEX_MAX_KEYS];
+ bool need_free[INDEX_MAX_KEYS];
bool formnewtup = false;
IndexTuple reformed;
int i;
@@ -2961,7 +2961,7 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
att = TupleDescAttr(tupleDescriptor, i);
/* Assume untoasted/already normalized datum initially */
- toast_free[i] = false;
+ need_free[i] = false;
normalized[i] = index_getattr(itup, att->attnum,
tupleDescriptor,
&isnull[i]);
@@ -2973,6 +2973,7 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
* index without further processing, so an external varlena header
* should never be encountered here
*/
+
if (VARATT_IS_EXTERNAL(DatumGetPointer(normalized[i])))
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
@@ -2984,11 +2985,32 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
{
formnewtup = true;
normalized[i] = PointerGetDatum(PG_DETOAST_DATUM(normalized[i]));
- toast_free[i] = true;
+ need_free[i] = true;
}
+ /*
+ * Short tuples may have 1B or 4B header. Convert 4B header of short
+ * tuples to 1B
+ */
+ else if (VARATT_CAN_MAKE_SHORT(DatumGetPointer(normalized[i])))
+ {
+ /* convert to short varlena */
+ Size len = VARATT_CONVERTED_SHORT_SIZE(DatumGetPointer(normalized[i]));
+ char *data = palloc(len);
+
+ SET_VARSIZE_SHORT(data, len);
+ memcpy(data + 1, VARDATA(DatumGetPointer(normalized[i])), len - 1);
+
+ formnewtup = true;
+ normalized[i] = PointerGetDatum(data);
+ need_free[i] = true;
+ }
+
}
- /* Easier case: Tuple has varlena datums, none of which are compressed */
+ /*
+ * Easier case: Tuple has varlena datums, none of which are compressed or
+ * short with 4B header
+ */
if (!formnewtup)
return itup;
@@ -2997,6 +3019,10 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
* creating normalized version of the tuple from uncompressed input datums
* (normalized input datums). This is rather naive, but shouldn't be
* necessary too often.
+ * Also tuple had short varlena datums with 4B header. Actually there is no
+ * restriction with have heap tuple containing varlena datum with 4B header
+ * and corresponding index tuple containing varlena datum with 1B header.
+ * For fingerprinting let's convert heap tuple varlena datum to 1B format.
*
* Note that we rely on deterministic index_form_tuple() TOAST compression
* of normalized input.
@@ -3006,7 +3032,7 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
/* Cannot leak memory here */
for (i = 0; i < tupleDescriptor->natts; i++)
- if (toast_free[i])
+ if (need_free[i])
pfree(DatumGetPointer(normalized[i]));
return reformed;
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] v3-0003-amcheck-avoid-failing-on-oversized-tuples.patch (6.3K, 3-v3-0003-amcheck-avoid-failing-on-oversized-tuples.patch)
download | inline diff:
From 01dd6155276ed97adcad3e5c55c064b7b8116bfd Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email protected]>
Date: Tue, 23 Jan 2024 23:03:28 +0500
Subject: [PATCH v3 3/3] amcheck: avoid failing on oversized tuples
Due to changes in toast policies, some heap tuples might become too
big index tuples. This commit prevents ERRORs in this case, because
this anomaly does not create dangerous conditions to DMS. However,
the database cannot be dumped-restored, so we emit a NOTICE.
Reported-by: Alexander Lakhin
---
contrib/amcheck/expected/check_btree.out | 14 ++++++
contrib/amcheck/sql/check_btree.sql | 11 +++++
contrib/amcheck/verify_nbtree.c | 54 +++++++++++++++++++++---
3 files changed, 73 insertions(+), 6 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index 36b1ebe34c..91394bed2b 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -270,6 +270,20 @@ SELECT bt_index_check('tbl_idx', true);
(1 row)
+DROP TABLE tbl;
+-- Check oversized datums that cannot be inserted into index
+CREATE TABLE t(f1 text);
+CREATE INDEX t_idx ON t(f1);
+INSERT INTO t VALUES(repeat('1234567890', 1000));
+ALTER TABLE t ALTER COLUMN f1 SET STORAGE plain;
+SELECT bt_index_check('t_idx', true);
+NOTICE: Index contain tuples that cannot fit into index page, if toasted with current toast policy
+ bt_index_check
+----------------
+
+(1 row)
+
+DROP TABLE t;
-- cleanup
DROP TABLE bttest_a;
DROP TABLE bttest_b;
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index 0a827edd1a..cdfbc8a141 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -172,6 +172,17 @@ INSERT INTO tbl VALUES (repeat('Test', 250));
ALTER TABLE tbl ALTER COLUMN t SET STORAGE extended;
SELECT bt_index_check('tbl_idx', true);
+DROP TABLE tbl;
+
+-- Check oversized datums that cannot be inserted into index
+
+CREATE TABLE t(f1 text);
+CREATE INDEX t_idx ON t(f1);
+INSERT INTO t VALUES(repeat('1234567890', 1000));
+ALTER TABLE t ALTER COLUMN f1 SET STORAGE plain;
+
+SELECT bt_index_check('t_idx', true);
+DROP TABLE t;
-- cleanup
DROP TABLE bttest_a;
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 6654b5afe7..f70e01191b 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -128,6 +128,11 @@ typedef struct BtreeCheckState
bloom_filter *filter;
/* Debug counter */
int64 heaptuplespresent;
+ /*
+ * During check we might find tuples that due to current TOAST policies
+ * should not reside in index, but still are there.
+ */
+ bool has_oversized_tuples;
} BtreeCheckState;
/*
@@ -1624,10 +1629,17 @@ bt_target_page_check(BtreeCheckState *state)
logtuple = bt_posting_plain_tuple(itup, i);
norm = bt_normalize_tuple(state, logtuple);
- bloom_add_element(state->filter, (unsigned char *) norm,
+ if (norm == NULL)
+ {
+ if (!state->has_oversized_tuples)
+ elog(NOTICE, "Index contain tuples that cannot fit into index page, if toasted with current toast policy");
+ state->has_oversized_tuples = true;
+ }
+ else
+ bloom_add_element(state->filter, (unsigned char *) norm,
IndexTupleSize(norm));
/* Be tidy */
- if (norm != logtuple)
+ if (norm != logtuple && norm != NULL)
pfree(norm);
pfree(logtuple);
}
@@ -1635,10 +1647,17 @@ bt_target_page_check(BtreeCheckState *state)
else
{
norm = bt_normalize_tuple(state, itup);
- bloom_add_element(state->filter, (unsigned char *) norm,
- IndexTupleSize(norm));
+ if (norm == NULL)
+ {
+ if (!state->has_oversized_tuples)
+ elog(NOTICE, "Index contain tuples that cannot fit into index page, if toasted with current toast policy");
+ state->has_oversized_tuples = true;
+ }
+ else
+ bloom_add_element(state->filter, (unsigned char *) norm,
+ IndexTupleSize(norm));
/* Be tidy */
- if (norm != itup)
+ if (norm != itup && norm != NULL)
pfree(norm);
}
}
@@ -2876,6 +2895,7 @@ bt_tuple_present_callback(Relation index, ItemPointer tid, Datum *values,
BtreeCheckState *state = (BtreeCheckState *) checkstate;
IndexTuple itup,
norm;
+ bool miss_oversized_tuple = false;
Assert(state->heapallindexed);
@@ -2883,9 +2903,19 @@ bt_tuple_present_callback(Relation index, ItemPointer tid, Datum *values,
itup = index_form_tuple(RelationGetDescr(index), values, isnull);
itup->t_tid = *tid;
norm = bt_normalize_tuple(state, itup);
+ if (norm == NULL)
+ {
+ if (state->has_oversized_tuples)
+ {
+ /* exempt this oversized tuple */
+ state->heaptuplespresent++;
+ pfree(itup);
+ return;
+ }
+ }
/* Probe Bloom filter -- tuple should be present */
- if (bloom_lacks_element(state->filter, (unsigned char *) norm,
+ if ((norm == NULL) || bloom_lacks_element(state->filter, (unsigned char *) norm,
IndexTupleSize(norm)))
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
@@ -2936,6 +2966,9 @@ bt_tuple_present_callback(Relation index, ItemPointer tid, Datum *values,
* Caller does normalization for non-pivot tuples that have a posting list,
* since dummy CREATE INDEX callback code generates new tuples with the same
* normalized representation.
+ *
+ * If the tuple is exampt from checking due to has_oversized_tuples this function
+ * returns NULL.
*/
static IndexTuple
bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
@@ -2947,6 +2980,7 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
bool formnewtup = false;
IndexTuple reformed;
int i;
+ Size data_size;
/* Caller should only pass "logical" non-pivot tuples here */
Assert(!BTreeTupleIsPosting(itup) && !BTreeTupleIsPivot(itup));
@@ -3026,6 +3060,14 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
if (!formnewtup)
return itup;
+ data_size = MAXALIGN(heap_compute_data_size(tupleDescriptor,
+ normalized, isnull)
+ + MAXALIGN(sizeof(IndexTupleData) + sizeof(IndexAttributeBitMapData)));
+ if ((data_size & INDEX_SIZE_MASK) != data_size)
+ {
+ return NULL;
+ }
+
/*
* Hard case: Tuple had compressed varlena datums that necessitate
* creating normalized version of the tuple from uncompressed input datums
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] v3-0002-amcheck-prevent-false-positives-from-extended-dat.patch (2.9K, 4-v3-0002-amcheck-prevent-false-positives-from-extended-dat.patch)
download | inline diff:
From e2ab4c0d3ad9777f70fccb21dd446e248cd5f52a Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email protected]>
Date: Tue, 23 Jan 2024 21:57:02 +0500
Subject: [PATCH v3 2/3] amcheck: prevent false positives from extended datums
Reported-by: Alexander Lakhin
---
contrib/amcheck/expected/check_btree.out | 12 ++++++++++++
contrib/amcheck/sql/check_btree.sql | 9 +++++++++
contrib/amcheck/verify_nbtree.c | 12 ++++++++++++
3 files changed, 33 insertions(+)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index e37901c402..36b1ebe34c 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -258,6 +258,18 @@ SELECT bt_index_check('varlena_bug_idx', true);
(1 row)
+-- Check extended varlena
+CREATE TABLE tbl(t text);
+ALTER TABLE tbl ALTER COLUMN t SET STORAGE plain;
+CREATE INDEX tbl_idx ON tbl (t);
+INSERT INTO tbl VALUES (repeat('Test', 250));
+ALTER TABLE tbl ALTER COLUMN t SET STORAGE extended;
+SELECT bt_index_check('tbl_idx', true);
+ bt_index_check
+----------------
+
+(1 row)
+
-- cleanup
DROP TABLE bttest_a;
DROP TABLE bttest_b;
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index e67eb6352f..0a827edd1a 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -164,6 +164,15 @@ COPY varlena_bug FROM :'filename';
CREATE INDEX varlena_bug_idx on varlena_bug(v);
SELECT bt_index_check('varlena_bug_idx', true);
+-- Check extended varlena
+CREATE TABLE tbl(t text);
+ALTER TABLE tbl ALTER COLUMN t SET STORAGE plain;
+CREATE INDEX tbl_idx ON tbl (t);
+INSERT INTO tbl VALUES (repeat('Test', 250));
+ALTER TABLE tbl ALTER COLUMN t SET STORAGE extended;
+
+SELECT bt_index_check('tbl_idx', true);
+
-- cleanup
DROP TABLE bttest_a;
DROP TABLE bttest_b;
diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index e7f01c2add..6654b5afe7 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -23,6 +23,7 @@
*/
#include "postgres.h"
+#include "access/heaptoast.h"
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/table.h"
@@ -2981,6 +2982,17 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
ItemPointerGetBlockNumber(&(itup->t_tid)),
ItemPointerGetOffsetNumber(&(itup->t_tid)),
RelationGetRelationName(state->rel))));
+ else if (!VARATT_IS_EXTENDED(DatumGetPointer(normalized[i])) &&
+ VARSIZE(DatumGetPointer(normalized[i])) > TOAST_INDEX_TARGET &&
+ (att->attstorage == TYPSTORAGE_EXTENDED ||
+ att->attstorage == TYPSTORAGE_MAIN))
+ {
+ /*
+ * this attribute will be compressed by index_form_tuple(),
+ * this might be already done in heap, so force forming.
+ */
+ formnewtup = true;
+ }
else if (VARATT_IS_COMPRESSED(DatumGetPointer(normalized[i])))
{
formnewtup = true;
--
2.37.1 (Apple Git-137.1)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox