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
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
