Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. amcheck: Use correct varlena size accessor in bt_normalize_tuple()

  1. [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Michael Zhilin <m.zhilin@postgrespro.ru> — 2023-12-14T16:18:11Z

    Hi,

Following example produces error raised from bt_index_check.

drop table if exists t;
create table t (v text);
alter table t alter column v set storage plain;
insert into t values ('x');
copy t to '/tmp/1.lst';
copy t from '/tmp/1.lst';
create index t_idx on t(v);
create extension if not exists amcheck;
select bt_index_check('t_idx', true);

postgres=# select bt_index_check('t_idx', true);
ERROR:  heap tuple (0,2) from table "t" lacks matching index tuple 
within index "t_idx"
HINT:  Retrying verification using the function bt_index_parent_check() 
might provide a more specific error.

As result table contains 2 logically identical tuples:
  - one contains varlena 'x' with 1B (1-byte) header (added by INSERT 
statement)
  - one contains varlena 'x' with 4B (4-bytes) header (added by COPY 
statement)
CREATE INDEX statement builds index with posting list referencing both 
heap tuples.
The function bt_index_check calculates fingerprints of 1B and 4B header 
datums,
they are different and function returns error.

The attached patch allows to avoid such kind of false positives by 
converting short
4B datums to 1B before fingerprinting. Also it contains test for 
provided case.

Thank you,
  Michael

-- 
Michael Zhilin
Postgres Professional

  2. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Lakhin <exclusion@gmail.com> — 2023-12-14T17:17:17Z

    Hi Michael,

14.12.2023 19:18, Michael Zhilin wrote:
> Hi,
>
> Following example produces error raised from bt_index_check.
>
> drop table if exists t;
> create table t (v text);
> alter table t alter column v set storage plain;
> insert into t values ('x');
> copy t to '/tmp/1.lst';
> copy t from '/tmp/1.lst';
> create index t_idx on t(v);
> create extension if not exists amcheck;
> select bt_index_check('t_idx', true);
>
> postgres=# select bt_index_check('t_idx', true);
> ERROR:  heap tuple (0,2) from table "t" lacks matching index tuple within index "t_idx"
> HINT:  Retrying verification using the function bt_index_parent_check() might provide a more specific error.
>
> As result table contains 2 logically identical tuples:
>  - one contains varlena 'x' with 1B (1-byte) header (added by INSERT statement)
>  - one contains varlena 'x' with 4B (4-bytes) header (added by COPY statement)
> CREATE INDEX statement builds index with posting list referencing both heap tuples.
> The function bt_index_check calculates fingerprints of 1B and 4B header datums,
> they are different and function returns error.
>
> The attached patch allows to avoid such kind of false positives by converting short
> 4B datums to 1B before fingerprinting. Also it contains test for provided case.

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

Best regards,
Alexander

  3. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-01-07T18:04:35Z

    
> On 14 Dec 2023, at 21:18, Michael Zhilin <m.zhilin@postgrespro.ru> wrote:

I've checked that:
* bug is reproduced by the test in the patch
* bug is fixed by the patch
* fix seems idiomatic, similar to nearby code

Patch needed a rebase, so please find attached rebased version. I did not change anything.

I see that using a temp file in PG_ABS_SRCDIR is common approach. But still I want to ask, maybe can we develop some clever way to reproduce the bug without external file?
Also, maybe nearby code would be slightly more readable, if normalized[i] was a local variable.
And one last question about the line:
char *data = palloc(len);
what if data is somehow corrupted here... are there enough sanity checks that we won't palloc(-1) or something like that?
Won't we memcpy() from some other memory when len is bogus?

Besides this paranoid questions, I think that this patch is ready for committer.

Thanks!


Best regards, Andrey Borodin.

  4. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-01-07T18:33:56Z

    
> On 7 Jan 2024, at 23:04, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> 
> I see that using a temp file in PG_ABS_SRCDIR is common approach. But still I want to ask, maybe can we develop some clever way to reproduce the bug without external file?

BTW this stuff is causing problems in CFbot [0,1]
COPY varlena_bug TO :'filename';
+ERROR:  could not open file "/tmp/cirrus-ci-build/contrib/amcheck/results/varlena_bug.dmp" for writing: No such file or directory
+HINT:  COPY TO instructs the PostgreSQL server process to write a file. You may want a client-side facility such as psql's \copy.


Best regards, Andrey Borodin.

[0] https://api.cirrus-ci.com/v1/artifact/task/4880592609738752/testrun/build/testrun/amcheck/regress/regression.diffs
[1] https://github.com/x4m/postgres_g/runs/20240081462

  5. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Lakhin <exclusion@gmail.com> — 2024-01-07T19:00:00Z

    Hello Andrey,

07.01.2024 21:04, Andrey M. Borodin wrote:
>> On 14 Dec 2023, at 21:18, Michael Zhilin <m.zhilin@postgrespro.ru> wrote:
> I've checked that:
> * bug is reproduced by the test in the patch
> * bug is fixed by the patch
> * fix seems idiomatic, similar to nearby code
>

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.

Best regards,
Alexander

  6. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-01-08T14:34:41Z

    Hi Alexander!

I think both cases are very interesting and deserve two separate bug items.

> On 14 Dec 2023, at 22:17, Alexander Lakhin <exclusion@gmail.com> 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 <exclusion@gmail.com> 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.

  7. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Michael Zhilin <m.zhilin@postgrespro.ru> — 2024-01-09T17:59:17Z

    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<exclusion@gmail.com>  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<exclusion@gmail.com>  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

  8. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-01-19T18:16:34Z

    
> On 9 Jan 2024, at 22:59, Michael Zhilin <m.zhilin@postgrespro.ru> wrote:
> 
> Here is rebased version (v2) of patch supposed to make CF bot happy. 

I've marked CF item as ready for committer.

> 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.

Alexander, do you plan to provide fixes for bugs you discovered?


Best regards, Andrey Borodin.

  9. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Lakhin <exclusion@gmail.com> — 2024-01-20T04:00:00Z

    Hi Andrey,

19.01.2024 21:16, Andrey Borodin wrote:
> I've marked CF item as ready for committer.
>> 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.
> 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.

Best regards,
Alexander

  10. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    jian he <jian.universality@gmail.com> — 2024-01-23T00:00:00Z

    On Wed, Jan 10, 2024 at 1:59 AM Michael Zhilin <m.zhilin@postgrespro.ru> wrote:
>
> Hi,
>
> Thank you, Andrey, for review and advice!
>
> Here is rebased version (v2) of patch supposed to make CF bot happy.

Hi

+--
+-- 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);

