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: y sokolov <[email protected]>
Subject: Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum
Date: Tue, 9 Jan 2024 20:59:17 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
Hi,
Thank you, Andrey, for review and advice!
Here is rebased version (v2) of patch supposed to make CF bot happy.
Best regards,
Michael
On 1/8/24 17:34, Andrey M. Borodin wrote:
> Hi Alexander!
>
> I think both cases are very interesting and deserve two separate bug items.
>
>> On 14 Dec 2023, at 22:17, Alexander Lakhin<[email protected]> wrote:
>>
>>
>> By changing the storage mode for a column, you can also get another error:
>> 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;
>>
>> CREATE EXTENSION amcheck;
>> SELECT bt_index_check('t_idx', true);
>>
>> ERROR: index row requires 10016 bytes, maximum size is 8191
>>
> I think In this case we should warn user that index contains tuples with datums that are not insertable anymore. And abort heapallindexed.
>
>
>> On 8 Jan 2024, at 00:00, Alexander Lakhin<[email protected]> wrote:
>>
>> What is your opinion regarding similar failures, which are not addressed
>> by the patch? Besides the case shown above, there is another one:
>> CREATE TABLE tbl(i int4, t text);
>> ALTER TABLE tbl ALTER COLUMN t SET STORAGE plain;
>> CREATE INDEX tbl_idx ON tbl (t, i) WITH (fillfactor = 10);
>> INSERT INTO tbl SELECT g, repeat('Test', 250) FROM generate_series(1, 130) g;
>> ALTER TABLE tbl ALTER COLUMN t SET STORAGE extended;
>>
>> CREATE EXTENSION amcheck;
>> SELECT bt_index_check('tbl_idx', true);
>>
>> ERROR: heap tuple (0,1) from table "tbl" lacks matching index tuple within index "tbl_idx"
>> HINT: Retrying verification using the function bt_index_parent_check() might provide a more specific error.
> IMO In this case we should handle VARATT_IS_EXTENDED in bt_normalize_tuple().
>
> BTW CI fails of the original patch ITT are related to the fact that COPY in\out file is created in PG_ABS_SRCDIR instead of PG_ABS_BUILDDIR. In an off-list conversation I recommended Michael to mimic tests regress/largeobject.sql. Though, there might be better ideas.
>
> Thanks!
>
>
> Best regards, Andrey Borodin.
>
--
Michael Zhilin
Postgres Professional
+7(925)3366270
https://www.postgrespro.ru
Attachments:
[text/x-patch] v2-0001-contrib-amcheck-must-support-different-header-siz.patch (6.2K, 3-v2-0001-contrib-amcheck-must-support-different-header-siz.patch)
download | inline diff:
From ae3850e64e75c1137acce1a20f276fa9a4cd0dbe Mon Sep 17 00:00:00 2001
From: Michael Zhilin <[email protected]>
Date: Thu, 14 Dec 2023 16:08:15 +0300
Subject: [PATCH v2] 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 86b38d93f41..e37901c4028 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 aa461f7fb97..e67eb6352f8 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 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
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