Horton

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

Peter Geoghegan <pg@bowt.ie>

View thread
From: Peter Geoghegan <pg@bowt.ie>
To: Noah Misch <noah@leadboat.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Alexander Korotkov <aekorotkov@gmail.com>, 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>, Mark Dilger <mark.dilger@enterprisedb.com>, David Steele <david@pgmasters.net>, Maxim Orlov <m.orlov@postgrespro.ru>, lubennikovaav@gmail.com, Hamid Akhtar <hamid.akhtar@percona.com>
Date: 2024-03-29T18:17:08Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. amcheck: Optimize speed of checking for unique constraint violation

  2. amcheck: Report an error when the next page to a leaf is not a leaf

  3. amcheck: Don't load the right sibling page into BtreeCheckState

  4. amcheck: Refactoring the storage of the last visible entry

  5. Teach contrib/amcheck to check the unique constraint violation

  6. Add macros in hash and btree AMs to get the special area of their pages

On Mon, Mar 25, 2024 at 2:24 PM Noah Misch <noah@leadboat.com> wrote:
> I wasn't thinking about changing the pre-v17 bt_right_page_check_scankey()
> code.  I got interested in this area when I saw the interaction of the new
> "first key on the next page" logic with bt_right_page_check_scankey().  The
> patch made bt_right_page_check_scankey() pass back rightfirstoffset.  The new
> code then does palloc_btree_page() and PageGetItem() with that offset, which
> bt_right_page_check_scankey() had already done.  That smelled like a misplaced
> distribution of responsibility.  For a time, I suspected the new code should
> move down into bt_right_page_check_scankey().  Then I transitioned to thinking
> checkunique didn't need new code for the page boundary.

Ah, I see. Somehow I missed this point when I recently took a fresh
look at the committed patch.

 I did notice (I meant to point out) that I have concerns about this
part of the new uniqueness check code:

"
if (P_IGNORE(topaque) || !P_ISLEAF(topaque))
    break;
"

My concern here is with the !P_ISLEAF(topaque) test -- it shouldn't be
required. If the page in question isn't a leaf page, then the index
must be corrupt (or the page deletion recycle safety/drain technique
thing is buggy). The " !P_ISLEAF(topaque)" part of the check is either
superfluous or something that ought to be reported as corruption --
it's not a legal/expected state.

Separately, I dislike the way the target block changes within
bt_target_page_check(). The general idea behind verify_nbtree.c's
target block is that every block becomes the target exactly once, in a
clearly defined place. All corruption (in the index structure itself)
is formally considered to be a problem with that particular target
block. I want to be able to clearly distinguish between the target and
target's right sibling here, to explain my concerns, but they're kinda
both the target, so that's a lot harder than it should be. (Admittedly
directly blaming the target block has always been a little bit
arbitrary, at least in certain cases, but even there it provides
structure that makes things much easier to describe unambiguously.)

-- 
Peter Geoghegan