On 02/04/2014 02:40 AM, Peter Geoghegan wrote:
> On Fri, Jan 31, 2014 at 9:09 AM, Heikki Linnakangas
> <hlinnakangas@vmware.com> wrote:
>> I refactored the loop in _bt_moveright to, well, not have that bug anymore.
>> The 'page' and 'opaque' pointers are now fetched at the beginning of the
>> loop. Did I miss something?
>
> I think so, yes. You still aren't assigning the value returned by
> _bt_getbuf() to 'buf'.
D'oh, you're right.
> Since, as I mentioned, _bt_finish_split() ultimately unlocks *and
> unpins*, it may not be the same buffer as before, so even with the
> refactoring there are race conditions.
Care to elaborate? Or are you just referring to the missing "buf = " ?
> A closely related issue is that you haven't mentioned anything about
> buffer pins/refcount side effects in comments above
> _bt_finish_split(), even though I believe you should.
Ok.
> A minor stylistic concern is that I think it would be better to only
> have one pair of _bt_finish_split()/_bt_getbuf() calls regardless of
> the initial value of 'access'.
Ok.
I also changed _bt_moveright to never return a write-locked buffer, when
the caller asked for a read-lock (an issue you pointed out earlier in
this thread).
Attached is a new version of the patch, with those issues fixed.
btree-incomplete-split-4.patch is a complete patch against the latest
fix-btree-page-deletion patch, and moveright-assign-fix.patch is just
the changes to _bt_moveright, if you want to review just the changes
since the previous patch I posted.
- Heikki
