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

On Mon, Mar 25, 2024 at 2:24 PM Noah Misch <noah(at)leadboat(dot)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