Re: Reviewing freeze map code

On Wed, Jul 27, 2016 at 3:24 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Jul 23, 2016 at 3:55 AM, Amit Kapila <amit.kapila16@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