public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Zhilin <[email protected]>
To: Andrey M. Borodin <[email protected]>
To: Alexander Lakhin <[email protected]>
To: [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 22:24:11 +0300
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]>
<[email protected]>
Hi,
Thank you, Jian, for nice comments!
PFA version with your recommendations.
Andrey,
I didn't yet check your patches, but at least compiler complains about
added, but unused variable "miss_oversized_tuple".
verify_nbtree.c:2898:7: warning: unused variable 'miss_oversized_tuple'
[-Wunused-variable]
bool miss_oversized_tuple = false;
So patch has been updated to fix this warning.
Attached v4, rebased version with Jian's comments & removed unused variable.
Thanks,
Michael.
On 1/23/24 21:09, Andrey M. Borodin wrote:
>
>> 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.
>
--
Michael Zhilin
Postgres Professional
+7(925)3366270
https://www.postgrespro.ru
Attachments:
[text/x-patch] v4-0001-contrib-amcheck-must-support-different-header-siz.patch (6.1K, 3-v4-0001-contrib-amcheck-must-support-different-header-siz.patch)
download | inline diff:
From 3cf43195e54439433854cb434a27e851b7ac6528 Mon Sep 17 00:00:00 2001
From: Michael Zhilin <[email protected]>
Date: Thu, 14 Dec 2023 16:08:15 +0300
Subject: [PATCH v4 1/3] contrib/amcheck: must support different header size of
short varlena datum
---
contrib/amcheck/expected/check_btree.out | 17 +++++++++++
contrib/amcheck/sql/check_btree.sql | 16 +++++++++++
contrib/amcheck/verify_nbtree.c | 36 ++++++++++++++++++++----
3 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index 86b38d93f41..6ef7915b319 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -1,3 +1,4 @@
+-- directory paths are passed to us in environment variables
CREATE TABLE bttest_a(id int8);
CREATE TABLE bttest_b(id int8);
CREATE TABLE bttest_multi(id int8, data int8);
@@ -240,6 +241,21 @@ 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
+-- https://postgr.es/m/[email protected]
+--
+CREATE TABLE varlena_bug (v text);
+ALTER TABLE varlena_bug ALTER column v SET storage plain;
+INSERT INTO varlena_bug VALUES ('x');
+COPY varlena_bug from stdin;
+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 +266,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 aa461f7fb97..e17924b1549 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -1,3 +1,4 @@
+-- directory paths are passed to us in environment variables
CREATE TABLE bttest_a(id int8);
CREATE TABLE bttest_b(id int8);
CREATE TABLE bttest_multi(id int8, data int8);
@@ -148,6 +149,20 @@ 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
+-- https://postgr.es/m/[email protected]
+--
+
+CREATE TABLE varlena_bug (v text);
+ALTER TABLE varlena_bug ALTER column v SET storage plain;
+INSERT INTO varlena_bug VALUES ('x');
+COPY varlena_bug from stdin;
+x
+\.
+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 +173,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 91caa53dd8b..e7f01c2addb 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.43.0
[text/x-patch] v4-0002-amcheck-prevent-false-positives-from-extended-dat.patch (2.9K, 4-v4-0002-amcheck-prevent-false-positives-from-extended-dat.patch)
download | inline diff:
From f231643a2e990d767d78766a2bbd073d30bc6e4d Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email protected]>
Date: Tue, 23 Jan 2024 21:57:02 +0500
Subject: [PATCH v4 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 6ef7915b319..f8638582180 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -256,6 +256,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 e17924b1549..5da49ea94fd 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -163,6 +163,15 @@ x
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 e7f01c2addb..6654b5afe73 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.43.0
[text/x-patch] v4-0003-amcheck-avoid-failing-on-oversized-tuples.patch (6.0K, 5-v4-0003-amcheck-avoid-failing-on-oversized-tuples.patch)
download | inline diff:
From aa611847e8fee0994b35cda881058cc74a7a5ab0 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email protected]>
Date: Tue, 23 Jan 2024 23:03:28 +0500
Subject: [PATCH v4 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 | 53 +++++++++++++++++++++---
3 files changed, 72 insertions(+), 6 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index f8638582180..0ea2e3e1f38 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -268,6 +268,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 5da49ea94fd..ceeb7258754 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -171,6 +171,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 6654b5afe73..e30f162e0fb 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);
}
}
@@ -2883,9 +2902,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 +2965,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 +2979,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 +3059,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.43.0
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], [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