At Sat, 06 Aug 2016 12:45:32 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote in 
> 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;

Right. And I found that it is already committed. Thanks.


Kyotaro Horiguchi
NTT Open Source Software Center

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to