On Wed, Jul 27, 2016 at 3:24 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Sat, Jul 23, 2016 at 3:55 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>> Attached patch fixes the problem for me. Note, I have not tried to
>> reproduce the problem for heap_lock_updated_tuple_rec(), but I think
>> if you are convinced with above cases, then we should have a similar
>> check in it as well.
>
> I don't think this hunk is correct:
>
> + /*
> + * If we didn't pin the visibility map page and the page has become
> + * all visible, we'll have to unlock and re-lock. See
> heap_lock_tuple
> + * for details.
> + */
> + if (vmbuffer == InvalidBuffer &&
> PageIsAllVisible(BufferGetPage(buf)))
> + {
> + LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> + visibilitymap_pin(rel, block, &vmbuffer);
> + LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
> + goto l4;
> + }
>
> The code beginning at label l4 appears that the buffer is unlocked,
> but this code leaves the buffer unlocked. Also, I don't see the point
> of doing this test so far down in the function. Why not just recheck
> *immediately* after taking the buffer lock?
>Right, in this case we can recheck immediately after taking buffer lock, updated patch attached. In the passing by, I have noticed that heap_delete() doesn't do this unlocking, pinning of vm and locking at appropriate place. It just checks immediately after taking lock, whereas in the down code, it do unlock and lock the buffer again. I think we should do it as in attached patch (pin_vm_heap_delete-v1.patch). -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
pin_vm_lock_tuple-v2.patch
Description: Binary data
pin_vm_heap_delete-v1.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
