From f967749e3021e2c3b5c2680275f35aa6d428abf9 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 9 Jun 2025 01:45:56 +0200 Subject: [PATCH v5 3/4] patch 2: gin_check_parent_keys_consistency 1) When we add new items to the entry tree stack, ptr->parenttup is always null because GinPageGetOpaque(page)->rightlink is never NULL. /* last tuple in layer has no high key */ if (i != maxoff && !GinPageGetOpaque(page)->rightlink) ptr->parenttup = CopyIndexTuple(idxtuple); else ptr->parenttup = NULL; Right way to check if entry doesn't have right neighbour is GinPageGetOpaque(page)->rightlink == InvalidBlockNumber But even if we fix it, the condition would not do what the comment says. If we want to have NULL as parenttup only for the last tuple of the btree layer, the right check would be: if (i == maxoff && rightlink == InvalidBlockNumber) ptr->parenttup = NULL; else ptr->parenttup = CopyIndexTuple(idxtuple); 8) In the function gin_refind_parent() the code if (ItemPointerGetBlockNumber(&(itup->t_tid)) == childblkno) triggers Assert(ItemPointerIsValid(pointer)) within the ItemPointerGetBlockNumber(), because itup->t_tid could be invalid. AFAIS GIN code uses a special method GinGetDownlink(itup) that avoids this Assert. So we can use it here too. Please find the attached patch with fixes for issues above. Also there are added regression test for multicolumn index, and several tap tests with some basic corrupted index cases. I'm not sure if it's the right way to write such tests and would be glad to hear any feedback, especially about invalid_entry_columns_order_test() where it seems important to preserve byte ordering. Also all tests expect standard page size 8192 now. --- 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 8c5975d2e37..a999a13d183 100644 --- a/contrib/amcheck/t/006_verify_gin.pl +++ b/contrib/amcheck/t/006_verify_gin.pl @@ -31,6 +31,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 { @@ -158,6 +160,82 @@ sub invalid_entry_columns_order_test ok($stderr =~ "index \"$indexname\" has wrong tuple order on entry tree page, block 1, offset 2, rightlink 4294967295"); } +sub inconsistent_with_parent_key__parent_key_corrupted_test +{ + my $relname = "test"; + my $indexname = "test_gin_idx"; + + $node->safe_psql( + 'postgres', qq( + DROP TABLE IF EXISTS $relname; + CREATE TABLE $relname (a text[]); + CREATE INDEX $indexname ON $relname USING gin (a); + INSERT INTO $relname (a) VALUES (('{' || 'llllllllll' || random_string(1870) ||'}')::text[]); + INSERT INTO $relname (a) VALUES (('{' || 'mmmmmmmmmm' || random_string(1870) ||'}')::text[]); + INSERT INTO $relname (a) VALUES (('{' || 'nnnnnnnnnn' || random_string(1870) ||'}')::text[]); + INSERT INTO $relname (a) VALUES (('{' || 'xxxxxxxxxx' || random_string(1870) ||'}')::text[]); + INSERT INTO $relname (a) VALUES (('{' || 'yyyyyyyyyy' || random_string(1870) ||'}')::text[]); + SELECT gin_clean_pending_list('$indexname'); + )); + 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"', + $blksize, + $blkno + ); + + $node->start; + + my ($result, $stdout, $stderr) = $node->psql('postgres', qq(SELECT gin_index_check('$indexname'))); + ok($stderr =~ "index \"$indexname\" has inconsistent records on page 5 offset 3"); +} + +sub inconsistent_with_parent_key__child_key_corrupted_test +{ + my $relname = "test"; + my $indexname = "test_gin_idx"; + + $node->safe_psql( + 'postgres', qq( + DROP TABLE IF EXISTS $relname; + CREATE TABLE $relname (a text[]); + CREATE INDEX $indexname ON $relname USING gin (a); + INSERT INTO $relname (a) VALUES (('{' || 'llllllllll' || random_string(1870) ||'}')::text[]); + INSERT INTO $relname (a) VALUES (('{' || 'mmmmmmmmmm' || random_string(1870) ||'}')::text[]); + INSERT INTO $relname (a) VALUES (('{' || 'nnnnnnnnnn' || random_string(1870) ||'}')::text[]); + INSERT INTO $relname (a) VALUES (('{' || 'xxxxxxxxxx' || random_string(1870) ||'}')::text[]); + INSERT INTO $relname (a) VALUES (('{' || 'yyyyyyyyyy' || random_string(1870) ||'}')::text[]); + SELECT gin_clean_pending_list('$indexname'); + )); + my $relpath = relation_filepath($indexname); + + $node->stop; + + my $blkno = 5; # leaf + + # we have nnnnnnnnnn... as parent key in the root, so replace child key with something bigger + string_replace_block( + $relpath, + "nnnnnnnnnn", + '"pppppppppp"', + $blksize, + $blkno + ); + + $node->start; + + my ($result, $stdout, $stderr) = $node->psql('postgres', qq(SELECT gin_index_check('$indexname'))); + ok($stderr =~ "index \"$indexname\" has inconsistent records on page 5 offset 3"); +} + # 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 26b98571b56..8f6a5410cb7 100644 --- a/contrib/amcheck/verify_gin.c +++ b/contrib/amcheck/verify_gin.c @@ -609,10 +609,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; @@ -750,7 +750,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