Thanks.
I went through the patches, polished the commit messages and did some
minor tweaks in patch 0002 (to make the variable names a bit more
consistent, and reduce the scope a little bit). I left it as a separate
patch to make the changes clearer, but it should be merged into 0002.
Please read through the commit messages, and let me know if I got some
of the details wrong (or not clear enough). Otherwise I plan to start
pushing this soon (~tomorrow).
regards
--
Tomas Vondra
From cb24bb068582a39df9e9e59c2a9347889e896cf2 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@vondra.me>
Date: Mon, 9 Jun 2025 01:42:52 +0200
Subject: [PATCH v8 1/6] amcheck: Add gin_index_check on a multicolumn index
Adds a regression test with gin_index_check() on a multicolumn index,
to verify it's handled correctly and improve test coverage for code
introduced by 14ffaece0fb5.
Author: Arseniy Mukhin <arseniy.mukhin....@gmail.com>
Reviewed-by: Andrey M. Borodin <x4...@yandex-team.ru>
Discussion: https://postgr.es/m/CAE7r3MJ611B9TE=yqbbncewp7-k64vws+sjk7xf6fjux77u...@mail.gmail.com
---
contrib/amcheck/expected/check_gin.out | 12 ++++++++++++
contrib/amcheck/sql/check_gin.sql | 10 ++++++++++
2 files changed, 22 insertions(+)
diff --git a/contrib/amcheck/expected/check_gin.out b/contrib/amcheck/expected/check_gin.out
index b4f0b110747..8dd01ced8d1 100644
--- a/contrib/amcheck/expected/check_gin.out
+++ b/contrib/amcheck/expected/check_gin.out
@@ -76,3 +76,15 @@ SELECT gin_index_check('gin_check_jsonb_idx');
-- cleanup
DROP TABLE gin_check_jsonb;
+-- Test GIN multicolumn index
+CREATE TABLE "gin_check_multicolumn"(a text[], b text[]);
+INSERT INTO gin_check_multicolumn (a,b) values ('{a,c,e}','{b,d,f}');
+CREATE INDEX "gin_check_multicolumn_idx" on gin_check_multicolumn USING GIN(a,b);
+SELECT gin_index_check('gin_check_multicolumn_idx');
+ gin_index_check
+-----------------
+
+(1 row)
+
+-- cleanup
+DROP TABLE gin_check_multicolumn;
diff --git a/contrib/amcheck/sql/check_gin.sql b/contrib/amcheck/sql/check_gin.sql
index 66f42c34311..11caed3d6a8 100644
--- a/contrib/amcheck/sql/check_gin.sql
+++ b/contrib/amcheck/sql/check_gin.sql
@@ -50,3 +50,13 @@ SELECT gin_index_check('gin_check_jsonb_idx');
-- cleanup
DROP TABLE gin_check_jsonb;
+
+-- Test GIN multicolumn index
+CREATE TABLE "gin_check_multicolumn"(a text[], b text[]);
+INSERT INTO gin_check_multicolumn (a,b) values ('{a,c,e}','{b,d,f}');
+CREATE INDEX "gin_check_multicolumn_idx" on gin_check_multicolumn USING GIN(a,b);
+
+SELECT gin_index_check('gin_check_multicolumn_idx');
+
+-- cleanup
+DROP TABLE gin_check_multicolumn;
--
2.49.0
From 04b0c0c718dd109b9b4598d316b27daab281688d Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@vondra.me>
Date: Mon, 9 Jun 2025 01:44:59 +0200
Subject: [PATCH v8 2/6] amcheck: Fix checks of entry order for GIN indexes
This tightens a couple checks in checking GIN indexes, which might have
resulted in incorrect results (false positives/negatives).
* The code skipped ordering checks if the entries were for different
attributes (for multi-column GIN indexes), possibly missing some cases
of data corruption. But the attribute number is part of the ordering,
so we can check that.
* The root page was skipped when checking entry order, but that is
unnecessary. The root page is subject to the same ordering rules, we
can process it just like any other page.
* The high key on the right-most page was not checked, but that is
needed only for inner pages (we don't store the high key for those).
For leaf pages we can check the high key just fine.
* Correct the detection of split pages. If the page gets split, the
cached parent key is creater than the current child key (not less, as
the core incorrectly expected).
Issues reported by Arseniy Mikhin, along with a proposed patch. Review
by Andrey M. Borodin, cleanup and improvements by me.
Author: Arseniy Mukhin <arseniy.mukhin....@gmail.com>
Reviewed-by: Andrey M. Borodin <x4...@yandex-team.ru>
Discussion: https://postgr.es/m/CAE7r3MJ611B9TE=yqbbncewp7-k64vws+sjk7xf6fjux77u...@mail.gmail.com
---
contrib/amcheck/meson.build | 1 +
contrib/amcheck/t/006_verify_gin.pl | 199 ++++++++++++++++++++++++++++
contrib/amcheck/verify_gin.c | 37 +++---
3 files changed, 219 insertions(+), 18 deletions(-)
create mode 100644 contrib/amcheck/t/006_verify_gin.pl
diff --git a/contrib/amcheck/meson.build b/contrib/amcheck/meson.build
index b33e8c9b062..1f0c347ed54 100644
--- a/contrib/amcheck/meson.build
+++ b/contrib/amcheck/meson.build
@@ -49,6 +49,7 @@ tests += {
't/003_cic_2pc.pl',
't/004_verify_nbtree_unique.pl',
't/005_pitr.pl',
+ 't/006_verify_gin.pl',
],
},
}
diff --git a/contrib/amcheck/t/006_verify_gin.pl b/contrib/amcheck/t/006_verify_gin.pl
new file mode 100644
index 00000000000..7fdde170e06
--- /dev/null
+++ b/contrib/amcheck/t/006_verify_gin.pl
@@ -0,0 +1,199 @@
+
+# Copyright (c) 2021-2025, PostgreSQL Global Development Group
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+
+use Test::More;
+
+my $node;
+my $blksize;
+
+# to get the split fast, we want tuples to be as large as possible, but the same time we don't want them to be toasted.
+my $filler_size = 1900;
+
+#
+# Test set-up
+#
+$node = PostgreSQL::Test::Cluster->new('test');
+$node->init(no_data_checksums => 1);
+$node->append_conf('postgresql.conf', 'autovacuum=off');
+$node->start;
+$blksize = int($node->safe_psql('postgres', 'SHOW block_size;'));
+$node->safe_psql('postgres', q(CREATE EXTENSION amcheck));
+$node->safe_psql(
+ 'postgres', q(
+ CREATE OR REPLACE FUNCTION random_string( INT ) RETURNS text AS $$
+ SELECT string_agg(substring('0123456789abcdefghijklmnopqrstuvwxyz', ceil(random() * 36)::integer, 1), '') from generate_series(1, $1);
+ $$ LANGUAGE SQL;));
+
+# Tests
+invalid_entry_order_leaf_page_test();
+invalid_entry_order_inner_page_test();
+invalid_entry_columns_order_test();
+
+sub invalid_entry_order_leaf_page_test
+{
+ my $relname = "test";
+ my $indexname = "test_gin_idx";
+
+ $node->safe_psql(
+ 'postgres', qq(
+ DROP TABLE IF EXISTS $relname;
+ CREATE TABLE $relname (a text[]);
+ INSERT INTO $relname (a) VALUES ('{aaaaa,bbbbb}');
+ CREATE INDEX $indexname ON $relname USING gin (a);
+ ));
+ my $relpath = relation_filepath($indexname);
+
+ $node->stop;
+
+ my $blkno = 1; # root
+
+ # produce wrong order by replacing aaaaa with ccccc
+ string_replace_block(
+ $relpath,
+ 'aaaaa',
+ 'ccccc',
+ $blkno
+ );
+
+ $node->start;
+
+ my ($result, $stdout, $stderr) = $node->psql('postgres', qq(SELECT gin_index_check('$indexname')));
+ my $expected = "index \"$indexname\" has wrong tuple order on entry tree page, block 1, offset 2, rightlink 4294967295";
+ like($stderr, qr/$expected/);
+}
+
+sub invalid_entry_order_inner_page_test
+{
+ my $relname = "test";
+ my $indexname = "test_gin_idx";
+
+ # to break the order in the inner page we need at least 3 items (rightmost key in the inner level is not checked for the order)
+ # so fill table until we have 2 splits
+ $node->safe_psql(
+ 'postgres', qq(
+ DROP TABLE IF EXISTS $relname;
+ CREATE TABLE $relname (a text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'pppppppppp' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'qqqqqqqqqq' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'rrrrrrrrrr' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'ssssssssss' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'tttttttttt' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'uuuuuuuuuu' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'vvvvvvvvvv' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'wwwwwwwwww' || random_string($filler_size) ||'}')::text[]);
+ CREATE INDEX $indexname ON $relname USING gin (a);
+ ));
+ my $relpath = relation_filepath($indexname);
+
+ $node->stop;
+
+ my $blkno = 1; # root
+
+ # we have rrrrrrrrr... and tttttttttt... as keys in the root, so produce wrong order by replacing rrrrrrrrrr....
+ string_replace_block(
+ $relpath,
+ 'rrrrrrrrrr',
+ 'zzzzzzzzzz',
+ $blkno
+ );
+
+ $node->start;
+
+ my ($result, $stdout, $stderr) = $node->psql('postgres', qq(SELECT gin_index_check('$indexname')));
+ my $expected = "index \"$indexname\" has wrong tuple order on entry tree page, block 1, offset 2, rightlink 4294967295";
+ like($stderr, qr/$expected/);
+}
+
+sub invalid_entry_columns_order_test
+{
+ my $relname = "test";
+ my $indexname = "test_gin_idx";
+
+ $node->safe_psql(
+ 'postgres', qq(
+ DROP TABLE IF EXISTS $relname;
+ CREATE TABLE $relname (a text[],b text[]);
+ INSERT INTO $relname (a,b) VALUES ('{aaa}','{bbb}');
+ CREATE INDEX $indexname ON $relname USING gin (a,b);
+ ));
+ my $relpath = relation_filepath($indexname);
+
+ $node->stop;
+
+ my $blkno = 1; # root
+
+ # mess column numbers
+ # root items order before: (1,aaa), (2,bbb)
+ # root items order after: (2,aaa), (1,bbb)
+ my $attrno_1 = pack('s', 1);
+ my $attrno_2 = pack('s', 2);
+
+ my $find = qr/($attrno_1)(.)(aaa)/s;
+ my $replace = $attrno_2 . '$2$3';
+ string_replace_block(
+ $relpath,
+ $find,
+ $replace,
+ $blkno
+ );
+
+ $find = qr/($attrno_2)(.)(bbb)/s;
+ $replace = $attrno_1 . '$2$3';
+ string_replace_block(
+ $relpath,
+ $find,
+ $replace,
+ $blkno
+ );
+
+ $node->start;
+
+ my ($result, $stdout, $stderr) = $node->psql('postgres', qq(SELECT gin_index_check('$indexname')));
+ my $expected = "index \"$indexname\" has wrong tuple order on entry tree page, block 1, offset 2, rightlink 4294967295";
+ like($stderr, qr/$expected/);
+}
+
+# Returns the filesystem path for the named relation.
+sub relation_filepath
+{
+ my ($relname) = @_;
+
+ my $pgdata = $node->data_dir;
+ my $rel = $node->safe_psql('postgres',
+ qq(SELECT pg_relation_filepath('$relname')));
+ die "path not found for relation $relname" unless defined $rel;
+ return "$pgdata/$rel";
+}
+
+# substitute pattern 'find' with 'replace' within the block with number 'blkno' in the file 'filename'
+sub string_replace_block
+{
+ my ($filename, $find, $replace, $blkno) = @_;
+
+ my $fh;
+ open($fh, '+<', $filename) or BAIL_OUT("open failed: $!");
+ binmode $fh;
+
+ my $offset = $blkno * $blksize;
+ my $buffer;
+
+ sysseek($fh, $offset, 0) or BAIL_OUT("seek failed: $!");
+ sysread($fh, $buffer, $blksize) or BAIL_OUT("read failed: $!");
+
+ $buffer =~ s/$find/'"' . $replace . '"'/gee;
+
+ sysseek($fh, $offset, 0) or BAIL_OUT("seek failed: $!");
+ syswrite($fh, $buffer) or BAIL_OUT("write failed: $!");
+
+ close($fh) or BAIL_OUT("close failed: $!");
+
+ return;
+}
+
+done_testing();
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index b5f363562e3..26b98571b56 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -463,17 +463,18 @@ gin_check_parent_keys_consistency(Relation rel,
Datum parent_key = gintuple_get_key(&state,
stack->parenttup,
&parent_key_category);
+ OffsetNumber parent_key_attnum = gintuple_get_attrnum(&state, stack->parenttup);
ItemId iid = PageGetItemIdCareful(rel, stack->blkno,
page, maxoff);
IndexTuple idxtuple = (IndexTuple) PageGetItem(page, iid);
- OffsetNumber attnum = gintuple_get_attrnum(&state, idxtuple);
+ OffsetNumber page_max_key_attnum = gintuple_get_attrnum(&state, idxtuple);
GinNullCategory page_max_key_category;
Datum page_max_key = gintuple_get_key(&state, idxtuple, &page_max_key_category);
if (rightlink != InvalidBlockNumber &&
- ginCompareEntries(&state, attnum, page_max_key,
- page_max_key_category, parent_key,
- parent_key_category) > 0)
+ ginCompareAttEntries(&state, page_max_key_attnum, page_max_key,
+ page_max_key_category, parent_key_attnum,
+ parent_key, parent_key_category) < 0)
{
/* split page detected, install right link to the stack */
GinScanItem *ptr;
@@ -528,20 +529,18 @@ gin_check_parent_keys_consistency(Relation rel,
current_key = gintuple_get_key(&state, idxtuple, ¤t_key_category);
/*
- * First block is metadata, skip order check. Also, never check
- * for high key on rightmost page, as this key is not really
- * stored explicitly.
+ * Never check for high key on rightmost inner page, as this key
+ * is not really stored explicitly.
*
* Also make sure to not compare entries for different attnums,
* which may be stored on the same page.
*/
- if (i != FirstOffsetNumber && attnum == prev_attnum && stack->blkno != GIN_ROOT_BLKNO &&
- !(i == maxoff && rightlink == InvalidBlockNumber))
+ if (i != FirstOffsetNumber && !(i == maxoff && rightlink == InvalidBlockNumber && !GinPageIsLeaf(page)))
{
prev_key = gintuple_get_key(&state, prev_tuple, &prev_key_category);
- if (ginCompareEntries(&state, attnum, prev_key,
- prev_key_category, current_key,
- current_key_category) >= 0)
+ if (ginCompareAttEntries(&state, prev_attnum, prev_key,
+ prev_key_category, attnum,
+ current_key, current_key_category) >= 0)
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
errmsg("index \"%s\" has wrong tuple order on entry tree page, block %u, offset %u, rightlink %u",
@@ -556,13 +555,14 @@ gin_check_parent_keys_consistency(Relation rel,
i == maxoff)
{
GinNullCategory parent_key_category;
+ OffsetNumber parent_key_attnum = gintuple_get_attrnum(&state, stack->parenttup);
Datum parent_key = gintuple_get_key(&state,
stack->parenttup,
&parent_key_category);
- if (ginCompareEntries(&state, attnum, current_key,
- current_key_category, parent_key,
- parent_key_category) > 0)
+ if (ginCompareAttEntries(&state, attnum, current_key,
+ current_key_category, parent_key_attnum,
+ parent_key, parent_key_category) > 0)
{
/*
* There was a discrepancy between parent and child
@@ -581,6 +581,7 @@ gin_check_parent_keys_consistency(Relation rel,
stack->blkno, stack->parentblk);
else
{
+ parent_key_attnum = gintuple_get_attrnum(&state, stack->parenttup);
parent_key = gintuple_get_key(&state,
stack->parenttup,
&parent_key_category);
@@ -589,9 +590,9 @@ gin_check_parent_keys_consistency(Relation rel,
* Check if it is properly adjusted. If succeed,
* proceed to the next key.
*/
- if (ginCompareEntries(&state, attnum, current_key,
- current_key_category, parent_key,
- parent_key_category) > 0)
+ if (ginCompareAttEntries(&state, attnum, current_key,
+ current_key_category, parent_key_attnum,
+ parent_key, parent_key_category) > 0)
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
errmsg("index \"%s\" has inconsistent records on page %u offset %u",
--
2.49.0
From d79cdc4ef65a23d8d42eb6f150373f6a76490c54 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@vondra.me>
Date: Mon, 16 Jun 2025 17:09:18 +0200
Subject: [PATCH v8 3/6] tweaks
---
contrib/amcheck/verify_gin.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index 26b98571b56..25d47eefcbc 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -514,9 +514,7 @@ gin_check_parent_keys_consistency(Relation rel,
{
ItemId iid = PageGetItemIdCareful(rel, stack->blkno, page, i);
IndexTuple idxtuple = (IndexTuple) PageGetItem(page, iid);
- OffsetNumber attnum = gintuple_get_attrnum(&state, idxtuple);
- GinNullCategory prev_key_category;
- Datum prev_key;
+ OffsetNumber current_attnum = gintuple_get_attrnum(&state, idxtuple);
GinNullCategory current_key_category;
Datum current_key;
@@ -529,17 +527,23 @@ gin_check_parent_keys_consistency(Relation rel,
current_key = gintuple_get_key(&state, idxtuple, ¤t_key_category);
/*
- * Never check for high key on rightmost inner page, as this key
- * is not really stored explicitly.
+ * Compare the entry to the preceding one.
*
- * Also make sure to not compare entries for different attnums,
- * which may be stored on the same page.
+ * Don't check for high key on the rightmost inner page, as this
+ * key is not really stored explicitly.
+ *
+ * The entries may be for different attributes, so make sure to
+ * use ginCompareAttEntries for comparison.
*/
- if (i != FirstOffsetNumber && !(i == maxoff && rightlink == InvalidBlockNumber && !GinPageIsLeaf(page)))
+ if ((i != FirstOffsetNumber) &&
+ !(i == maxoff && rightlink == InvalidBlockNumber && !GinPageIsLeaf(page)))
{
+ Datum prev_key;
+ GinNullCategory prev_key_category;
+
prev_key = gintuple_get_key(&state, prev_tuple, &prev_key_category);
if (ginCompareAttEntries(&state, prev_attnum, prev_key,
- prev_key_category, attnum,
+ prev_key_category, current_attnum,
current_key, current_key_category) >= 0)
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
@@ -560,7 +564,7 @@ gin_check_parent_keys_consistency(Relation rel,
stack->parenttup,
&parent_key_category);
- if (ginCompareAttEntries(&state, attnum, current_key,
+ if (ginCompareAttEntries(&state, current_attnum, current_key,
current_key_category, parent_key_attnum,
parent_key, parent_key_category) > 0)
{
@@ -590,7 +594,7 @@ gin_check_parent_keys_consistency(Relation rel,
* Check if it is properly adjusted. If succeed,
* proceed to the next key.
*/
- if (ginCompareAttEntries(&state, attnum, current_key,
+ if (ginCompareAttEntries(&state, current_attnum, current_key,
current_key_category, parent_key_attnum,
parent_key, parent_key_category) > 0)
ereport(ERROR,
@@ -645,7 +649,7 @@ gin_check_parent_keys_consistency(Relation rel,
}
prev_tuple = CopyIndexTuple(idxtuple);
- prev_attnum = attnum;
+ prev_attnum = current_attnum;
}
LockBuffer(buffer, GIN_UNLOCK);
--
2.49.0
From 7326918e2ef50a52233c119712c7bc7abec993ba Mon Sep 17 00:00:00 2001
From: Arseniy Mukhin <arseniy.mukhin....@gmail.com>
Date: Mon, 9 Jun 2025 20:39:13 +0300
Subject: [PATCH v8 4/6] amcheck: Fix parent key check in gin_index_check()
The checks introduced by commit 14ffaece0fb5 did not get the parent key
checks quite right, missing some data corruption cases. In particular:
* The "rightlink" check was not working as intended, because rightlink
is a BlockNumber, and InvalidBlockNumber is 0xFFFFFFFF, so
!GinPageGetOpaque(page)->rightlink
almost always evaluates to false (except for rightlink=0). So in most
cases parenttup was left NULL, preventing any checks against parent.
* Use GinGetDownlink() to retrieve child blkno to avoid triggering
Assert, same as the core GIN code.
Issues reported by Arseniy Mikhin, along with a proposed patch. Review
by Andrey M. Borodin, cleanup and improvements by me.
Author: Arseniy Mukhin <arseniy.mukhin....@gmail.com>
Reviewed-by: Andrey M. Borodin <x4...@yandex-team.ru>
Discussion: https://postgr.es/m/CAE7r3MJ611B9TE=yqbbncewp7-k64vws+sjk7xf6fjux77u...@mail.gmail.com
---
contrib/amcheck/t/006_verify_gin.pl | 78 +++++++++++++++++++++++++++++
contrib/amcheck/verify_gin.c | 8 +--
2 files changed, 82 insertions(+), 4 deletions(-)
diff --git a/contrib/amcheck/t/006_verify_gin.pl b/contrib/amcheck/t/006_verify_gin.pl
index 7fdde170e06..308e53b2f75 100644
--- a/contrib/amcheck/t/006_verify_gin.pl
+++ b/contrib/amcheck/t/006_verify_gin.pl
@@ -34,6 +34,8 @@ $node->safe_psql(
invalid_entry_order_leaf_page_test();
invalid_entry_order_inner_page_test();
invalid_entry_columns_order_test();
+inconsistent_with_parent_key__parent_key_corrupted_test();
+inconsistent_with_parent_key__child_key_corrupted_test();
sub invalid_entry_order_leaf_page_test
{
@@ -159,6 +161,82 @@ sub invalid_entry_columns_order_test
like($stderr, qr/$expected/);
}
+sub inconsistent_with_parent_key__parent_key_corrupted_test
+{
+ my $relname = "test";
+ my $indexname = "test_gin_idx";
+
+ # fill the table until we have a split
+ $node->safe_psql(
+ 'postgres', qq(
+ DROP TABLE IF EXISTS $relname;
+ CREATE TABLE $relname (a text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'llllllllll' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'mmmmmmmmmm' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'nnnnnnnnnn' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'xxxxxxxxxx' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'yyyyyyyyyy' || random_string($filler_size) ||'}')::text[]);
+ CREATE INDEX $indexname ON $relname USING gin (a);
+ ));
+ my $relpath = relation_filepath($indexname);
+
+ $node->stop;
+
+ my $blkno = 1; # root
+
+ # we have nnnnnnnnnn... as parent key in the root, so replace it with something smaller then child's keys
+ string_replace_block(
+ $relpath,
+ 'nnnnnnnnnn',
+ 'aaaaaaaaaa',
+ $blkno
+ );
+
+ $node->start;
+
+ my ($result, $stdout, $stderr) = $node->psql('postgres', qq(SELECT gin_index_check('$indexname')));
+ my $expected = "index \"$indexname\" has inconsistent records on page 3 offset 3";
+ like($stderr, qr/$expected/);
+}
+
+sub inconsistent_with_parent_key__child_key_corrupted_test
+{
+ my $relname = "test";
+ my $indexname = "test_gin_idx";
+
+ # fill the table until we have a split
+ $node->safe_psql(
+ 'postgres', qq(
+ DROP TABLE IF EXISTS $relname;
+ CREATE TABLE $relname (a text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'llllllllll' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'mmmmmmmmmm' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'nnnnnnnnnn' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'xxxxxxxxxx' || random_string($filler_size) ||'}')::text[]);
+ INSERT INTO $relname (a) VALUES (('{' || 'yyyyyyyyyy' || random_string($filler_size) ||'}')::text[]);
+ CREATE INDEX $indexname ON $relname USING gin (a);
+ ));
+ my $relpath = relation_filepath($indexname);
+
+ $node->stop;
+
+ my $blkno = 3; # leaf
+
+ # we have nnnnnnnnnn... as parent key in the root, so replace child key with something bigger
+ string_replace_block(
+ $relpath,
+ 'nnnnnnnnnn',
+ 'pppppppppp',
+ $blkno
+ );
+
+ $node->start;
+
+ my ($result, $stdout, $stderr) = $node->psql('postgres', qq(SELECT gin_index_check('$indexname')));
+ my $expected = "index \"$indexname\" has inconsistent records on page 3 offset 3";
+ like($stderr, qr/$expected/);
+}
+
# Returns the filesystem path for the named relation.
sub relation_filepath
{
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index 25d47eefcbc..ae36a4ea587 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -613,10 +613,10 @@ gin_check_parent_keys_consistency(Relation rel,
ptr = (GinScanItem *) palloc(sizeof(GinScanItem));
ptr->depth = stack->depth + 1;
/* last tuple in layer has no high key */
- if (i != maxoff && !GinPageGetOpaque(page)->rightlink)
- ptr->parenttup = CopyIndexTuple(idxtuple);
- else
+ if (i == maxoff && rightlink == InvalidBlockNumber)
ptr->parenttup = NULL;
+ else
+ ptr->parenttup = CopyIndexTuple(idxtuple);
ptr->parentblk = stack->blkno;
ptr->blkno = GinGetDownlink(idxtuple);
ptr->parentlsn = lsn;
@@ -754,7 +754,7 @@ gin_refind_parent(Relation rel, BlockNumber parentblkno,
ItemId p_iid = PageGetItemIdCareful(rel, parentblkno, parentpage, o);
IndexTuple itup = (IndexTuple) PageGetItem(parentpage, p_iid);
- if (ItemPointerGetBlockNumber(&(itup->t_tid)) == childblkno)
+ if (GinGetDownlink(itup) == childblkno)
{
/* Found it! Make copy and return it */
result = CopyIndexTuple(itup);
--
2.49.0
From 65efe9243fcbabe0b8f28fcc743524060cadd730 Mon Sep 17 00:00:00 2001
From: Arseniy Mukhin <arseniy.mukhin....@gmail.com>
Date: Mon, 9 Jun 2025 20:49:11 +0300
Subject: [PATCH v8 5/6] amcheck: Fix posting tree checks in gin_index_check()
Fix two issues in parent_key validation in posting trees:
* It's not enough to check stack->parentblk is valid to determine if the
parentkey is valid. It's possible parentblk is set to a valid block
number, but parentkey is invalid. So check parentkey directly.
* We don't need to invalidate parentkey for all child pages of the
rightmost page. It's enough to invalidate it for the rightmost child
only, which means we can check more cases (less false negatives).
Issues reported by Arseniy Mikhin, along with a proposed patch. Review
by Andrey M. Borodin, cleanup and improvements by me.
Author: Arseniy Mukhin <arseniy.mukhin....@gmail.com>
Reviewed-by: Andrey M. Borodin <x4...@yandex-team.ru>
Discussion: https://postgr.es/m/CAE7r3MJ611B9TE=yqbbncewp7-k64vws+sjk7xf6fjux77u...@mail.gmail.com
---
contrib/amcheck/t/006_verify_gin.pl | 39 +++++++++++++++++++++++++++++
contrib/amcheck/verify_gin.c | 12 +++------
2 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/contrib/amcheck/t/006_verify_gin.pl b/contrib/amcheck/t/006_verify_gin.pl
index 308e53b2f75..e540cd6606a 100644
--- a/contrib/amcheck/t/006_verify_gin.pl
+++ b/contrib/amcheck/t/006_verify_gin.pl
@@ -36,6 +36,7 @@ invalid_entry_order_inner_page_test();
invalid_entry_columns_order_test();
inconsistent_with_parent_key__parent_key_corrupted_test();
inconsistent_with_parent_key__child_key_corrupted_test();
+inconsistent_with_parent_key__parent_key_corrupted_posting_tree_test();
sub invalid_entry_order_leaf_page_test
{
@@ -237,6 +238,44 @@ sub inconsistent_with_parent_key__child_key_corrupted_test
like($stderr, qr/$expected/);
}
+sub inconsistent_with_parent_key__parent_key_corrupted_posting_tree_test
+{
+ my $relname = "test";
+ my $indexname = "test_gin_idx";
+
+ $node->safe_psql(
+ 'postgres', qq(
+ DROP TABLE IF EXISTS $relname;
+ CREATE TABLE $relname (a text[]);
+ INSERT INTO $relname (a) select ('{aaaaa}') from generate_series(1,10000);
+ CREATE INDEX $indexname ON $relname USING gin (a);
+ ));
+ my $relpath = relation_filepath($indexname);
+
+ $node->stop;
+
+ my $blkno = 2; # posting tree root
+
+ # we have a posting tree for 'aaaaa' key with the root at 2nd block
+ # and two leaf pages 3 and 4. replace 4th page's high key with (1,1)
+ # so that there are tid's in leaf page that are larger then the new high key.
+ my $find = pack('S*', 0, 4, 0) . '....';
+ my $replace = pack('S*', 0, 4, 0, 1, 1);
+ string_replace_block(
+ $relpath,
+ $find,
+ $replace,
+ $blkno
+ );
+
+ $node->start;
+
+ my ($result, $stdout, $stderr) = $node->psql('postgres', qq(SELECT gin_index_check('$indexname')));
+ my $expected = "index \"$indexname\": tid exceeds parent's high key in postingTree leaf on block 4";
+ like($stderr, qr/$expected/);
+}
+
+
# Returns the filesystem path for the named relation.
sub relation_filepath
{
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index ae36a4ea587..a1d57ac551f 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -346,7 +346,7 @@ gin_check_posting_tree_parent_keys_consistency(Relation rel, BlockNumber posting
* Check if this tuple is consistent with the downlink in the
* parent.
*/
- if (stack->parentblk != InvalidBlockNumber && i == maxoff &&
+ if (i == maxoff && ItemPointerIsValid(&stack->parentkey) &&
ItemPointerCompare(&stack->parentkey, &posting_item->key) < 0)
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
@@ -359,14 +359,10 @@ gin_check_posting_tree_parent_keys_consistency(Relation rel, BlockNumber posting
ptr->depth = stack->depth + 1;
/*
- * Set rightmost parent key to invalid item pointer. Its value
- * is 'Infinity' and not explicitly stored.
+ * The rightmost parent key is always invalid item pointer.
+ * Its value is 'Infinity' and not explicitly stored.
*/
- if (rightlink == InvalidBlockNumber)
- ItemPointerSetInvalid(&ptr->parentkey);
- else
- ptr->parentkey = posting_item->key;
-
+ ptr->parentkey = posting_item->key;
ptr->parentblk = stack->blkno;
ptr->blkno = BlockIdGetBlockNumber(&posting_item->child_blkno);
ptr->next = stack->next;
--
2.49.0
From ffc02618b19ff315f8daf992ad4a6d3d577adcca Mon Sep 17 00:00:00 2001
From: Arseniy Mukhin <arseniy.mukhin....@gmail.com>
Date: Mon, 9 Jun 2025 20:51:05 +0300
Subject: [PATCH v8 6/6] amcheck: Remove unused GinScanItem->parentlsn field
The field was introduced by commit 14ffaece0fb5, but is unused and
unnecessary. So remove it.
Issues reported by Arseniy Mikhin, along with a proposed patch. Review
by Andrey M. Borodin, cleanup and improvements by me.
Author: Arseniy Mukhin <arseniy.mukhin....@gmail.com>
Reviewed-by: Andrey M. Borodin <x4...@yandex-team.ru>
Discussion: https://postgr.es/m/CAE7r3MJ611B9TE=yqbbncewp7-k64vws+sjk7xf6fjux77u...@mail.gmail.com
---
contrib/amcheck/verify_gin.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index a1d57ac551f..c615d950736 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -38,7 +38,6 @@ typedef struct GinScanItem
int depth;
IndexTuple parenttup;
BlockNumber parentblk;
- XLogRecPtr parentlsn;
BlockNumber blkno;
struct GinScanItem *next;
} GinScanItem;
@@ -417,7 +416,6 @@ gin_check_parent_keys_consistency(Relation rel,
stack->depth = 0;
stack->parenttup = NULL;
stack->parentblk = InvalidBlockNumber;
- stack->parentlsn = InvalidXLogRecPtr;
stack->blkno = GIN_ROOT_BLKNO;
while (stack)
@@ -428,7 +426,6 @@ gin_check_parent_keys_consistency(Relation rel,
OffsetNumber i,
maxoff,
prev_attnum;
- XLogRecPtr lsn;
IndexTuple prev_tuple;
BlockNumber rightlink;
@@ -438,7 +435,6 @@ gin_check_parent_keys_consistency(Relation rel,
RBM_NORMAL, strategy);
LockBuffer(buffer, GIN_SHARE);
page = (Page) BufferGetPage(buffer);
- lsn = BufferGetLSNAtomic(buffer);
maxoff = PageGetMaxOffsetNumber(page);
rightlink = GinPageGetOpaque(page)->rightlink;
@@ -481,7 +477,6 @@ gin_check_parent_keys_consistency(Relation rel,
ptr->depth = stack->depth;
ptr->parenttup = CopyIndexTuple(stack->parenttup);
ptr->parentblk = stack->parentblk;
- ptr->parentlsn = stack->parentlsn;
ptr->blkno = rightlink;
ptr->next = stack->next;
stack->next = ptr;
@@ -615,7 +610,6 @@ gin_check_parent_keys_consistency(Relation rel,
ptr->parenttup = CopyIndexTuple(idxtuple);
ptr->parentblk = stack->blkno;
ptr->blkno = GinGetDownlink(idxtuple);
- ptr->parentlsn = lsn;
ptr->next = stack->next;
stack->next = ptr;
}
--
2.49.0
