Re: Reviewing freeze map code

On 2016-07-05 23:37:59 +0900, Masahiko Sawada wrote:
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index 57da57a..fd66527 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -3923,6 +3923,17 @@ l2:
>  
>      if (need_toast || newtupsize > pagefree)
>      {
> +        /*
> +         * To prevent data corruption due to updating old tuple by
> +         * other backends after released buffer

That's not really the reason, is it? The prime problem is crash safety /
replication. The row-lock we're faking (by setting xmax to our xid),
prevents concurrent updates until this backend died.

>                 , we need to emit that
> +         * xmax of old tuple is set and clear visibility map bits if
> +         * needed before releasing buffer. We can reuse xl_heap_lock
> +         * for this purpose. It should be fine even if we crash midway
> +         * from this section and the actual updating one later, since
> +         * the xmax will appear to come from an aborted xid.
> +         */
> +        START_CRIT_SECTION();
> +
>          /* Clear obsolete visibility flags ... */
>          oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
>          oldtup.t_data->t_infomask2 &= ~HEAP_KEYS_UPDATED;
> @@ -3936,6 +3947,46 @@ l2:
>          /* temporarily make it look not-updated */
>          oldtup.t_data->t_ctid = oldtup.t_self;
>          already_marked = true;
> +
> +        /* Clear PD_ALL_VISIBLE flags */
> +        if (PageIsAllVisible(BufferGetPage(buffer)))
> +        {
> +            all_visible_cleared = true;
> +            PageClearAllVisible(BufferGetPage(buffer));
> +            visibilitymap_clear(relation, BufferGetBlockNumber(buffer),
> +                                vmbuffer);
> +        }
> +
> +        MarkBufferDirty(buffer);
> +
> +        if (RelationNeedsWAL(relation))
> +        {
> +            xl_heap_lock xlrec;
> +            XLogRecPtr recptr;
> +
> +            /*
> +             * For logical decoding we need combocids to properly decode the
> +             * catalog.
> +             */
> +            if (RelationIsAccessibleInLogicalDecoding(relation))
> +                log_heap_new_cid(relation, &oldtup);

Hm, I don't see that being necessary here. Row locks aren't logically
decoded, so there's no need to emit this here.


> +    /* Clear PD_ALL_VISIBLE flags */
> +    if (PageIsAllVisible(page))
> +    {
> +        Buffer    vmbuffer = InvalidBuffer;
> +        BlockNumber    block = BufferGetBlockNumber(*buffer);
> +
> +        all_visible_cleared = true;
> +        PageClearAllVisible(page);
> +        visibilitymap_pin(relation, block, &vmbuffer);
> +        visibilitymap_clear(relation, block, vmbuffer);
> +    }
> +

That clears all-visible unnecessarily, we only need to clear all-frozen.



> @@ -8694,6 +8761,23 @@ heap_xlog_lock(XLogReaderState *record)
>          }
>          HeapTupleHeaderSetXmax(htup, xlrec->locking_xid);
>          HeapTupleHeaderSetCmax(htup, FirstCommandId, false);
> +
> +        /* The visibility map need to be cleared */
> +        if ((xlrec->infobits_set & XLHL_ALL_VISIBLE_CLEARED) != 0)
> +        {
> +            RelFileNode    rnode;
> +            Buffer        vmbuffer = InvalidBuffer;
> +            BlockNumber    blkno;
> +            Relation    reln;
> +
> +            XLogRecGetBlockTag(record, 0, &rnode, NULL, &blkno);
> +            reln = CreateFakeRelcacheEntry(rnode);
> +
> +            visibilitymap_pin(reln, blkno, &vmbuffer);
> +            visibilitymap_clear(reln, blkno, vmbuffer);
> +            PageClearAllVisible(page);
> +        }
> +


>          PageSetLSN(page, lsn);
>          MarkBufferDirty(buffer);
>      }
> diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
> index a822d0b..41b3c54 100644
> --- a/src/include/access/heapam_xlog.h
> +++ b/src/include/access/heapam_xlog.h
> @@ -242,6 +242,7 @@ typedef struct xl_heap_cleanup_info
>  #define XLHL_XMAX_EXCL_LOCK        0x04
>  #define XLHL_XMAX_KEYSHR_LOCK    0x08
>  #define XLHL_KEYS_UPDATED        0x10
> +#define XLHL_ALL_VISIBLE_CLEARED 0x20

Hm. We can't easily do that in the back-patched version; because a
standby won't know to check for the flag . That's kinda ok, since we
don't yet need to clear all-visible yet at that point of
heap_update. But that better means we don't do so on the master either.

Greetings,

Andres Freund