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>
In-Reply-To: <8635D8B1-45B7-4D67-86C7-D6ECB4F10B11@enterprisedb.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Fri, 10 May 2024 22:05:01 +0300
Message-ID: <CAPpHfduspEtrBN_BrAg90iW1VdoJcdTH7Hnx68GeiEqpAfocpg@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/CAPpHfduspEtrBN_BrAg90iW1VdoJcdTH7Hnx68GeiEqpAfocpg%40mail.gmail.com>
Precedence: bulk

On Fri, May 10, 2024 at 8:35=E2=80=AFPM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > On May 10, 2024, at 5:10 AM, Pavel Borisov <pashkin.elfe@gmail.com> wro=
te:
> > 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> wr=
ote:
> > > 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.
> >
> > 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. Thi=
s doesn't change code behavior.
> >
> > Patch v2-0003 doesn't contain credits and a discussion link. All other =
patches do.
> >
> > Overall, patches contain small performance optimization (0001), code re=
factoring and error reporting changes. IMO they could be pushed post-FF.
>
> v2-0001's commit message itself says, "This commit implements skipping ke=
ys". 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 un=
iqueness 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 a=
t this point in the release cycle.

Formally this could be classified as algorithmic change and probably
should be postponed to the next release.  But that's quite local
optimization, which just postpones a function call within the same
iteration of loop.  And the effect is huge.  Probably we could allow
this post-FF in the sake of quality release, given it's very local
change with a huge effect.

> v2-0002 does appear to just be refactoring.  I don't care for a small por=
tion of that patch, but I doubt it violates the post feature freeze rules. =
 In particular:
>
>   +   BtreeLastVisibleEntry lVis =3D {InvalidBlockNumber, InvalidOffsetNu=
mber, -1, NULL};

I don't understand what is the problem with this line and post feature
freeze rules.  Please, explain it more.

> v2-0003 may be an improvement in some way, but it compounds some preexist=
ing confusion also.  There is already a member of the BtreeCheckState calle=
d "target" and a memory context in that struct called "targetcontext".  Tha=
t context is used to allocate pages "state->target", "rightpage", "child" a=
nd "page", but not "metapage".  Perhaps "targetcontext" is a poor choice of=
 name?  "notmetacontext" is a terrible name, but closer to describing the p=
urpose 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 *
>          *
>          * Additional check of child items iff this is an internal page a=
nd
>          * caller holds a ShareLock.  This happens for every downlink (it=
em)
>          * in target excluding the negative-infinity downlink (again, thi=
s is
>          * because it has no useful value to compare).
>          */
>         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.

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.

0002 and 0003 don't address any bugs, but It would be very nice to
accept them, because it would simplify future backpatching in this
area.

> v2-0004 guards against a real threat, and is reasonable post feature free=
ze

Ok.

------
Regards,
Alexander Korotkov
Supabase