Re: Reviewing freeze map code

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