Department of second thoughts: remove PD_ALL_FROZEN.

Commit a892234f830e832110f63fc0a2afce2fb21d1584 added a second bit per
page to the visibility map, which still seems like a good idea, but it
also added a second page-level bit alongside PD_ALL_VISIBLE to track
whether the visibility map bit was set.  That no longer seems like a
clever plan, because we don't really need that bit for anything.  We
always clear both bits when the page is modified anyway.

Patch by me, reviewed by Kyotaro Horiguchi and Masahiko Sawada.

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 8a64321fe493adad6587f6087202955d2fc166b7..34ba385748f2d840df62e325b9d9cee55e681fed 100644 (file)
--- 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 2e64fc3dfe8c300f72b4b6db59f6aa8b80afb219..eaab4beccbc586f57819458275bcd491972ae536 100644 (file)
--- 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 8f7b2486e003311cce47385ace5b29692020197a..363b2d0d10ce7a00b132c2e91e8bf7e847f1dd97 100644 (file)
--- 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 0b023b3d85328ec752aa0588cabb774460454f2f..2ce3be765c0154699e1307e9d0b3a98be2ae9fc4 100644 (file)
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -178,10 +178,8 @@ 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 */
+#define PD_VALID_FLAG_BITS     0x0007          /* OR of all valid pd_flags bits */
 
 /*
  * Page layout version number 0 is for pre-7.3 Postgres releases.
@@ -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) \
 ( \