Re: Failure while inserting parent tuple to B-tree is not fun

Some more thoughts:

Please add comments above _bt_mark_page_halfdead(), a new routine from
the dependency patch. I realize that this is substantially similar to
part of how _bt_pagedel() used to work, but it's still incongruous.

> ! Our approach is to create any missing downlinks on-they-fly, when
> ! searching the tree for a new insertion. It could be done during searches,
> ! too, but it seems best not to put any extra updates in what would otherwise

s/on-they-fly/on-the-fly/

I'm not sure about this:

> *************** _bt_findinsertloc(Relation rel,
> *** 675,680 ****
> --- 701,707 ----
> static void
> _bt_insertonpg(Relation rel,
> Buffer buf,
> + Buffer cbuf,
> BTStack stack,
> IndexTuple itup,
> OffsetNumber newitemoff,

Wouldn't lcbuf be a better name for the new argument? After all, in
relevant contexts 'buf' is the parent of both of the pages post-split,
but there are two children in play here. You say:

>+ * When inserting to a non-leaf page, 'cbuf' is the left-sibling of the
>+ * page we're inserting the downlink for. This function will clear the
>+ * INCOMPLETE_SPLIT flag on it, and release the buffer.

I suggest also noting in comments at this point that cbuf/the
left-sibling is the "old half" from the split.

Even having vented about cbuf's name, I can kind of see why you didn't
call it lcbuf: we already have an "BTPageOpaque lpageop" variable for
the special 'buf' metadata within the _bt_insertonpg() function; so
there might be an ambiguity between the possibly-soon-to-be-left page
(the target page) and the *child* left page that needs to have its
flag cleared. Although I still think you should change the name of
"lpageop" (further details follow).

Consider this:

> /* release buffers */
>+ if (!P_ISLEAF(lpageop))
>+ _bt_relbuf(rel, cbuf);

(Rebased, so this looks a bit different to your original version IIRC).

This is checking that the _bt_insertonpg() target page (whose special
data lpageop points to) is not a leaf page, and releasing the *child*
left page if it isn't. This is pretty confusing. So I suggest naming
"lpageop" "tpageop" ('t' for target, a terminology already used in the
comments above the function).

Also, I suggest you change this existing code comment:

* On entry, we must have the right buffer in which to do the
* insertion, and the buffer must be pinned and write-locked. On return,
* we will have dropped both the pin and the lock on the buffer.

Change "right" to "correct" here. Minor point, but "right" is a loaded word.

But why are you doing new things while checking P_ISLEAF(lpageop) in
_bt_insertonpg() to begin with? Would you not be better off doing
things when there is a child buffer passed (e.g. "if
(BufferIsValid(cbuf))..."), and only then asserting
!P_ISLEAF(lpageop)? That seems to me to more naturally establish the
context of "_bt_insertonpg() is called to insert downlink after
split". Although maybe that conflates things within "XLOG stuff". Hmm.

Perhaps some of these observations could equally well apply to
_bt_split(), which is similarly charged with releasing a left child
page on inserting a downlink. Particularly around reconsidering the
"left vs left child vs target" terminology.

Consider what happens when _bt_split() is passed a (left) child buffer
(a 'cbuf' argument). We are:

1. Splitting a page (first phase, which may only be finished lazily
some time later).

2. Iff this is a non-leaf page split, simultaneously unmark the flag
from some *other* split which we have a child buffer to unmark. Needs
to be part of same critical section. Unlock + release cbuf, again only
iff target/right split page contains a leaf page.

So, at the risk of belaboring the point:

* Some callers to _bt_split() and _bt_insertonpg() pass a 'cbuf' that
is not invalid.

* If either of those 2 functions don't unlock + release those buffers,
no one ever will.

* Therefore, at the very least we should unlock + release those
buffers **just because they're not invalid**. That's a sufficient
condition to do so, and attaching that to "level" is unnecessarily
unclear. As I said, assert non-leafness.

Incidentally, I think that in general we could be clearer on the fact
that a root page may also be a leaf page.
--
Peter Geoghegan