you can simply replace
+\set filename :abs_builddir '/results/varlena_bug.dmp'
+COPY varlena_bug TO :'filename';
+COPY varlena_bug FROM :'filename';

with

COPY varlena_bug from stdin;
x
\.


In the comments, adding the postgres link
(https://postgr.es/m/7bdbe559-d61a-4ae4-a6e1-48abdf3024cc@postgrespro.ru)
would be great.

  11. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-01-23T18:09:43Z

    
> On 20 Jan 2024, at 09:00, Alexander Lakhin <exclusion@gmail.com> 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.

  12. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Michael Zhilin <m.zhilin@postgrespro.ru> — 2024-01-23T19:24:11Z

    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<exclusion@gmail.com>  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

  13. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    jian he <jian.universality@gmail.com> — 2024-01-26T00:00:00Z

    On Wed, Jan 24, 2024 at 3:24 AM Michael Zhilin <m.zhilin@postgrespro.ru> wrote:
>
> 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.
>

this deserve some comments, given the whole C file, a large portion of
is comments
+ 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;
+ }

whitespace error.
git apply $PATCHES/v4-0003-amcheck-avoid-failing-on-oversized-tuples.patch
/home/jian/Downloads/patches/v4-0003-amcheck-avoid-failing-on-oversized-tuples.patch:147:
trailing whitespace.
 *
warning: 1 line adds whitespace errors.

Is this part unnecessary?
+-- directory paths are passed to us in environment variables

