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

> On May 10, 2024, at 5:10 AM, Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com> wrote:
>
> Hi, Alexander!
>
> On Fri, 10 May 2024 at 12:39, Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> On Fri, May 10, 2024 at 3:43 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Alexander Korotkov <aekorotkov(at)gmail(dot)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 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 = {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 *
*
* 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 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

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company