Amit Kapila <amit.kapil...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers