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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers