public inbox for [email protected]
help / color / mirror / Atom feedFrom: Mark Dilger <[email protected]>
To: Alexander Korotkov <[email protected]>
Cc: Pavel Borisov <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Aleksander Alekseev <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Maxim Orlov <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Greg Stark <[email protected]>
Cc: Julien Rouhaud <[email protected]>
Cc: David Steele <[email protected]>
Cc: Peter Geoghegan <[email protected]>
Cc: Maxim Orlov <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
Date: Fri, 17 May 2024 10:41:19 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAPpHfdvfA6nCqGOcNrv33YMR3_VkVZqdxWmk1RiFuA2M1DDGQg@mail.gmail.com>
References: <CALT9ZEG9Zk5TxuDTnZPGe9H+b7DL40gXfeouWCaZ3A70yvyWLw@mail.gmail.com>
<CALT9ZEG1YAfPBH-ZMnK-cxu0SDdrV_w0QEBs9BLHfWvddpOHFA@mail.gmail.com>
<[email protected]>
<CACG=ezbRVZrdTguWphiFP-L2cCi_jy+spZA0wPJWA806B0=2dQ@mail.gmail.com>
<CACG=ezYk7up-BUL30SrGozURYWt97BK2f=XhUL6S+qU3F9qVzQ@mail.gmail.com>
<CAJ7c6TOe-ifS9JAqhk-5rPrAbnfGkxNYQiWnR6CUaAqLP31T6A@mail.gmail.com>
<CAPpHfduqJ9QCOrXDjkPxPRxJRws1FgFELwB2Yz3L6-n7FQMsrA@mail.gmail.com>
<[email protected]>
<CAPpHfdvY7UitNMgUKVx73+W_6STCRoiRBmoE2t7WP1-KBodT+g@mail.gmail.com>
<CALT9ZEFY-vsL4-cxta49wP=H7RjDaZk+Wg-ft4m-kVS_=WMsAQ@mail.gmail.com>
<[email protected]>
<CAPpHfdsLcQ2w24r9CCNVwaDwKMXabQzG15KwSaaqMAczF1DdVg@mail.gmail.com>
<CAPpHfdvxK+KSDqopaVdmCHgbO92AN4ibrbiCJ6i6-+g4oma-6Q@mail.gmail.com>
<[email protected]>
<CAPpHfdstoBC7TLsLZ5eKUW7LNHcVo2d5X4msmgsigb7nxiD-5A@mail.gmail.com>
<CALT9ZEFS8z2KoZ9omyfVyz-h54DSPP_jLp5Zc3Fzt6g1Fnw9MA@mail.gmail.com>
<[email protected]>
<CAPpHfduspEtrBN_BrAg90iW1VdoJcdTH7Hnx68GeiEqpAfocpg@mail.gmail.com>
<[email protected]>
<CAPpHfdu4jF3TrML0rcNczYcf__xXRycKzn2Vi+kKgiYNNC0w-A@mail.gmail.com>
<CAPpHfdu6bGLxe-+9jfDwoji_T2iD6mjdiAGBr0JT-iX+TWNWOw@mail.gmail.com>
<CAPpHfdvfA6nCqGOcNrv33YMR3_VkVZqdxWmk1RiFuA2M1DDGQg@mail.gmail.com>
> On May 17, 2024, at 3:11 AM, Alexander Korotkov <[email protected]> wrote:
>
> On Mon, May 13, 2024 at 4:42 AM Alexander Korotkov <[email protected]> wrote:
>> On Mon, May 13, 2024 at 12:23 AM Alexander Korotkov
>> <[email protected]> wrote:
>>> On Sat, May 11, 2024 at 4:13 AM Mark Dilger
>>> <[email protected]> wrote:
>>>>> On May 10, 2024, at 12:05 PM, Alexander Korotkov <[email protected]> wrote:
>>>>> The only bt_target_page_check() caller is
>>>>> bt_check_level_from_leftmost(), which overrides state->target in the
>>>>> next iteration anyway. I think the patch is just refactoring to
>>>>> eliminate the confusion pointer by Peter Geoghegan upthread.
>>>>
>>>> I find your argument unconvincing.
>>>>
>>>> After bt_target_page_check() returns at line 919, and before bt_check_level_from_leftmost() overrides state->target in the next iteration, bt_check_level_from_leftmost() conditionally fetches an item from the page referenced by state->target. See line 963.
>>>>
>>>> I'm left with four possibilities:
>>>>
>>>>
>>>> 1) bt_target_page_check() never gets to the code that uses "rightpage" rather than "state->target" in the same iteration where bt_check_level_from_leftmost() conditionally fetches an item from state->target, so the change you're making doesn't matter.
>>>>
>>>> 2) The code prior to v2-0003 was wrong, having changed state->target in an inappropriate way, causing the wrong thing to happen at what is now line 963. The patch fixes the bug, because state->target no longer gets overwritten where you are now using "rightpage" for the value.
>>>>
>>>> 3) The code used to work, having set up state->target correctly in the place where you are now using "rightpage", but v2-0003 has broken that.
>>>>
>>>> 4) It's been broken all along and your patch just changes from wrong to wrong.
>>>>
>>>>
>>>> If you believe (1) is true, then I'm complaining that you are relying far to much on action at a distance, and that you are not documenting it. Even with documentation of this interrelationship, I'd be unhappy with how brittle the code is. I cannot easily discern that the two don't ever happen in the same iteration, and I'm not at all convinced one way or the other. I tried to set up some Asserts about that, but none of the test cases actually reach the new code, so adding Asserts doesn't help to investigate the question.
>>>>
>>>> If (2) is true, then I'm complaining that the commit message doesn't mention the fact that this is a bug fix. Bug fixes should be clearly documented as such, otherwise future work might assume the commit can be reverted with only stylistic consequences.
>>>>
>>>> If (3) is true, then I'm complaining that the patch is flat busted.
>>>>
>>>> If (4) is true, then maybe we should revert the entire feature, or have a discussion of mitigation efforts that are needed.
>>>>
>>>> Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.
>>>>
>>>>
>>>> For reference, I said something similar earlier today in another email to this thread:
>>>>
>>>> This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did. That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch. Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used. So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else? Is this a bug being introduced, being fixed, or ... ?
>>>
>>> Thank you for your analysis. I'm inclined to believe in 2, but not
>>> yet completely sure. It's really pity that our tests don't cover
>>> this. I'm investigating this area.
>>
>> It seems that I got to the bottom of this. Changing
>> BtreeCheckState.target for a cross-page unique constraint check is
>> wrong, but that happens only for leaf pages. After that
>> BtreeCheckState.target is only used for setting the low key. The low
>> key is only used for non-leaf pages. So, that didn't lead to any
>> visible bug. I've revised the commit message to reflect this.
>>
>> So, the picture for the patches is the following now.
>> 0001 – optimization, but rather simple and giving huge effect
>> 0002 – refactoring
>> 0003 – fix for the bug
>> 0004 – better error reporting
>
> I think the thread contains enough motivation on why 0002, 0003 and
> 0004 are material for post-FF. They are fixes and refactoring for
> new-in-v17 feature. I'm going to push them if no objections.
>
> Regarding 0001, I'd like to ask Tom and Mark if they find convincing
> that given that optimization is small, simple and giving huge effect,
> it could be pushed post-FF? Otherwise, this could wait for v18.
I won't pretend to be part of the Release Management Team. Perhaps Tom wishes to respond.
I wrote a TAP test to check the uniqueness checker. bt_index_check() sometimes fails to detect a corruption. This is true both before and after applying v3-0001. The bt_index_parent_check() seems to always detect the corruption created by the TAP test. Likewise, this is true both before and after applying v3-0001.
The documentation in https://www.postgresql.org/docs/devel/amcheck.html#AMCHECK-FUNCTIONS is ambiguous:
"bt_index_check does not verify invariants that span child/parent relationships, but will verify the presence of all heap tuples as index tuples within the index when heapallindexed is true. When checkunique is true bt_index_check will check that no more than one among duplicate entries in unique index is visible. When a routine, lightweight test for corruption is required in a live production environment, using bt_index_check often provides the best trade-off between thoroughness of verification and limiting the impact on application performance and availability."
The second sentence, "When checkunique is true bt_index_check will check that no more than one among duplicate entries in unique index is visible." is not strictly true, as it won't check if the violation spans a page boundary. That's implied by the surrounding sentences, but I'm not sure a reader can be trusted to know which way to interpret how "checkunique" works. Clarification is needed.
The attached TAP test is not intended for commit. I am only including it here because you might want to use the TAP test as a starting point for creating and testing for new kinds of corruption. Beware the test intentionally includes an infinite loop, which is helpful for a developer examining the code, but not at all appropriate otherwise. It loads all blocks of the index into memory each loop, which could be made more efficient if we wanted this to be part of the core codebase. I just threw it together this morning. It's not polished, documented, checked for portability, or otherwise production quality.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[application/octet-stream] v1-0001-Add-a-WIP-corruption-checker.patch (4.6K, ../[email protected]/2-v1-0001-Add-a-WIP-corruption-checker.patch)
download | inline diff:
From cc7bf7991ff827c4b41c0cd7888a073b91f66827 Mon Sep 17 00:00:00 2001
From: Mark Dilger <[email protected]>
Date: Fri, 17 May 2024 10:24:11 -0700
Subject: [PATCH v1] Add a WIP corruption checker
To help analyze Alexander Korotkov's v3 series of patches, add a
corruption checker that runs an infinite loop corrupting an index
and seeing if the corruption is detected.
THIS IS NOT FOR COMMIT.
---
contrib/amcheck/t/006_corrupt_idx.pl | 136 +++++++++++++++++++++++++++
1 file changed, 136 insertions(+)
create mode 100644 contrib/amcheck/t/006_corrupt_idx.pl
diff --git a/contrib/amcheck/t/006_corrupt_idx.pl b/contrib/amcheck/t/006_corrupt_idx.pl
new file mode 100644
index 0000000000..d27b0ff9b6
--- /dev/null
+++ b/contrib/amcheck/t/006_corrupt_idx.pl
@@ -0,0 +1,136 @@
+
+# Copyright (c) 2023-2024, PostgreSQL Global Development Group
+
+# This regression test checks the behavior of the btree validation in the
+# presence of breaking sort order changes.
+#
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Fcntl 'SEEK_SET';
+
+my $node = PostgreSQL::Test::Cluster->new('test');
+$node->init;
+$node->append_conf('postgresql.conf', 'autovacuum = off');
+$node->start;
+
+# Create tables and indexes over some values
+$node->safe_psql('postgres',qq(
+CREATE EXTENSION amcheck;
+CREATE TABLE tbl (i TEXT);
+INSERT INTO tbl (SELECT 'MAGIC_' || gs::TEXT FROM generate_series(1,100000) gs ORDER BY RANDOM());
+CREATE UNIQUE INDEX idx ON tbl (i);
+));
+
+my @blocks;
+
+while (1)
+{
+ my ($result, $stdout, $stderr);
+
+ $result = $node->safe_psql(
+ 'postgres', q(
+ SELECT bt_index_parent_check('idx', true, true, true);
+ ));
+ is($result, '', 'run amcheck on non-broken idx');
+
+ my $pgdata = $node->data_dir;
+ my $rel = $node->safe_psql('postgres',
+ qq(SELECT pg_relation_filepath('public.idx')));
+ my $relpath = "$pgdata/$rel";
+ $node->stop;
+
+ my ($blksize, @blocks) = read_blocks($relpath);
+
+ my $ttl = 1000;
+ my $corrupted_blkno = undef;
+ while (!defined($corrupted_blkno) && $ttl--)
+ {
+ my $blkno = int(rand(scalar(@blocks)-1));
+
+ if ($blocks[$blkno] =~ m/.*MAGIC_(\d+)/)
+ {
+ my $magic = $1;
+ my $corrupted_block = $blocks[$blkno];
+
+ my $next_magic = $magic + 1;
+ my $prev_magic = $magic - 1;
+ if ($next_magic >= 1 && $next_magic <= 100000 && $blocks[$blkno+1] =~ m/MAGIC_$next_magic/)
+ {
+ if ($corrupted_block =~ s/MAGIC_$magic/MAGIC_$next_magic/)
+ {
+ write_block($relpath, $blksize, $blkno, $corrupted_block);
+ $corrupted_blkno = $blkno;
+ }
+ }
+ elsif ($prev_magic >= 1 && $prev_magic <= 100000 && $blocks[$blkno+1] =~ m/MAGIC_$prev_magic/)
+ {
+ if ($corrupted_block =~ s/MAGIC_$magic/MAGIC_$prev_magic/)
+ {
+ write_block($relpath, $blksize, $blkno, $corrupted_block);
+ $corrupted_blkno = $blkno;
+ }
+ }
+ }
+ }
+
+ BAIL_OUT("Failed to corrupt anything") unless($ttl > 0);
+
+ # Ok, we've corrupted the file. Restart the node and see if the
+ # corruption checker notices anything.
+ $node->start;
+
+ ($result, $stdout, $stderr) = $node->psql(
+ 'postgres', q(
+ SELECT bt_index_parent_check('idx', true, true, true);
+ ));
+ ok( $stderr =~ /item order invariant violated for index "idx"|index uniqueness is violated for index "idx"|could not find tuple using search from root page in index "idx"|mismatch between parent key and child high key in index "idx"|detected uniqueness violation for index "idx"/);
+
+ # Repair the damage.
+ $node->stop;
+ write_block($relpath, $blksize, $corrupted_blkno, $blocks[$corrupted_blkno]);
+
+ # Restart the database and confirm the index is back to passing
+ $node->start;
+ $result = $node->safe_psql(
+ 'postgres', q(
+ SELECT bt_index_check('idx', true, true);
+ ));
+ is($result, '', 'run amcheck on non-broken idx');
+}
+
+# Not reached
+done_testing();
+
+sub read_blocks
+{
+ my ($relpath) = @_;
+ my $file;
+ open($file, '+<', $relpath)
+ or BAIL_OUT("open failed: $!");
+ binmode $file;
+ my ($dev, $ino, $mode, $nlink, $uid, $gid, $rdev, $size, $atime, $mtime, $ctime, $blksize, $blocks) = stat($file);
+
+ my @result;
+ for (my $blkno = 0; $blkno < $blocks; $blkno++)
+ {
+ sysseek($file, $blkno * $blksize, SEEK_SET);
+ sysread($file, $result[$blkno], $blksize);
+ }
+ close($file);
+ return ($blksize, @result);
+}
+
+sub write_block
+{
+ my ($relpath, $blksize, $blkno, $block) = @_;
+ my $file;
+ open($file, '+<', $relpath)
+ or BAIL_OUT("open failed: $!");
+ binmode $file;
+ sysseek($file, $blkno * $blksize, SEEK_SET);
+ syswrite($file, $block);
+ close($file);
+}
--
2.39.3 (Apple Git-145)
view thread (24+ messages) latest in thread
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], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
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