MIME-Version: 1.0
References: <CALT9ZEG9Zk5TxuDTnZPGe9H+b7DL40gXfeouWCaZ3A70yvyWLw@mail.gmail.com>
 <CALT9ZEG1YAfPBH-ZMnK-cxu0SDdrV_w0QEBs9BLHfWvddpOHFA@mail.gmail.com>
 <20220922151328.lakwdytvwnyyif3b@awork3.anarazel.de> <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>
 <8c74cc5e-4b97-4cd7-9aaf-67886822a78c@eisentraut.org> <CAPpHfdvY7UitNMgUKVx73+W_6STCRoiRBmoE2t7WP1-KBodT+g@mail.gmail.com>
 <CALT9ZEFY-vsL4-cxta49wP=H7RjDaZk+Wg-ft4m-kVS_=WMsAQ@mail.gmail.com>
 <20240501022412.85@rfd.leadboat.com> <CAPpHfdsLcQ2w24r9CCNVwaDwKMXabQzG15KwSaaqMAczF1DdVg@mail.gmail.com>
 <CAPpHfdvxK+KSDqopaVdmCHgbO92AN4ibrbiCJ6i6-+g4oma-6Q@mail.gmail.com>
 <1375614.1715301775@sss.pgh.pa.us> <CAPpHfdstoBC7TLsLZ5eKUW7LNHcVo2d5X4msmgsigb7nxiD-5A@mail.gmail.com>
 <CALT9ZEFS8z2KoZ9omyfVyz-h54DSPP_jLp5Zc3Fzt6g1Fnw9MA@mail.gmail.com>
 <8635D8B1-45B7-4D67-86C7-D6ECB4F10B11@enterprisedb.com> <CAPpHfduspEtrBN_BrAg90iW1VdoJcdTH7Hnx68GeiEqpAfocpg@mail.gmail.com>
 <BC582142-AD7E-4591-A541-7578B62DE3D8@enterprisedb.com>
In-Reply-To: <BC582142-AD7E-4591-A541-7578B62DE3D8@enterprisedb.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Mon, 13 May 2024 00:23:42 +0300
Message-ID: <CAPpHfdu4jF3TrML0rcNczYcf__xXRycKzn2Vi+kKgiYNNC0w-A@mail.gmail.com>
Subject: Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
To: Mark Dilger <mark.dilger@enterprisedb.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, 
	Noah Misch <noah@leadboat.com>, Peter Eisentraut <peter@eisentraut.org>, 
	Aleksander Alekseev <aleksander@timescale.com>, 
	Postgres hackers <pgsql-hackers@lists.postgresql.org>, Maxim Orlov <orlovmg@gmail.com>, 
	Andres Freund <andres@anarazel.de>, Greg Stark <stark@mit.edu>, Julien Rouhaud <rjuju123@gmail.com>, 
	David Steele <david@pgmasters.net>, Peter Geoghegan <pg@bowt.ie>, Maxim Orlov <m.orlov@postgrespro.ru>, 
	lubennikovaav@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAPpHfdu4jF3TrML0rcNczYcf__xXRycKzn2Vi%2BkKgiYNNC0w-A%40mail.gmail.com>
Precedence: bulk

On Sat, May 11, 2024 at 4:13=E2=80=AFAM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > On May 10, 2024, at 12:05 PM, Alexander Korotkov <aekorotkov@gmail.com>=
 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_lev=
el_from_leftmost() overrides state->target in the next iteration, bt_check_=
level_from_leftmost() conditionally fetches an item from the page reference=
d 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" r=
ather 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 overwr=
itten where you are now using "rightpage" for the value.
>
> 3)  The code used to work, having set up state->target correctly in the p=
lace 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.  Eve=
n with documentation of this interrelationship, I'd be unhappy with how bri=
ttle the code is.  I cannot easily discern that the two don't ever happen i=
n 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 actual=
ly reach the new code, so adding Asserts doesn't help to investigate the qu=
estion.
>
> If (2) is true, then I'm complaining that the commit message doesn't ment=
ion the fact that this is a bug fix.  Bug fixes should be clearly documente=
d as such, otherwise future work might assume the commit can be reverted wi=
th 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 r=
egression 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 "righ=
tpage" rather than overwriting "state->target", which the old implementatio=
n 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 l=
ines further down in bt_check_level_from_leftmost() state->target is clearl=
y 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.

------
Regards,
Alexander Korotkov