Re: Possible duplicate release of buffer lock. - Mailing list pgsql-hackers

From Kyotaro HORIGUCHI
Subject Re: Possible duplicate release of buffer lock.
Date
Msg-id 20160808.163820.39642563.horiguchi.kyotaro@lab.ntt.co.jp
Whole thread Raw
In response to Re: Possible duplicate release of buffer lock.  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-hackers
Hello,

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

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





pgsql-hackers by date:

Previous
From: Victor Wagner
Date:
Subject: Re: handling unconvertible error messages
Next
From: Vladimir Sitnikov
Date:
Subject: Re: No longer possible to query catalogs for index capabilities?