Skip to content

Commit 77a1d1e

Browse files
robertmhaasrobertmhaas
committed
Department of second thoughts: remove PD_ALL_FROZEN.
Commit a892234 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.
1 parent ba0a198 commit 77a1d1e

File tree

4 files changed

+21
-45
lines changed

4 files changed

+21
-45
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7855,10 +7855,7 @@ heap_xlog_visible(XLogReaderState *record)
78557855
*/
78567856
page = BufferGetPage(buffer);
78577857

7858-
if (xlrec->flags & VISIBILITYMAP_ALL_VISIBLE)
7859-
PageSetAllVisible(page);
7860-
if (xlrec->flags & VISIBILITYMAP_ALL_FROZEN)
7861-
PageSetAllFrozen(page);
7858+
PageSetAllVisible(page);
78627859

78637860
MarkBufferDirty(buffer);
78647861
}

‎src/backend/access/heap/visibilitymap.c

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,15 @@
3939
*
4040
* When we *set* a visibility map during VACUUM, we must write WAL. This may
4141
* seem counterintuitive, since the bit is basically a hint: if it is clear,
42-
* it may still be the case that every tuple on the page is all-visible or
43-
* all-frozen we just don't know that for certain. The difficulty is that
44-
* there are two bits which are typically set together: the PD_ALL_VISIBLE
45-
* or PD_ALL_FROZEN bit on the page itself, and the corresponding visibility
46-
* map bit. If a crash occurs after the visibility map page makes it to disk
47-
* and before the updated heap page makes it to disk, redo must set the bit on
48-
* the heap page. Otherwise, the next insert, update, or delete on the heap
49-
* page will fail to realize that the visibility map bit must be cleared,
50-
* possibly causing index-only scans to return wrong answers.
42+
* it may still be the case that every tuple on the page is visible to all
43+
* transactions; we just don't know that for certain. The difficulty is that
44+
* there are two bits which are typically set together: the PD_ALL_VISIBLE bit
45+
* on the page itself, and the visibility map bit. If a crash occurs after the
46+
* visibility map page makes it to disk and before the updated heap page makes
47+
* it to disk, redo must set the bit on the heap page. Otherwise, the next
48+
* insert, update, or delete on the heap page will fail to realize that the
49+
* visibility map bit must be cleared, possibly causing index-only scans to
50+
* return wrong answers.
5151
*
5252
* VACUUM will normally skip pages for which the visibility map bit is set;
5353
* 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)
251251
* to InvalidTransactionId when a page that is already all-visible is being
252252
* marked all-frozen.
253253
*
254-
* Caller is expected to set the heap page's PD_ALL_VISIBLE or PD_ALL_FROZEN
255-
* bit before calling this function. Except in recovery, caller should also
256-
* pass the heap buffer and flags which indicates what flag we want to set.
257-
* When checksums are enabled and we're not in recovery, we must add the heap
258-
* buffer to the WAL chain to protect it from being torn.
254+
* Caller is expected to set the heap page's PD_ALL_VISIBLE bit before calling
255+
* this function. Except in recovery, caller should also pass the heap
256+
* buffer. When checksums are enabled and we're not in recovery, we must add
257+
* the heap buffer to the WAL chain to protect it from being torn.
259258
*
260259
* You must pass a buffer containing the correct map page to this function.
261260
* 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,
315314
{
316315
Page heapPage = BufferGetPage(heapBuf);
317316

318-
/* Caller is expected to set page-level bits first. */
319-
Assert((flags & VISIBILITYMAP_ALL_VISIBLE) == 0 || PageIsAllVisible(heapPage));
320-
Assert((flags & VISIBILITYMAP_ALL_FROZEN) == 0 || PageIsAllFrozen(heapPage));
321-
317+
/* caller is expected to set PD_ALL_VISIBLE first */
318+
Assert(PageIsAllVisible(heapPage));
322319
PageSetLSN(heapPage, recptr);
323320
}
324321
}

‎src/backend/commands/vacuumlazy.c

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -766,7 +766,6 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
766766
log_newpage_buffer(buf, true);
767767

768768
PageSetAllVisible(page);
769-
PageSetAllFrozen(page);
770769
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
771770
vmbuffer, InvalidTransactionId,
772771
VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
@@ -1024,6 +1023,9 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
10241023
{
10251024
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
10261025

1026+
if (all_frozen)
1027+
flags |= VISIBILITYMAP_ALL_FROZEN;
1028+
10271029
/*
10281030
* It should never be the case that the visibility map page is set
10291031
* while the page-level bit is clear, but the reverse is allowed
@@ -1038,11 +1040,6 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
10381040
* rare cases after a crash, it is not worth optimizing.
10391041
*/
10401042
PageSetAllVisible(page);
1041-
if (all_frozen)
1042-
{
1043-
PageSetAllFrozen(page);
1044-
flags |= VISIBILITYMAP_ALL_FROZEN;
1045-
}
10461043
MarkBufferDirty(buf);
10471044
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
10481045
vmbuffer, visibility_cutoff_xid, flags);
@@ -1093,10 +1090,6 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
10931090
else if (all_visible_according_to_vm && all_visible && all_frozen &&
10941091
!VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
10951092
{
1096-
/* Page is marked all-visible but should be all-frozen */
1097-
PageSetAllFrozen(page);
1098-
MarkBufferDirty(buf);
1099-
11001093
/*
11011094
* We can pass InvalidTransactionId as the cutoff XID here,
11021095
* because setting the all-frozen bit doesn't cause recovery
@@ -1344,11 +1337,7 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
13441337
*/
13451338
if (heap_page_is_all_visible(onerel, buffer, &visibility_cutoff_xid,
13461339
&all_frozen))
1347-
{
13481340
PageSetAllVisible(page);
1349-
if (all_frozen)
1350-
PageSetAllFrozen(page);
1351-
}
13521341

13531342
/*
13541343
* All the changes to the heap page have been done. If the all-visible

‎src/include/storage/bufpage.h

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -178,10 +178,8 @@ typedef PageHeaderData *PageHeader;
178178
* tuple? */
179179
#define PD_ALL_VISIBLE 0x0004 /* all tuples on page are visible to
180180
* everyone */
181-
#define PD_ALL_FROZEN 0x0008 /* all tuples on page are completely
182-
frozen */
183181

184-
#define PD_VALID_FLAG_BITS 0x000F /* OR of all valid pd_flags bits */
182+
#define PD_VALID_FLAG_BITS 0x0007 /* OR of all valid pd_flags bits */
185183

186184
/*
187185
* Page layout version number 0 is for pre-7.3 Postgres releases.
@@ -369,12 +367,7 @@ typedef PageHeaderData *PageHeader;
369367
#define PageSetAllVisible(page) \
370368
(((PageHeader) (page))->pd_flags |= PD_ALL_VISIBLE)
371369
#define PageClearAllVisible(page) \
372-
(((PageHeader) (page))->pd_flags &= ~(PD_ALL_VISIBLE | PD_ALL_FROZEN))
373-
374-
#define PageIsAllFrozen(page) \
375-
(((PageHeader) (page))->pd_flags & PD_ALL_FROZEN)
376-
#define PageSetAllFrozen(page) \
377-
(((PageHeader) (page))->pd_flags |= PD_ALL_FROZEN)
370+
(((PageHeader) (page))->pd_flags &= ~PD_ALL_VISIBLE)
378371

379372
#define PageIsPrunable(page, oldestxmin) \
380373
( \

0 commit comments

Comments
 (0)