I'm not native English speaker, but I doubt this sentence conveys the
meaning properly.
+ if (!state->has_oversized_tuples)
+ elog(NOTICE, "Index contain tuples that cannot fit into index page,
if toasted with current toast policy");

+ * If the tuple is exampt from checking due to has_oversized_tuples
this function
+ * returns NULL.
maybe
+ * If the tuple is exempt from checking due to has_oversized_tuples,
this function
+ * returns NULL.

you changed
`bool toast_free[INDEX_MAX_KEYS];`
to
`bool need_free[INDEX_MAX_KEYS];`
maybe some comments address the changes, otherwise people would say
why change (maybe I am over thinking).

  14. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Lakhin <exclusion@gmail.com> — 2024-01-26T07:00:00Z

    Hi Andrey,

23.01.2024 21:09, Andrey M. Borodin wrote:
> 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.
>

I''m confused by a NOTICE added, as it printed now even for cases, which
worked before, for example:
CREATE TABLE t(f1 text);
CREATE INDEX idx ON t(f1);
INSERT INTO t VALUES(repeat('1234567890', 1000));
SELECT bt_index_check('idx', true);
NOTICE:  Index contain tuples that cannot fit into index page, if toasted with current toast policy
  bt_index_check
----------------

(1 row)

Best regards,
Alexander

  15. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-03-20T10:24:55Z

    Hi!


On Fri, Jan 26, 2024 at 9:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> 23.01.2024 21:09, Andrey M. Borodin wrote:
> > 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.
> >
>
> I''m confused by a NOTICE added, as it printed now even for cases, which
> worked before, for example:
> CREATE TABLE t(f1 text);
> CREATE INDEX idx ON t(f1);
> INSERT INTO t VALUES(repeat('1234567890', 1000));
> SELECT bt_index_check('idx', true);
> NOTICE:  Index contain tuples that cannot fit into index page, if toasted with current toast policy
>   bt_index_check
> ----------------
>
> (1 row)


Right, the patch number 0003 looks wrong to me.  It uses
heap_compute_data_size() to compute the size of the future index
tuple.  But heap_compute_data_size() doesn't apply any compression on
the attributes.  I suggest instead we just need to have a version of
index_form_tuple() that reports an oversized tuple in a return value
rather than throwing the error.

BTW, 0001 and 0002 look good to me.  I'm going to push them if no objections.

------
Regards,
Alexander Korotkov

  16. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Lakhin <exclusion@gmail.com> — 2024-03-20T16:00:00Z

    Hi Alexander,

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:
+-- directory paths are passed to us in environment variables
looks like an irrelevant change (perhaps it was relevant in v1/v2, but
that's not so now.)

I'm also not sure about:
+-- BUG: must support different header size of short varlena datum
+-- https://postgr.es/m/7bdbe559-d61a-4ae4-a6e1-48abdf3024cc@postgrespro.ru

AFAICS, for most similar bug fixes, the bug report referenced in a commit
message only (there is no such comment in 0002, either). I also suspect
that the comment:
* Also tuple had short varlena datums with 4B header. ...
might looks incorrect for native English speakers.

This patch also adds a couple of empty lines, which may be not needed.
@@ -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),
---
         }
