On Mon, Mar 7, 2016 at 12:41 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote:
> Attached latest version optimisation patch.
> I'm still consider regarding pg_upgrade regression test code, so I
> will submit that patch later.
I was thinking more about this today and I think that we don't
actually need the PD_ALL_FROZEN page-level bit for anything. It's
enough that the bit is present in the visibility map. The only point
of PD_ALL_VISIBLE is that it tells us that we need to clear the
visibility map bit, but that bit is enough to tell us to clear both
visibility map bits. So I propose the attached cleanup patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 8a64321..34ba385 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -7855,10 +7855,7 @@ heap_xlog_visible(XLogReaderState *record)
*/
page = BufferGetPage(buffer);
- if (xlrec->flags & VISIBILITYMAP_ALL_VISIBLE)
- PageSetAllVisible(page);
- if (xlrec->flags & VISIBILITYMAP_ALL_FROZEN)
- PageSetAllFrozen(page);
+ PageSetAllVisible(page);
MarkBufferDirty(buffer);
}
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 2e64fc3..eaab4be 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -39,15 +39,15 @@
*
* When we *set* a visibility map during VACUUM, we must write WAL. This may
* seem counterintuitive, since the bit is basically a hint: if it is clear,
- * it may still be the case that every tuple on the page is all-visible or
- * all-frozen we just don't know that for certain. The difficulty is that
- * there are two bits which are typically set together: the PD_ALL_VISIBLE
- * or PD_ALL_FROZEN bit on the page itself, and the corresponding visibility
- * map bit. If a crash occurs after the visibility map page makes it to disk
- * and before the updated heap page makes it to disk, redo must set the bit on
- * the heap page. Otherwise, the next insert, update, or delete on the heap
- * page will fail to realize that the visibility map bit must be cleared,
- * possibly causing index-only scans to return wrong answers.
+ * it may still be the case that every tuple on the page is visible to all
+ * transactions; we just don't know that for certain. The difficulty is that
+ * there are two bits which are typically set together: the PD_ALL_VISIBLE bit
+ * on the page itself, and the visibility map bit. If a crash occurs after the
+ * visibility map page makes it to disk and before the updated heap page makes
+ * it to disk, redo must set the bit on the heap page. Otherwise, the next
+ * insert, update, or delete on the heap page will fail to realize that the
+ * visibility map bit must be cleared, possibly causing index-only scans to
+ * return wrong answers.
*
* VACUUM will normally skip pages for which the visibility map bit is set;
* such pages can't contain any dead tuples and therefore don't need vacuuming.
@@ -251,11 +251,10 @@ visibilitymap_pin_ok(BlockNumber heapBlk, Buffer buf)
* to InvalidTransactionId when a page that is already all-visible is being
* marked all-frozen.
*
- * Caller is expected to set the heap page's PD_ALL_VISIBLE or PD_ALL_FROZEN
- * bit before calling this function. Except in recovery, caller should also
- * pass the heap buffer and flags which indicates what flag we want to set.
- * When checksums are enabled and we're not in recovery, we must add the heap
- * buffer to the WAL chain to protect it from being torn.
+ * Caller is expected to set the heap page's PD_ALL_VISIBLE bit before calling
+ * this function. Except in recovery, caller should also pass the heap
+ * buffer. When checksums are enabled and we're not in recovery, we must add
+ * the heap buffer to the WAL chain to protect it from being torn.
*
* You must pass a buffer containing the correct map page to this function.
* Call visibilitymap_pin first to pin the right one. This function doesn't do
@@ -315,10 +314,8 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
{
Page heapPage = BufferGetPage(heapBuf);
- /* Caller is expected to set page-level bits first. */
- Assert((flags & VISIBILITYMAP_ALL_VISIBLE) == 0 || PageIsAllVisible(heapPage));
- Assert((flags & VISIBILITYMAP_ALL_FROZEN) == 0 || PageIsAllFrozen(heapPage));
-
+ /* caller is expected to set PD_ALL_VISIBLE first */
+ Assert(PageIsAllVisible(heapPage));
PageSetLSN(heapPage, recptr);
}
}
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 8f7b248..363b2d0 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -766,7 +766,6 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
log_newpage_buffer(buf, true);
PageSetAllVisible(page);
- PageSetAllFrozen(page);
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
vmbuffer, InvalidTransactionId,
VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
@@ -1024,6 +1023,9 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
{
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
+ if (all_frozen)
+ flags |= VISIBILITYMAP_ALL_FROZEN;
+
/*
* It should never be the case that the visibility map page is set
* while the page-level bit is clear, but the reverse is allowed
@@ -1038,11 +1040,6 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
* rare cases after a crash, it is not worth optimizing.
*/
PageSetAllVisible(page);
- if (all_frozen)
- {
- PageSetAllFrozen(page);
- flags |= VISIBILITYMAP_ALL_FROZEN;
- }
MarkBufferDirty(buf);
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
vmbuffer, visibility_cutoff_xid, flags);
@@ -1093,10 +1090,6 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
else if (all_visible_according_to_vm && all_visible && all_frozen &&
!VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
{
- /* Page is marked all-visible but should be all-frozen */
- PageSetAllFrozen(page);
- MarkBufferDirty(buf);
-
/*
* We can pass InvalidTransactionId as the cutoff XID here,
* because setting the all-frozen bit doesn't cause recovery
@@ -1344,11 +1337,7 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
*/
if (heap_page_is_all_visible(onerel, buffer, &visibility_cutoff_xid,
&all_frozen))
- {
PageSetAllVisible(page);
- if (all_frozen)
- PageSetAllFrozen(page);
- }
/*
* All the changes to the heap page have been done. If the all-visible
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 0b023b3..d930166 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -178,8 +178,6 @@ typedef PageHeaderData *PageHeader;
* tuple? */
#define PD_ALL_VISIBLE 0x0004 /* all tuples on page are visible to
* everyone */
-#define PD_ALL_FROZEN 0x0008 /* all tuples on page are completely
- frozen */
#define PD_VALID_FLAG_BITS 0x000F /* OR of all valid pd_flags bits */
@@ -369,12 +367,7 @@ typedef PageHeaderData *PageHeader;
#define PageSetAllVisible(page) \
(((PageHeader) (page))->pd_flags |= PD_ALL_VISIBLE)
#define PageClearAllVisible(page) \
- (((PageHeader) (page))->pd_flags &= ~(PD_ALL_VISIBLE | PD_ALL_FROZEN))
-
-#define PageIsAllFrozen(page) \
- (((PageHeader) (page))->pd_flags & PD_ALL_FROZEN)
-#define PageSetAllFrozen(page) \
- (((PageHeader) (page))->pd_flags |= PD_ALL_FROZEN)
+ (((PageHeader) (page))->pd_flags &= ~PD_ALL_VISIBLE)
#define PageIsPrunable(page, oldestxmin) \
( \
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
