Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

On Fri, May 10, 2024 at 8:35 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > On May 10, 2024, at 5:10 AM, Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > On Fri, 10 May 2024 at 12:39, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Fri, May 10, 2024 at 3:43 AM 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.
> >
> > 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.
> >
> > Patch v2-0003 doesn't contain credits and a discussion link. All other patches do.
> >
> > Overall, patches contain small performance optimization (0001), code refactoring and error reporting changes. IMO
theycould be pushed post-FF. 
>
> v2-0001's commit message itself says, "This commit implements skipping keys". I take no position on the correctness
orvalue of the improvement, but it seems out of scope post feature freeze.  The patch seems to postpone uniqueness
checkinguntil later in the scan than what the prior version did, and that kind of change could require more analysis
thanwe have time for at 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 portion of that patch, but I doubt it violates
thepost feature freeze rules.  In particular: 
>
>   +   BtreeLastVisibleEntry lVis = {InvalidBlockNumber, InvalidOffsetNumber, -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 preexisting confusion also.  There is already a
memberof the BtreeCheckState called "target" and a memory context in that struct called "targetcontext".  That context
isused to allocate pages "state->target", "rightpage", "child" and "page", but not "metapage".  Perhaps "targetcontext"
isa 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 *
>          *
>          * 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).
>          */
>         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
getsprocessed in the downlink check and thereafter.  This is either introducing a bug, or fixing one, but the commit
messageis totally ambiguous about whether this is a bugfix or a code cleanup or something else?  I think this kind of
patchshould 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 freeze

Ok.

------
Regards,
Alexander Korotkov
Supabase