On Fri, Mar 1, 2019 at 5:00 PM Peter Geoghegan <p...@bowt.ie> wrote: > I favor keeping the test, but having it throw a > ERRCODE_INDEX_CORRUPTED error, just like _bt_pagedel() does already. A > comment could point out that the test is historical/defensive, and > probably isn't actually necessary. What do you think of that idea?
Actually, while 9.4 did indeed start treating "internal + half dead" pages as corrupt, it didn't exactly remove the *concept* of a half dead internal page. I think that the cross check (the one referenced by comments above the corresponding leaf/_bt_mark_page_halfdead() call to _bt_is_page_halfdead()) might have problems in the event of an interrupted *multi-level* page deletion. I wonder, is there a subtle bug here that bugfix commit 8da31837803 didn't quite manage to prevent? (This commit added both of the _bt_is_page_halfdead() checks.) (Thinks some more...) Actually, I think that bugfix commit 8da31837803 works despite possible "logically half dead internal pages", because in the event of such an internal page the sibling would actually have to be the *cousin* of the original parent (half dead/leaf page parent), not the "true sibling" (otherwise, cousin's multi-level page deletion should never have taken place). I think that we'll end up doing the right thing with the downlinks in the grandparent page, despite there being an interrupted multi-level deletion in the cousin's subtree. Since cousin *atomically* removed its downlink in our shared *grandparent* (not its parent) at the same time some leaf page was initially marked half-dead, everything works out. Page deletion is painfully complicated. Seems wise to keep the internal page test, out of sheer paranoia, while making it an error as suggested earlier. I will definitely want to think about it some more, though. -- Peter Geoghegan
