On Fri, Mar 1, 2019 at 3:59 PM Peter Geoghegan <p...@bowt.ie> wrote: > /* > * Perform the same check on this internal level that > * _bt_mark_page_halfdead performed on the leaf level. > */ > if (_bt_is_page_halfdead(rel, *rightsib))
> I thought that internal pages were never half-dead after Postgres 9.4. > If that happens, then the check within _bt_pagedel() will throw an > ERRCODE_INDEX_CORRUPTED error, and tell the DBA to REINDEX. Shouldn't > this internal level _bt_is_page_halfdead() check contain a "can't > happen" error, or even a simple assertion? I think that we should get rid of this code on HEAD shortly, because it's effectively dead code. You don't have to know anything about B-Trees to see why this must be true: VACUUM is specifically checking if an internal page is half-dead here, even though it's already treating half-dead internal pages as ipso facto corrupt in higher level code (it's the first thing we check in _bt_pagedel()). This is clearly contradictory. If there is a half-dead internal page, then there is no danger of VACUUM not complaining loudly that you need to REINDEX. This has been true since the page deletion overhaul that went into 9.4. Attached patch removes the internal page check, and adds a comment that explains why it's sufficient to check on the leaf level alone. Admittedly, it's much easier to understand why the _bt_is_page_halfdead() internal page check is useless than it is to understand why replacing it with this comment is helpful. My observation that a half-dead leaf page is representative of the subtree whose root the leaf page has stored as its "top parent" is hardly controversial, though -- that's the whole basis of multi-level page deletion. If you *visualize* how multi-level deletion works, and consider its rightmost-in-subtree restriction, then it isn't hard to see why everything works out with just the leaf level right-sibling-is-half-dead check: We can only have two adjacent "skinny" pending-deletion subtrees in cases where the removed check might seem to be helpful -- each page is both the leftmost and the rightmost on its level in its subtree. It's okay to just check if the leaf is half-dead because it "owns" exactly the same range in the keyspace as the internal pages up to and including its top parent, if any, and because it is marked half-dead by the same atomic operation that does initial removal of downlinks in an ancestor page. I'm fine with waiting until we branch-off v12 before pushing the patch, even though it seems low risk. -- Peter Geoghegan
0001-Remove-nbtree-half-dead-internal-page-check.patch
Description: Binary data