Re: Possible duplicate release of buffer lock.

Hello,

At Sat, 06 Aug 2016 12:45:32 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in <16748(dot)1470501932(at)sss(dot)pgh(dot)pa(dot)us>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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