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: <CALT9ZEHquSYBU9FZnkp-FJG-yQdJCEYiYDtYNY-grRfE19RanA@mail.gmail.com>
Date: Fri, 10 May 2024 12:27:11 -0700
Cc: Alexander Korotkov <aekorotkov@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: <A760AD31-D6D7-4086-9792-3818282E6AF9@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>
 <CALT9ZEHquSYBU9FZnkp-FJG-yQdJCEYiYDtYNY-grRfE19RanA@mail.gmail.com>
To: Pavel Borisov <pashkin.elfe@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/A760AD31-D6D7-4086-9792-3818282E6AF9%40enterprisedb.com>
Precedence: bulk


> On May 10, 2024, at 11:42 AM, Pavel Borisov <pashkin.elfe@gmail.com> =
wrote:
>=20
> IMO 0003 doesn't introduce nor fixes a bug. It loads rightpage into a =
local variable, rather that to a BtreeCheckState that can have another =
users of state->target afterb uniqueness check in the future, but don't =
have now. So the original patch is correct, and the goal of this =
refactoring is to untie rightpage fron state structure as it's used only =
transiently for cross-page unuque check. It's the same style as already =
used bt_right_page_check_scankey() that loads rightpage into a local =
variable.

Well, you can put an Assert(false) dead in the middle of the code we're =
discussing and all the regression tests still pass, so I'd argue the =
change is getting zero test coverage.

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

Having a regression test that actually touches this code would go a fair =
way towards helping the analysis.

> For 0002 I doubt I understand your actual positiob. Could you explain =
what it violates or doesn't violate?

v2-0002 is does not violate the post feature freeze restriction on new =
features so far as I can tell, but I just don't care for the variable =
initialization because it doesn't name the fields.  If anybody =
refactored the struct they might not notice that the need to reorder =
this initialization, and depending on various factors including compiler =
flags they might not get an error.

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