public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Korotkov <[email protected]>
To: Alexander Lakhin <[email protected]>
Cc: Andrey M. Borodin <[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: Sat, 23 Mar 2024 13:37:04 +0200
Message-ID: <CAPpHfdspqdzVdEU2qHMibKn8OwVuLoZZYvKm-m5Ffqy2aMXgGQ@mail.gmail.com> (raw)
In-Reply-To: <CAPpHfdu412Z+jh5Oyc1yzWFM0+52h0kSGQKV4=xQ0aT0UeY2BA@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAPpHfdsAs-B7O2_=jGbF+BQzuW3kGSboY9CcxzEOWUgxxZCr5Q@mail.gmail.com>
<[email protected]>
<CAPpHfdtZxhgjZTpaLd--dCxzUOL6tN+aAZPBeTsGKHLkMQAZpw@mail.gmail.com>
<CAPpHfdu412Z+jh5Oyc1yzWFM0+52h0kSGQKV4=xQ0aT0UeY2BA@mail.gmail.com>
On Sat, Mar 23, 2024 at 2:39 AM Alexander Korotkov <[email protected]> wrote:
> On Wed, Mar 20, 2024 at 7:00 PM Alexander Korotkov <[email protected]> wrote:
> > On Wed, Mar 20, 2024 at 6:00 PM Alexander Lakhin <[email protected]> wrote:
> > > 20.03.2024 13:24, Alexander Korotkov wrote:
> > > > BTW, 0001 and 0002 look good to me. I'm going to push them if no objections.
> > >
> > > Maybe these patches should be polished before committing:
> >
> > Yes, Alexander. Sorry, I forgot to mention I'm going to polish
> > comments and commit messages before pushing anyway. I'll post it for
> > your review later today.
>
> There are revised versions of patches. Alexander, please, check them
> before I push.
Fixed typo s/much/match/ in the commit message spotted by Andrey Borodin.
------
Regards,
Alexander Korotkov
Attachments:
[application/octet-stream] v6-0001-amcheck-Support-for-different-header-sizes-of-sho.patch (5.6K, 2-v6-0001-amcheck-Support-for-different-header-sizes-of-sho.patch)
download | inline diff:
From a5418f7c95d5153f298ef2c2fd9e37c830586b51 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Sat, 23 Mar 2024 02:36:31 +0200
Subject: [PATCH v6 1/2] amcheck: Support for different header sizes of short
varlena datum
In the heap, tuples may contain short varlena datum with both 1B header and 4B
eaders. But the corresponding index tuple should always have such varlena's
with 1B headers. So, for fingerprinting, we need to convert.
Backpatch to all supported versions.
Discussion: https://postgr.es/m/flat/7bdbe559-d61a-4ae4-a6e1-48abdf3024cc%40postgrespro.ru
Author: Michael Zhilin
Reviewed-by: Alexander Lakhin, Andrey Borodin, Jian He, Alexander Korotkov
Backpatch-through: 12
---
contrib/amcheck/expected/check_btree.out | 13 +++++++++
contrib/amcheck/sql/check_btree.sql | 11 ++++++++
contrib/amcheck/verify_nbtree.c | 36 ++++++++++++++++++++----
3 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index 86b38d93f41..d87e7178866 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -240,6 +240,18 @@ SELECT bt_index_check('bttest_unique_nulls_b_c_idx', heapallindexed => true, che
(1 row)
+-- Check support of both 1B and 4B header sizes 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');
+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 +262,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..b37fff05078 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -148,6 +148,16 @@ 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);
+-- Check support of both 1B and 4B header sizes 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');
+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 +168,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 1ef4cff88e8..e0dffd9bcca 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -2943,7 +2943,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;
@@ -2962,7 +2962,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]);
@@ -2985,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;
@@ -2999,6 +3020,11 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
* (normalized input datums). This is rather naive, but shouldn't be
* necessary too often.
*
+ * In the heap, tuples may contain short varlena datums with both 1B
+ * header and 4B headers. But the corresponding index tuple should always
+ * have such varlena's with 1B headers. So, if there is a short varlena
+ * with 4B header, we need to convert it for for fingerprinting.
+ *
* Note that we rely on deterministic index_form_tuple() TOAST compression
* of normalized input.
*/
@@ -3007,7 +3033,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.39.3 (Apple Git-145)
[application/octet-stream] v6-0002-amcheck-Normalize-index-tuples-containing-uncompr.patch (3.4K, 3-v6-0002-amcheck-Normalize-index-tuples-containing-uncompr.patch)
download | inline diff:
From b626608a276bef4b4a2fb5df608c42f2be5595eb Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Sat, 23 Mar 2024 02:29:24 +0200
Subject: [PATCH v6 2/2] amcheck: Normalize index tuples containing
uncompressed varlena
It might happen that the varlena value wasn't compressed by index_form_tuple()
due to current storage parameters. If compression is currently enabled, we
need to compress such values to match index tuple coming from the heap.
Backpatch to all supported versions.
Discussion: https://postgr.es/m/flat/7bdbe559-d61a-4ae4-a6e1-48abdf3024cc%40postgrespro.ru
Author: Andrey Borodin
Reviewed-by: Alexander Lakhin, Michael Zhilin, Jian He, Alexander Korotkov
Backpatch-through: 12
---
contrib/amcheck/expected/check_btree.out | 10 ++++++++++
contrib/amcheck/sql/check_btree.sql | 6 ++++++
contrib/amcheck/verify_nbtree.c | 13 +++++++++++++
3 files changed, 29 insertions(+)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index d87e7178866..cf8284fe12e 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -252,6 +252,16 @@ SELECT bt_index_check('varlena_bug_idx', true);
(1 row)
+-- Also check that we compress varlena values, which were previously stored
+-- uncompressed in index.
+INSERT INTO varlena_bug VALUES (repeat('Test', 250));
+ALTER TABLE varlena_bug ALTER COLUMN v SET STORAGE extended;
+SELECT bt_index_check('varlena_bug_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 b37fff05078..68bd71b064f 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -158,6 +158,12 @@ x
CREATE INDEX varlena_bug_idx on varlena_bug(v);
SELECT bt_index_check('varlena_bug_idx', true);
+-- Also check that we compress varlena values, which were previously stored
+-- uncompressed in index.
+INSERT INTO varlena_bug VALUES (repeat('Test', 250));
+ALTER TABLE varlena_bug ALTER COLUMN v SET STORAGE extended;
+SELECT bt_index_check('varlena_bug_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 e0dffd9bcca..f71f1854e0a 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,18 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
ItemPointerGetBlockNumber(&(itup->t_tid)),
ItemPointerGetOffsetNumber(&(itup->t_tid)),
RelationGetRelationName(state->rel))));
+ else if (!VARATT_IS_COMPRESSED(DatumGetPointer(normalized[i])) &&
+ VARSIZE(DatumGetPointer(normalized[i])) > TOAST_INDEX_TARGET &&
+ (att->attstorage == TYPSTORAGE_EXTENDED ||
+ att->attstorage == TYPSTORAGE_MAIN))
+ {
+ /*
+ * This value will be compressed by index_form_tuple() with the
+ * current storage settings. We may be here because this tuple
+ * was formed with different storage settings. So, force forming.
+ */
+ formnewtup = true;
+ }
else if (VARATT_IS_COMPRESSED(DatumGetPointer(normalized[i])))
{
formnewtup = true;
--
2.39.3 (Apple Git-145)
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: <CAPpHfdspqdzVdEU2qHMibKn8OwVuLoZZYvKm-m5Ffqy2aMXgGQ@mail.gmail.com>
* 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