Re: Reviewing freeze map code

On Wed, Jun 15, 2016 at 7:13 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Jun 15, 2016 at 9:43 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Wed, Jun 15, 2016 at 6:26 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
> >> On Wed, Jun 15, 2016 at 2:41 AM, Thomas Munro
> >> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> >> > I spent some time chasing down the exact circumstances. I suspect
> >> > that there may be an interlocking problem in heap_update. Using the
> >> > line numbers from cae1c788 [1], I see the following interaction
> >> > between the VACUUM, UPDATE and SELECT (pg_check_visible) backends,
all
> >> > in reference to the same block number:
> >> >
> >> > [VACUUM] sets all visible bit
> >> >
> >> > [UPDATE] heapam.c:3931 HeapTupleHeaderSetXmax(oldtup.t_data,
> >> > xmax_old_tuple);
> >> > [UPDATE] heapam.c:3938 LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> >> >
> >> > [SELECT] LockBuffer(buffer, BUFFER_LOCK_SHARE);
> >> > [SELECT] observes VM_ALL_VISIBLE as true
> >> > [SELECT] observes tuple in HEAPTUPLE_DELETE_IN_PROGRESS state
> >> > [SELECT] barfs
> >> >
> >> > [UPDATE] heapam.c:4116 visibilitymap_clear(...)
> >>
> >> Yikes: heap_update() sets the tuple's XMAX, CMAX, infomask, infomask2,
> >> and CTID without logging anything or clearing the all-visible flag and
> >> then releases the lock on the heap page to go do some more work that
> >> might even ERROR out.
> >
> > Can't we clear the all-visible flag before releasing the lock? We can
use
> > logic of already_marked as it is currently used in code to clear it just
> > once.
>
> That just kicks the can down the road. Then you have PD_ALL_VISIBLE
> clear but the VM bit is still set.

I mean to say clear both as we are doing currently in code:
if (PageIsAllVisible(BufferGetPage(buffer)))
{
all_visible_cleared = true;
PageClearAllVisible(BufferGetPage(buffer));
visibilitymap_clear(relation, BufferGetBlockNumber(buffer),
vmbuffer);
}

>
> And you still haven't WAL-logged
> anything.
>

Yeah, I think WAL requirement is more difficult to meet and I think
releasing the lock on buffer before writing WAL could lead to flush of such
a buffer before WAL.

I feel this is an existing-bug and should go to Older Bugs Section in open
items page.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com