+       /*
...
+           need_free[i] = true;
+       }
 > +
     }

Best regards,
Alexander

  17. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-03-20T17:00:32Z

    On Wed, Mar 20, 2024 at 6:00 PM Alexander Lakhin <exclusion@gmail.com> 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.

------
Regards,
Alexander Korotkov

  18. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-03-21T05:46:43Z

    
> On 20 Mar 2024, at 15:24, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> 
> Right, the patch number 0003 looks wrong to me. 

Indeed, step 0003 was added to the patchset after entry marked RfC. And you are right that it’s not ready yet, and, honestly, I do not know how to fix it properly.
Let’s proceed with other bugs and patches in this thread, and for bug of the step 0003 we will create separate thread later.

Thanks for working on this!


Best regards, Andrey Borodin.

  19. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-03-23T00:39:36Z

    On Wed, Mar 20, 2024 at 7:00 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Wed, Mar 20, 2024 at 6:00 PM Alexander Lakhin <exclusion@gmail.com> 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.

------
Regards,
Alexander Korotkov

  20. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-03-23T11:37:04Z

    On Sat, Mar 23, 2024 at 2:39 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Wed, Mar 20, 2024 at 7:00 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Wed, Mar 20, 2024 at 6:00 PM Alexander Lakhin <exclusion@gmail.com> 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

  21. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andres Freund <andres@anarazel.de> — 2026-05-01T17:11:13Z

    Hi,

On 2024-03-23 13:37:04 +0200, Alexander Korotkov wrote:
> @@ -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;

While hacking on something, I added an assertion to VARSIZE() that the
argument is actually a VARATT_4B (which it assumes). Worked everywhere, except
for this caller: amcheck/regress fails, because sometimes the varlena is
actually a short/1B varlena.

Note that VARSIZE_4B on a short datum will give you completely bogus
answers. E.g. in the case that failed the assertion, VARSIZE_1B() is 2, but
VARSIZE_4B(PTR) is 7681.

diff --git i/contrib/amcheck/verify_nbtree.c w/contrib/amcheck/verify_nbtree.c
index b74ab5f7a05..0b87109fde3 100644
--- i/contrib/amcheck/verify_nbtree.c
+++ w/contrib/amcheck/verify_nbtree.c
@@ -2889,6 +2889,7 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
                             ItemPointerGetOffsetNumber(&(itup->t_tid)),
                             RelationGetRelationName(state->rel))));
         else if (!VARATT_IS_COMPRESSED(DatumGetPointer(normalized[i])) &&
+                 !VARATT_IS_SHORT(DatumGetPointer(normalized[i])) &&
                  VARSIZE(DatumGetPointer(normalized[i])) > TOAST_INDEX_TARGET &&
                  (att->attstorage == TYPSTORAGE_EXTENDED ||
                   att->attstorage == TYPSTORAGE_MAIN))


Fixes the assert failure.


I guess I find it aesthetically a bit unpleasing to check the same stuff so
many times for one varlena :). Not that it should matter performance-wise
here...

Greetings,

Andres Freund

  22. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2026-05-01T18:06:49Z

    
> On 1 May 2026, at 22:11, Andres Freund <andres@anarazel.de> wrote:
> 
> While hacking on something, I added an assertion to VARSIZE() that the
> argument is actually a VARATT_4B (which it assumes). Worked everywhere, except
> for this caller: amcheck/regress fails, because sometimes the varlena is
> actually a short/1B varlena.
> 
> Note that VARSIZE_4B on a short datum will give you completely bogus
> answers. E.g. in the case that failed the assertion, VARSIZE_1B() is 2, but
> VARSIZE_4B(PTR) is 7681.

I remember the original code was taken from somewhere else because there
was already some instances like this:

/*
 * If value is above size target, and is of a compressible datatype,
 * try to compress it in-line.
 */
