Hello, At Sat, 06 Aug 2016 12:45:32 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote in <16748.1470501...@sss.pgh.pa.us> > 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.
Agreed. > 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; Right. And I found that it is already committed. Thanks. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
