Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1s81aA-00929s-TH for pgsql-hackers@arkaria.postgresql.org; Fri, 17 May 2024 17:41:28 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1s81aA-00FzED-QX for pgsql-hackers@arkaria.postgresql.org; Fri, 17 May 2024 17:41:26 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1s81aA-00FzE4-7E for pgsql-hackers@lists.postgresql.org; Fri, 17 May 2024 17:41:26 +0000 Received: from mail-pl1-x635.google.com ([2607:f8b0:4864:20::635]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1s81a6-000hC1-2J for pgsql-hackers@lists.postgresql.org; Fri, 17 May 2024 17:41:24 +0000 Received: by mail-pl1-x635.google.com with SMTP id d9443c01a7336-1eeb1a4c10aso15578105ad.3 for ; Fri, 17 May 2024 10:41:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb.com; s=google; t=1715967681; x=1716572481; darn=lists.postgresql.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=Y2wAFSSegXTaHc9DMY+xHCegtCjcyhdsIv5J4gUWFOs=; b=YGA9sNvqBegJrNGdlAsFSAYoNVTSpmYJpxpBA2dRMNw4T0BVuQgENK5b5hrHxpsKx3 o7vbZm8cPFLhffO2jMN4FsiNVR+H59K62Rey6oqRp2G8Ho945zRBEtV7o5QrPd5DgtcB XVAJf0DTkKWFiO2XPbrMGMWALCc9EcVFMdKh4u3CSNfhEnrWdVBjF5uKdoZBw0hdq+/b tNJtlORTmtK7J+kKKLC9IQ0njCzVvRaV/ZAXudYHE6fxqJTdjfdinxFOuuehZ+xvwtib qbwoLbZ2ZE/71BjFkxO2fxYWH8Ze5h0vbjZNc/GQkknwSEOfu2FYIZh9+5Ki6gpVHoO/ ZnMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715967681; x=1716572481; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Y2wAFSSegXTaHc9DMY+xHCegtCjcyhdsIv5J4gUWFOs=; b=aj40v4UPMrdk76LT7XdhwLRiSb2drlvd0WN4W03KeZZ0IKCY8TTyv/A6k/2rO1E4jr 5sJmRlJbdPiolLYO5c9SMGMvL18f0TMiUK4lGAFyPuzyO9FpFWfpaDmWim5EdmazQ79i dyofV+U6KWvQl1TH5Ab03abyOysgDHjHsrpPkuxHRKvSmDJg51rtQBXjY1dovrjzKorb vCufitS8fsGGR1pE127deUKWsYDH41etkvQcfhq3oL20MRH0yATKxE3/WqNZ7x+1qYrg OfcSVZbgwXRWE6qBm0q54Efdn0QhnUDV4Xuk0HvomIYhv4CFXbWADwTSEf9Y5PsZi1bb +RyA== X-Forwarded-Encrypted: i=1; AJvYcCWsoVCCSP29374vC2vcXAqostXNCf1nM2u1ARTd5bjjPBdJS1BZ/6cCFDo9aq4WkMcdU5ONbZBgW73/XjTeBBuBBiCsdXNY8ZdpFoCXOYUSvxM2 X-Gm-Message-State: AOJu0YxB0hROgFJwBaEnfoAZs3Tv7of5rmsBD66iJCMCFsRruAzQMHvu SVyYh7Xo6k96+Q1/mzU9q2bE/Xsjnq/FZ48Tk5ge/ni3xTFdAPjSMI57Rq0hmw== X-Google-Smtp-Source: AGHT+IH/Jo9Q2dXAC8kPx12yydkc/04ddJG4qlEz5lEUItF87Ecm3/UFsuni3S4+MFnySgwSZmGMqA== X-Received: by 2002:a05:6a20:2583:b0:1a8:4254:5cdf with SMTP id adf61e73a8af0-1afde0dc8c8mr24602474637.22.1715967680853; Fri, 17 May 2024 10:41:20 -0700 (PDT) Received: from smtpclient.apple (24-113-197-127.wavecable.com. [24.113.197.127]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-6f677a20c6bsm5109439b3a.129.2024.05.17.10.41.19 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 May 2024 10:41:20 -0700 (PDT) From: Mark Dilger Message-Id: <2EDAC2EB-E8C2-4B56-9094-288ABD45DA4E@enterprisedb.com> Content-Type: multipart/mixed; boundary="Apple-Mail=_D6E106FA-E0DC-4800-A3DA-05A5612B7C0A" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.700.6.1.1\)) Subject: Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Date: Fri, 17 May 2024 10:41:19 -0700 In-Reply-To: Cc: Pavel Borisov , Tom Lane , Noah Misch , Peter Eisentraut , Aleksander Alekseev , Postgres hackers , Maxim Orlov , Andres Freund , Greg Stark , Julien Rouhaud , David Steele , Peter Geoghegan , Maxim Orlov , lubennikovaav@gmail.com To: Alexander Korotkov References: <20220922151328.lakwdytvwnyyif3b@awork3.anarazel.de> <8c74cc5e-4b97-4cd7-9aaf-67886822a78c@eisentraut.org> <20240501022412.85@rfd.leadboat.com> <1375614.1715301775@sss.pgh.pa.us> <8635D8B1-45B7-4D67-86C7-D6ECB4F10B11@enterprisedb.com> X-Mailer: Apple Mail (2.3731.700.6.1.1) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_D6E106FA-E0DC-4800-A3DA-05A5612B7C0A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On May 17, 2024, at 3:11 AM, Alexander Korotkov = wrote: >=20 > On Mon, May 13, 2024 at 4:42=E2=80=AFAM Alexander Korotkov = wrote: >> On Mon, May 13, 2024 at 12:23=E2=80=AFAM Alexander Korotkov >> wrote: >>> On Sat, May 11, 2024 at 4:13=E2=80=AFAM Mark Dilger >>> wrote: >>>>> On May 10, 2024, at 12:05 PM, Alexander Korotkov = 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. >>>>=20 >>>> I find your argument unconvincing. >>>>=20 >>>> 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. >>>>=20 >>>> I'm left with four possibilities: >>>>=20 >>>>=20 >>>> 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. >>>>=20 >>>> 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. >>>>=20 >>>> 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. >>>>=20 >>>> 4) It's been broken all along and your patch just changes from = wrong to wrong. >>>>=20 >>>>=20 >>>> 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. >>>>=20 >>>> 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. >>>>=20 >>>> If (3) is true, then I'm complaining that the patch is flat busted. >>>>=20 >>>> If (4) is true, then maybe we should revert the entire feature, or = have a discussion of mitigation efforts that are needed. >>>>=20 >>>> Regardless of which of 1..4 you pick, I think it could all do with = more regression test coverage. >>>>=20 >>>>=20 >>>> For reference, I said something similar earlier today in another = email to this thread: >>>>=20 >>>> 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 ... ? >>>=20 >>> 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. >>=20 >> 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. >>=20 >> So, the picture for the patches is the following now. >> 0001 =E2=80=93 optimization, but rather simple and giving huge effect >> 0002 =E2=80=93 refactoring >> 0003 =E2=80=93 fix for the bug >> 0004 =E2=80=93 better error reporting >=20 > 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. >=20 > 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. --Apple-Mail=_D6E106FA-E0DC-4800-A3DA-05A5612B7C0A Content-Disposition: attachment; filename=v1-0001-Add-a-WIP-corruption-checker.patch Content-Type: application/octet-stream; x-unix-mode=0644; name="v1-0001-Add-a-WIP-corruption-checker.patch" Content-Transfer-Encoding: quoted-printable =46rom=20cc7bf7991ff827c4b41c0cd7888a073b91f66827=20Mon=20Sep=2017=20= 00:00:00=202001=0AFrom:=20Mark=20Dilger=20=0A= Date:=20Fri,=2017=20May=202024=2010:24:11=20-0700=0ASubject:=20[PATCH=20= v1]=20Add=20a=20WIP=20corruption=20checker=0A=0ATo=20help=20analyze=20= Alexander=20Korotkov's=20v3=20series=20of=20patches,=20add=20a=0A= corruption=20checker=20that=20runs=20an=20infinite=20loop=20corrupting=20= an=20index=0Aand=20seeing=20if=20the=20corruption=20is=20detected.=0A=0A= THIS=20IS=20NOT=20FOR=20COMMIT.=0A---=0A=20= contrib/amcheck/t/006_corrupt_idx.pl=20|=20136=20= +++++++++++++++++++++++++++=0A=201=20file=20changed,=20136=20= insertions(+)=0A=20create=20mode=20100644=20= contrib/amcheck/t/006_corrupt_idx.pl=0A=0Adiff=20--git=20= a/contrib/amcheck/t/006_corrupt_idx.pl=20= b/contrib/amcheck/t/006_corrupt_idx.pl=0Anew=20file=20mode=20100644=0A= index=200000000000..d27b0ff9b6=0A---=20/dev/null=0A+++=20= b/contrib/amcheck/t/006_corrupt_idx.pl=0A@@=20-0,0=20+1,136=20@@=0A+=0A= +#=20Copyright=20(c)=202023-2024,=20PostgreSQL=20Global=20Development=20= Group=0A+=0A+#=20This=20regression=20test=20checks=20the=20behavior=20of=20= the=20btree=20validation=20in=20the=0A+#=20presence=20of=20breaking=20= sort=20order=20changes.=0A+#=0A+use=20strict;=0A+use=20warnings=20FATAL=20= =3D>=20'all';=0A+use=20PostgreSQL::Test::Cluster;=0A+use=20= PostgreSQL::Test::Utils;=0A+use=20Test::More;=0A+use=20Fcntl=20= 'SEEK_SET';=0A+=0A+my=20$node=20=3D=20= PostgreSQL::Test::Cluster->new('test');=0A+$node->init;=0A= +$node->append_conf('postgresql.conf',=20'autovacuum=20=3D=20off');=0A= +$node->start;=0A+=0A+#=20Create=20tables=20and=20indexes=20over=20some=20= values=0A+$node->safe_psql('postgres',qq(=0A+CREATE=20EXTENSION=20= amcheck;=0A+CREATE=20TABLE=20tbl=20(i=20TEXT);=0A+INSERT=20INTO=20tbl=20= (SELECT=20'MAGIC_'=20||=20gs::TEXT=20FROM=20generate_series(1,100000)=20= gs=20ORDER=20BY=20RANDOM());=0A+CREATE=20UNIQUE=20INDEX=20idx=20ON=20tbl=20= (i);=0A+));=09=0A+=0A+my=20@blocks;=0A+=0A+while=20(1)=0A+{=0A+=09my=20= ($result,=20$stdout,=20$stderr);=0A+=0A+=09$result=20=3D=20= $node->safe_psql(=0A+=09=09'postgres',=20q(=0A+=09=09SELECT=20= bt_index_parent_check('idx',=20true,=20true,=20true);=0A+=09));=0A+=09= is($result,=20'',=20'run=20amcheck=20on=20non-broken=20idx');=0A+=0A+=09= my=20$pgdata=20=3D=20$node->data_dir;=0A+=09my=20$rel=20=3D=20= $node->safe_psql('postgres',=0A+=09=09qq(SELECT=20= pg_relation_filepath('public.idx')));=0A+=09my=20$relpath=20=3D=20= "$pgdata/$rel";=0A+=09$node->stop;=0A+=0A+=09my=20($blksize,=20@blocks)=20= =3D=20read_blocks($relpath);=0A+=0A+=09my=20$ttl=20=3D=201000;=0A+=09my=20= $corrupted_blkno=20=3D=20undef;=0A+=09while=20= (!defined($corrupted_blkno)=20&&=20$ttl--)=0A+=09{=0A+=09=09my=20$blkno=20= =3D=20int(rand(scalar(@blocks)-1));=0A+=0A+=09=09if=20($blocks[$blkno]=20= =3D~=20m/.*MAGIC_(\d+)/)=0A+=09=09{=0A+=09=09=09my=20$magic=20=3D=20$1;=0A= +=09=09=09my=20$corrupted_block=20=3D=20$blocks[$blkno];=0A+=0A+=09=09=09= my=20$next_magic=20=3D=20$magic=20+=201;=0A+=09=09=09my=20$prev_magic=20= =3D=20$magic=20-=201;=0A+=09=09=09if=20($next_magic=20>=3D=201=20&&=20= $next_magic=20<=3D=20100000=20&&=20$blocks[$blkno+1]=20=3D~=20= m/MAGIC_$next_magic/)=0A+=09=09=09{=0A+=09=09=09=09if=20= ($corrupted_block=20=3D~=20s/MAGIC_$magic/MAGIC_$next_magic/)=0A+=09=09=09= =09{=0A+=09=09=09=09=09write_block($relpath,=20$blksize,=20$blkno,=20= $corrupted_block);=0A+=09=09=09=09=09$corrupted_blkno=20=3D=20$blkno;=0A= +=09=09=09=09}=0A+=09=09=09}=0A+=09=09=09elsif=20($prev_magic=20>=3D=201=20= &&=20$prev_magic=20<=3D=20100000=20&&=20$blocks[$blkno+1]=20=3D~=20= m/MAGIC_$prev_magic/)=0A+=09=09=09{=0A+=09=09=09=09if=20= ($corrupted_block=20=3D~=20s/MAGIC_$magic/MAGIC_$prev_magic/)=0A+=09=09=09= =09{=0A+=09=09=09=09=09write_block($relpath,=20$blksize,=20$blkno,=20= $corrupted_block);=0A+=09=09=09=09=09$corrupted_blkno=20=3D=20$blkno;=0A= +=09=09=09=09}=0A+=09=09=09}=0A+=09=09}=0A+=09}=0A+=0A+=09= BAIL_OUT("Failed=20to=20corrupt=20anything")=20unless($ttl=20>=200);=0A+=0A= +=09#=20Ok,=20we've=20corrupted=20the=20file.=20=20Restart=20the=20node=20= and=20see=20if=20the=0A+=09#=20corruption=20checker=20notices=20= anything.=0A+=09$node->start;=0A+=0A+=09($result,=20$stdout,=20$stderr)=20= =3D=20$node->psql(=0A+=09=09'postgres',=20q(=0A+=09=09SELECT=20= bt_index_parent_check('idx',=20true,=20true,=20true);=0A+=09));=0A+=09= ok(=20$stderr=20=3D~=20/item=20order=20invariant=20violated=20for=20= index=20"idx"|index=20uniqueness=20is=20violated=20for=20index=20= "idx"|could=20not=20find=20tuple=20using=20search=20from=20root=20page=20= in=20index=20"idx"|mismatch=20between=20parent=20key=20and=20child=20= high=20key=20in=20index=20"idx"|detected=20uniqueness=20violation=20for=20= index=20"idx"/);=0A+=0A+=09#=20Repair=20the=20damage.=0A+=09$node->stop;=0A= +=09write_block($relpath,=20$blksize,=20$corrupted_blkno,=20= $blocks[$corrupted_blkno]);=0A+=0A+=09#=20Restart=20the=20database=20and=20= confirm=20the=20index=20is=20back=20to=20passing=0A+=09$node->start;=0A+=20= =09$result=20=3D=20$node->safe_psql(=0A+=09=09'postgres',=20q(=0A+=09=09= SELECT=20bt_index_check('idx',=20true,=20true);=0A+=09));=0A+=09= is($result,=20'',=20'run=20amcheck=20on=20non-broken=20idx');=0A+}=0A+=0A= +#=20Not=20reached=0A+done_testing();=0A+=0A+sub=20read_blocks=0A+{=0A+=09= my=20($relpath)=20=3D=20@_;=0A+=09my=20$file;=0A+=09open($file,=20'+<',=20= $relpath)=0A+=09=09or=20BAIL_OUT("open=20failed:=20$!");=0A+=09binmode=20= $file;=0A+=09my=20($dev,=20$ino,=20$mode,=20$nlink,=20$uid,=20$gid,=20= $rdev,=20$size,=20$atime,=20$mtime,=20$ctime,=20$blksize,=20$blocks)=20=3D= =20stat($file);=0A+=0A+=09my=20@result;=0A+=09for=20(my=20$blkno=20=3D=20= 0;=20$blkno=20<=20$blocks;=20$blkno++)=0A+=09{=0A+=09=09sysseek($file,=20= $blkno=20*=20$blksize,=20SEEK_SET);=0A+=09=09sysread($file,=20= $result[$blkno],=20$blksize);=0A+=09}=0A+=09close($file);=0A+=09return=20= ($blksize,=20@result);=0A+}=0A+=0A+sub=20write_block=0A+{=0A+=09my=20= ($relpath,=20$blksize,=20$blkno,=20$block)=20=3D=20@_;=0A+=09my=20$file;=0A= +=09open($file,=20'+<',=20$relpath)=0A+=09=09or=20BAIL_OUT("open=20= failed:=20$!");=0A+=09binmode=20$file;=0A+=09sysseek($file,=20$blkno=20*=20= $blksize,=20SEEK_SET);=0A+=09syswrite($file,=20$block);=0A+=09= close($file);=0A+}=0A--=20=0A2.39.3=20(Apple=20Git-145)=0A=0A= --Apple-Mail=_D6E106FA-E0DC-4800-A3DA-05A5612B7C0A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 =E2=80=94 Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company --Apple-Mail=_D6E106FA-E0DC-4800-A3DA-05A5612B7C0A--