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

On Fri, 10 May 2024, 22:42 Pavel Borisov, <pashkin.elfe@gmail.com> wrote:

> Hi, Mark!
>
>
> On Fri, 10 May 2024, 21:35 Mark Dilger, <mark.dilger@enterprisedb.com>
> wrote:
>
>>
>>
>> > On May 10, 2024, at 5:10 AM, Pavel Borisov <pashkin.elfe@gmail.com>
>> wrote:
>> >
>> > Hi, Alexander!
>> >
>> > 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 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
>>
>
> 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.
>
> For 0002 I doubt I understand your actual positiob. Could you explain what
> it violates or doesn't violate?
>
Please forgive many typos in the previous message, I wrote from phone.

Pavel.

>