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: <CALT9ZEFS8z2KoZ9omyfVyz-h54DSPP_jLp5Zc3Fzt6g1Fnw9MA@mail.gmail.com>
Date: Fri, 10 May 2024 10:35:13 -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: <8635D8B1-45B7-4D67-86C7-D6ECB4F10B11@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>
To: Pavel Borisov <pashkin.elfe@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/8635D8B1-45B7-4D67-86C7-D6ECB4F10B11%40enterprisedb.com>
Precedence: bulk


> On May 10, 2024, at 5:10 AM, Pavel Borisov <pashkin.elfe@gmail.com> =
wrote:
>=20
> Hi, Alexander!
>=20
> On Fri, 10 May 2024 at 12:39, Alexander Korotkov =
<aekorotkov@gmail.com> wrote:
> On Fri, May 10, 2024 at 3:43=E2=80=AFAM Tom Lane <tgl@sss.pgh.pa.us> =
wrote:
> > Alexander Korotkov <aekorotkov@gmail.com> writes:
> > > The revised patchset is attached.  I applied cosmetical changes.  =
I'm
> > > going to push it if no objections.
> >
> > Is this really suitable material to be pushing post-feature-freeze?
> > It doesn't look like it's fixing any new-in-v17 issues.
>=20
> These are code improvements to the 5ae2087202, which answer critics in
> the thread.  0001 comprises an optimization, but it's rather small and
> simple.  0002 and 0003 contain refactoring.  0004 contains better
> error reporting.  For me this looks like pretty similar to what others
> commit post-FF, isn't it?
> I've re-checked patches v2. Differences from v1 are in improving =
naming/pgindent's/commit messages.
> In 0002 order of variables in struct BtreeLastVisibleEntry changed. =
This doesn't change code behavior.
>=20
> Patch v2-0003 doesn't contain credits and a discussion link. All other =
patches do.=20
>=20
> Overall, patches contain small performance optimization (0001), code =
refactoring and error reporting changes. IMO they could be pushed =
post-FF.

v2-0001's commit message itself says, "This commit implements skipping =
keys". I take no position on the correctness or value of the =
improvement, but it seems out of scope post feature freeze.  The patch =
seems to postpone uniqueness checking until later in the scan than what =
the prior version did, and that kind of change could require more =
analysis than we have time for at this point in the release cycle.


v2-0002 does appear to just be refactoring.  I don't care for a small =
portion of that patch, but I doubt it violates the post feature freeze =
rules.  In particular:

  +   BtreeLastVisibleEntry lVis =3D {InvalidBlockNumber, =
InvalidOffsetNumber, -1, NULL};


v2-0003 may be an improvement in some way, but it compounds some =
preexisting confusion also.  There is already a member of the =
BtreeCheckState called "target" and a memory context in that struct =
called "targetcontext".  That context is used to allocate pages =
"state->target", "rightpage", "child" and "page", but not "metapage".  =
Perhaps "targetcontext" is a poor choice of name?  "notmetacontext" is a =
terrible name, but closer to describing the purpose of the memory =
context.  Care to propose something sensible?

Prior to applying v2-0003, the rightpage was stored in state->target, =
and continued to be in state->target later when entering

        /*
         * * Downlink check *
         * =20
         * Additional check of child items iff this is an internal page =
and
         * caller holds a ShareLock.  This happens for every downlink =
(item)
         * in target excluding the negative-infinity downlink (again, =
this is
         * because it has no useful value to compare).
         */=20
        if (!P_ISLEAF(topaque) && state->readonly)
            bt_child_check(state, skey, offset);

and thereafter.  Now, the rightpage of state->target is created, =
checked, and free'd, and then the old state->target gets processed in =
the downlink check and thereafter.  This is either introducing a bug, or =
fixing one, but the commit message is totally ambiguous about whether =
this is a bugfix or a code cleanup or something else?  I think this kind =
of patch should have a super clear commit message about what it thinks =
it is doing.


v2-0004 guards against a real threat, and is reasonable post feature =
freeze


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