On Fri, Mar 1, 2019 at 5:00 PM Peter Geoghegan <pg@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
