Content-Type: text/plain;
	charset=utf-8
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.
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: <CAPpHfduspEtrBN_BrAg90iW1VdoJcdTH7Hnx68GeiEqpAfocpg@mail.gmail.com>
Date: Fri, 10 May 2024 18:12:57 -0700
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-Transfer-Encoding: quoted-printable
Message-Id: <BC582142-AD7E-4591-A541-7578B62DE3D8@enterprisedb.com>
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>
To: Alexander Korotkov <aekorotkov@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/BC582142-AD7E-4591-A541-7578B62DE3D8%40enterprisedb.com>
Precedence: bulk


> On May 10, 2024, at 12:05 PM, Alexander Korotkov =
<aekorotkov@gmail.com> wrote:
>=20
> 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.=20=


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 ... ?

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company