Amit Kapila <amit.kapila16@gmail.com> writes:
> Isn't the problem here is that due to some reason (like concurrent
> split), the code in question (loop --
> while (P_ISDELETED(opaque) || opaque->btpo_next != target)) releases
> the lock on target buffer and the caller again tries to release the
> lock on target buffer when false is returned.
Yeah. I do not think there is anything wrong with the loop-to-find-
current-leftsib logic. The problem is lack of care about what
resources are held on failure return. The sole caller thinks it
should do "_bt_relbuf(rel, buf)" --- that is, drop lock and pin on
what _bt_unlink_halfdead_page calls the leafbuf. But that function
already dropped the lock (line 1582 in 9.4), and won't have reacquired
it unless target != leafblkno. Another problem is that if the target
is different from leafblkno, we'll hold a pin on the target page, which
is leaked entirely in this code path.
The caller knows nothing of the "target" block so it can't reasonably
deal with all these cases. I'm inclined to change the function's API
spec to say that on failure return, it's responsible for dropping
lock and pin on the passed buffer. We could alternatively reacquire
lock before returning, but that seems pointless.
Another thing that looks fishy is at line 1611:
leftsib = opaque->btpo_prev;
At this point we already released lock on leafbuf so it seems pretty
unsafe to fetch its left-link. And it's unnecessary cause we already
did; the line should be just
leftsib = leafleftsib;
regards, tom lane