if (!VARATT_IS_EXTENDED(DatumGetPointer(untoasted_values[i])) &&
VARSIZE(DatumGetPointer(untoasted_values[i])) > TOAST_INDEX_TARGET &&
(att->attstorage == TYPSTORAGE_EXTENDED ||
att->attstorage == TYPSTORAGE_MAIN))
{

I don't have VARATT_IS_EXTENDED vs VARATT_IS_COMPRESSED vs VARATT_IS_SHORT
business in my warm cache right away, but I'll try to remember what it means soon.


Best regards, Andrey Borodin.

  23. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andres Freund <andres@anarazel.de> — 2026-05-01T19:41:05Z

    Hi,

On 2026-05-01 23:06:49 +0500, Andrey Borodin wrote:
> > On 1 May 2026, at 22:11, Andres Freund <andres@anarazel.de> wrote:
> > 
> > While hacking on something, I added an assertion to VARSIZE() that the
> > argument is actually a VARATT_4B (which it assumes). Worked everywhere, except
> > for this caller: amcheck/regress fails, because sometimes the varlena is
> > actually a short/1B varlena.
> > 
> > Note that VARSIZE_4B on a short datum will give you completely bogus
> > answers. E.g. in the case that failed the assertion, VARSIZE_1B() is 2, but
> > VARSIZE_4B(PTR) is 7681.
> 
> I remember the original code was taken from somewhere else because there
> was already some instances like this:

> /*
>  * If value is above size target, and is of a compressible datatype,
>  * try to compress it in-line.
>  */
> if (!VARATT_IS_EXTENDED(DatumGetPointer(untoasted_values[i])) &&
> VARSIZE(DatumGetPointer(untoasted_values[i])) > TOAST_INDEX_TARGET &&
> (att->attstorage == TYPSTORAGE_EXTENDED ||
> att->attstorage == TYPSTORAGE_MAIN))
> {
> 
> I don't have VARATT_IS_EXTENDED vs VARATT_IS_COMPRESSED vs VARATT_IS_SHORT
> business in my warm cache right away, but I'll try to remember what it means soon.

This is checking (as you noted) !VARATT_IS_EXTENDED, whereas the
bt_normalize_tuple() code is checking !VARATT_IS_COMPRESSED.

VARATT_IS_EXTENDED() will return true for short varlenas (because it's not a
standard 4 byte uncompressed varlena), whereas VARATT_IS_COMPRESSED() will
return false for a short varlena (since it's not compressed).

I didn't find other instanes of similar code that uses !VARATT_IS_COMPRESSED.

Greetings,

Andres Freund

  24. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2026-05-03T18:17:19Z

    
> On 2 May 2026, at 00:41, Andres Freund <andres@anarazel.de> wrote:
> 
> This is checking (as you noted) !VARATT_IS_EXTENDED, whereas the
> bt_normalize_tuple() code is checking !VARATT_IS_COMPRESSED.
> 
> VARATT_IS_EXTENDED() will return true for short varlenas (because it's not a
> standard 4 byte uncompressed varlena), whereas VARATT_IS_COMPRESSED() will
> return false for a short varlena (since it's not compressed).
> 
> I didn't find other instanes of similar code that uses !VARATT_IS_COMPRESSED.

As far as I understand 

!VARATT_IS_COMPRESSED(DatumGetPointer(normalized[i])) && !VARATT_IS_SHORT(DatumGetPointer(normalized[i]))

is exactly

!VARATT_IS_EXTENDED(DatumGetPointer(untoasted_values[i]))

Which is what was proposed in v2 patch. But later was changed to !VARATT_IS_COMPRESSED().
As I understood it was done to further strengthen normalization.

So the intent might be that short varatts need normalization in some cases. But we have no tests that show such a case.
I tried to build a problematic storage alternation like [0], but everything works nicely.

So I propose something in a line with attached patch.


Best regards, Andrey Borodin.

[0] https://github.com/postgres/postgres/blob/master/contrib/amcheck/sql/check_btree.sql#L170-L172

  25. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Korotkov <aekorotkov@gmail.com> — 2026-05-03T19:44:32Z

    On Sun, May 3, 2026 at 9:17 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > On 2 May 2026, at 00:41, Andres Freund <andres@anarazel.de> wrote:
> >
> > This is checking (as you noted) !VARATT_IS_EXTENDED, whereas the
> > bt_normalize_tuple() code is checking !VARATT_IS_COMPRESSED.
> >
> > VARATT_IS_EXTENDED() will return true for short varlenas (because it's not a
> > standard 4 byte uncompressed varlena), whereas VARATT_IS_COMPRESSED() will
> > return false for a short varlena (since it's not compressed).
> >
> > I didn't find other instanes of similar code that uses !VARATT_IS_COMPRESSED.
>
> As far as I understand
>
> !VARATT_IS_COMPRESSED(DatumGetPointer(normalized[i])) && !VARATT_IS_SHORT(DatumGetPointer(normalized[i]))
>
> is exactly
>
> !VARATT_IS_EXTENDED(DatumGetPointer(untoasted_values[i]))
>
> Which is what was proposed in v2 patch. But later was changed to !VARATT_IS_COMPRESSED().
> As I understood it was done to further strengthen normalization.
>
> So the intent might be that short varatts need normalization in some cases. But we have no tests that show such a case.
> I tried to build a problematic storage alternation like [0], but everything works nicely.
>
> So I propose something in a line with attached patch.
>
>
> Best regards, Andrey Borodin.
>
> [0] https://github.com/postgres/postgres/blob/master/contrib/amcheck/sql/check_btree.sql#L170-L172

AFAICS, this is correct.  However, I propose an alternative approach:
use VARSIZE_ANY().  This might be a bit slower, but looks more
intuitive to me.

------
Regards,
Alexander Korotkov
Supabase

  26. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2026-05-04T05:20:04Z

    
> On 4 May 2026, at 00:44, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> 
> AFAICS, this is correct.  However, I propose an alternative approach:
> use VARSIZE_ANY().  This might be a bit slower, but looks more
> intuitive to me.
Works for me.
However, I'd like to note that (VARSIZE_1B() < TOAST_INDEX_TARGET) is
constantly true for 8Kb+ pages.


Best regards, Andrey Borodin.

  27. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Michael Paquier <michael@paquier.xyz> — 2026-05-08T23:07:06Z

    On Mon, May 04, 2026 at 10:20:04AM +0500, Andrey Borodin wrote:
> However, I'd like to note that (VARSIZE_1B() < TOAST_INDEX_TARGET) is
> constantly true for 8Kb+ pages.

How much slower?  I cannot imagine that it matters much in this code
path, but you are getting me worried.
--
Michael

  28. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2026-05-12T09:17:27Z

    
> On 9 May 2026, at 04:07, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Mon, May 04, 2026 at 10:20:04AM +0500, Andrey Borodin wrote:
>> However, I'd like to note that (VARSIZE_1B() < TOAST_INDEX_TARGET) is
>> constantly true for 8Kb+ pages.
> 
> How much slower?  I cannot imagine that it matters much in this code
> path, but you are getting me worried.


I think there will be no performance difference.

Change proposed by Alexander only prevents use of VARSIZE() against datum
that is VARSIZE_1B. AFAICS no actual behavior would change.

On some occasions we would have to normilize less tuples.


Best regards, Andrey Borodin.

  29. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Alexander Korotkov <aekorotkov@gmail.com> — 2026-05-12T10:22:59Z

    On Tue, May 12, 2026 at 12:17 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > On 9 May 2026, at 04:07, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Mon, May 04, 2026 at 10:20:04AM +0500, Andrey Borodin wrote:
> >> However, I'd like to note that (VARSIZE_1B() < TOAST_INDEX_TARGET) is
> >> constantly true for 8Kb+ pages.
> >
> > How much slower?  I cannot imagine that it matters much in this code
> > path, but you are getting me worried.
>
>
> I think there will be no performance difference.
>
> Change proposed by Alexander only prevents use of VARSIZE() against datum
> that is VARSIZE_1B. AFAICS no actual behavior would change.
>
> On some occasions we would have to normilize less tuples.

Any objections if I push and backpatch this?

------
Regards,
Alexander Korotkov
Supabase

  30. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Andrey Borodin <x4mmm@yandex-team.ru> — 2026-06-11T07:57:32Z

    
> On 12 May 2026, at 13:22, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> 
> On Tue, May 12, 2026 at 12:17 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>>> On 9 May 2026, at 04:07, Michael Paquier <michael@paquier.xyz> wrote:
>>> 
>>> On Mon, May 04, 2026 at 10:20:04AM +0500, Andrey Borodin wrote:
>>>> However, I'd like to note that (VARSIZE_1B() < TOAST_INDEX_TARGET) is
>>>> constantly true for 8Kb+ pages.
>>> 
>>> How much slower?  I cannot imagine that it matters much in this code
>>> path, but you are getting me worried.
>> 
>> 
>> I think there will be no performance difference.
>> 
>> Change proposed by Alexander only prevents use of VARSIZE() against datum
>> that is VARSIZE_1B. AFAICS no actual behavior would change.
>> 
>> On some occasions we would have to normilize less tuples.
> 
> Any objections if I push and backpatch this?

Kind reminder, let's resolve this thread. Thank you!


Best regards, Andrey Borodin.

  31. Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

    Michael Paquier <michael@paquier.xyz> — 2026-06-12T01:32:10Z

    On Thu, Jun 11, 2026 at 10:57:32AM +0300, Andrey Borodin wrote:
> Kind reminder, let's resolve this thread. Thank you!

Thanks for the reminder.  FWIW, I had this thread on my radar for some
time, but I am refraining as Alexander seems to be already on it.
--
Michael