Re: VACUUM can finish an interrupted nbtree page split -- is that okay?

On Fri, Mar 1, 2019 at 4:41 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > FWIW, I notice that the logic that appears after the
> > _bt_lock_branch_parent() call to _bt_getstackbuf() anticipates that it
> > must defend against interrupted splits in at least the
> > grandparent-of-leaf page, and maybe even the parent, so it's probably
> > not unworkable to not finish the split:
>
> -ETOOMANYNEGATIVES ... can't quite parse your point here?

Sorry. :-)

My point was that it actually seems feasible to not do the split,
making the quoted paragraph from nbtree README correct as-is. But,
since we're happy to continue to finish the occasional interrupted
internal page split within VACUUM anyway, that isn't an important
point.

> I think that code is there to deal with the possibility of finding
> an old half-dead page. Don't know that it's safe to remove it yet.

I don't know that it is either. My first instinct is to assume that
it's fine to remove the code, since, as I said, we're treating
internal pages that are half-dead as being ipso facto corrupt -- we'll
throw an error before too long anyway. However, "internal + half dead"
was a valid state for an nbtree page prior to 9.4, and we make no
distinction about that (versioning nbtree indexes to deal with
cross-version incompatibilities came only in Postgres v11). Trying to
analyze whether or not it's truly safe to just not do this seems very
difficult, and I don't think that it's performance critical. This is a
problem only because it's distracting and confusing.

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?

--
Peter Geoghegan