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

From: Alexander Korotkov <[email-masked]>
Date: Fri, 10 May 2024 03:11:41 +0300

On Wed, May 1, 2024 at 5:26 AM Alexander Korotkov <[email-masked]> wrote:
> On Wed, May 1, 2024 at 5:24 AM Noah Misch <[email-masked]> wrote:
> > On Thu, Apr 25, 2024 at 04:59:54PM +0400, Pavel Borisov wrote:
> > > 0001: Optimize speed by avoiding heap visibility checking for different
> > > non-deduplicated index tuples as proposed by Noah Misch
> > >
> > > Speed measurements on my laptop using the exact method recommended by Noah
> > > upthread:
> > > Current master branch: checkunique off: 144s, checkunique on: 419s
> > > With patch 0001: checkunique off: 141s, checkunique on: 171s
> >
> > Where is the CPU time going to make it still be 21% slower w/ checkunique on?
> > It's a great improvement vs. current master, but I don't have an obvious
> > explanation for the remaining +21%.
>
> I think there is at least extra index tuples comparison.

The revised patchset is attached.  I applied cosmetical changes.  I'm
going to push it if no objections.

I don't post the patch with rename of new option.  It doesn't seem
there is a consensus.  I must admit that keeping all the options in
the same naming convention makes sense.

------
Regards,
Alexander Korotkov
Supabase

---

From: Tom Lane <[email-masked]>
Date: Thu, 09 May 2024 20:42:55 -0400

> 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.

			regards, tom lane

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 10 May 2024 11:34:25 +0400

Hi, Tom!

> It doesn't look like it's fixing any new-in-v17 issues.

I think these patches are nice-to-have optimizations and refactorings to
make code look better. They are not necessary for the main feature. They
don't fix any bugs. But they were requested in the thread, and make sense
in my opinion.

I really don't know what's the policy of applying code improvements other
than bugfixes post feature-freeze. IMO they are safe to be appiled to v17,
but they also could be added later.

Regards,
Pavel Borisov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Fri, 10 May 2024 11:39:18 +0300

> 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?

------
Regards,
Alexander Korotkov
Supabase

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 10 May 2024 16:10:04 +0400

Hi, Alexander!

> 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.

Regards,
Pavel.

---

From: Mark Dilger <[email-masked]>
Date: Fri, 10 May 2024 10:35:13 -0700

> On May 10, 2024, at 5:10 AM, Pavel Borisov <[email-masked]> wrote:
> 
> Hi, Alexander!
> 
> On Fri, 10 May 2024 at 12:39, Alexander Korotkov <[email-masked]> wrote:
> 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

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 10 May 2024 22:42:01 +0400

Hi, Mark!

On Fri, 10 May 2024, 21:35 Mark Dilger, <[email-masked]>
wrote:

> 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:
>
> 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 *
>          *
> 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.
>
>

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?

Best regards,
Pavel.

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 10 May 2024 22:43:46 +0400

On Fri, 10 May 2024, 22:42 Pavel Borisov, <[email-masked]> wrote:

> 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.
>
Please forgive many typos in the previous message, I wrote from phone.

Pavel.

---

From: Mark Dilger <[email-masked]>
Date: Fri, 10 May 2024 12:27:11 -0700

> On May 10, 2024, at 11:42 AM, Pavel Borisov <[email-masked]> wrote:
> 
> 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.

Well, you can put an Assert(false) dead in the middle of the code we're discussing and all the regression tests still pass, so I'd argue the change is getting zero test coverage.

This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did.  That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch.  Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used.  So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else?  Is this a bug being introduced, being fixed, or ... ?

Having a regression test that actually touches this code would go a fair way towards helping the analysis.

> For 0002 I doubt I understand your actual positiob. Could you explain what it violates or doesn't violate?

v2-0002 is does not violate the post feature freeze restriction on new features so far as I can tell, but I just don't care for the variable initialization because it doesn't name the fields.  If anybody refactored the struct they might not notice that the need to reorder this initialization, and depending on various factors including compiler flags they might not get an error.

—
Mark Dilger