On Fri, Mar 29, 2024 at 02:17:08PM -0400, Peter Geoghegan wrote: > On Mon, Mar 25, 2024 at 2:24 PM Noah Misch <n...@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.
> 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. Good point. > 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. Agreed.
