Re: Reviewing freeze map code

Hi,

So I'm generally happy with 0001, baring some relatively minor
adjustments. I am however wondering about one thing:

On 2016-07-11 23:51:05 +0900, Masahiko Sawada wrote:
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index 57da57a..e7cb8ca 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -3923,6 +3923,16 @@ l2:
>
> if (need_toast || newtupsize > pagefree)
> {
> + /*
> + * For crash safety, we need to emit that xmax of old tuple is set
> + * and clear only the all-frozen bit on visibility map if needed
> + * before releasing the 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 +3946,28 @@ l2:
> /* temporarily make it look not-updated */
> oldtup.t_data->t_ctid = oldtup.t_self;
> already_marked = true;
> +
> + MarkBufferDirty(buffer);
> +
> + if (RelationNeedsWAL(relation))
> + {
> + xl_heap_lock xlrec;
> + XLogRecPtr recptr;
> +
> + XLogBeginInsert();
> + XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
> +
> + xlrec.offnum = ItemPointerGetOffsetNumber(&oldtup.t_self);
> + xlrec.locking_xid = xmax_old_tuple;
> + xlrec.infobits_set = compute_infobits(oldtup.t_data->t_infomask,
> + oldtup.t_data->t_infomask2);
> + XLogRegisterData((char *) &xlrec, SizeOfHeapLock);
> + recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_LOCK);
> + PageSetLSN(page, recptr);
> + }

Master does
/* temporarily make it look not-updated */
oldtup.t_data->t_ctid = oldtup.t_self;
here, and as is the wal record won't reflect that, because:
static void
heap_xlog_lock(XLogReaderState *record)
{
...
/*
* Clear relevant update flags, but only if the modified infomask says
* there's no update.
*/
if (HEAP_XMAX_IS_LOCKED_ONLY(htup->t_infomask))
{
HeapTupleHeaderClearHotUpdated(htup);
/* Make sure there is no forward chain link in t_ctid */
ItemPointerSet(&htup->t_ctid,
BufferGetBlockNumber(buffer),
offnum);
}
won't enter the branch, because HEAP_XMAX_LOCK_ONLY won't be set. Which
will leave t_ctid and HEAP_HOT_UPDATED set differently on the master and
standby / after crash recovery. I'm failing to see any harmful
consequences right now, but differences between master and standby are a bad
thing. Pre 9.3 that's not a problem, we reset ctid and HOT_UPDATED
unconditionally there. I think I'm more comfortable with setting
HEAP_XMAX_LOCK_ONLY until the tuple is finally updated - that also
coincides more closely with the actual meaning.

Any arguments against?

>
> + /* Clear only the all-frozen bit on visibility map if needed */
> + if (PageIsAllVisible(BufferGetPage(buffer)) &&
> + VM_ALL_FROZEN(relation, block, &vmbuffer))
> + {
> + visibilitymap_clear_extended(relation, block, vmbuffer,
> + VISIBILITYMAP_ALL_FROZEN);
> + }
> +

FWIW, I don't think it's worth introducing visibilitymap_clear_extended.
As this is a 9.6 only patch, i think it's better to change
visibilitymap_clear's API.

Unless somebody protests I'm planning to commit with those adjustments
tomorrow.

Greetings,

Andres Freund