Rearrange lazy_scan_heap to avoid visibility map race conditions.

We must set the visibility map bit before releasing our exclusive lock
on the heap page; otherwise, someone might clear the heap page bit
before we set the visibility map bit, leading to a situation where the
visibility map thinks the page is all-visible but it's really not.

This problem has existed since 8.4, but it wasn't critical before we
had index-only scans, since the worst case scenario was that the page
wouldn't get vacuumed until the next scan_all vacuum.

Along the way, a couple of minor, related improvements: (1) if we
pause the heap scan to do an index vac cycle, release any visibility
map page we're holding, since really long-running pins are not good
for a variety of reasons; and (2) warn if we see a page that's marked
all-visible in the visibility map but not on the page level, since
that should never happen any more (it was allowed in previous
releases, but not in 9.2).

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index d2cfcc024c1ae801075b4b5bc27139c05fe58ca7..60171470d36fbba31b621ef15f12f517153fa440 100644 (file)
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -490,6 +490,18 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
                if ((vacrelstats->max_dead_tuples - vacrelstats->num_dead_tuples) < MaxHeapTuplesPerPage &&
                        vacrelstats->num_dead_tuples > 0)
                {
+                       /*
+                        * Before beginning index vacuuming, we release any pin we may hold
+                        * on the visibility map page.  This isn't necessary for correctness,
+                        * but we do it anyway to avoid holding the pin across a lengthy,
+                        * unrelated operation.
+                        */
+                       if (BufferIsValid(vmbuffer))
+                       {
+                               ReleaseBuffer(vmbuffer);
+                               vmbuffer = InvalidBuffer;
+                       }
+
                        /* Log cleanup info before we touch indexes */
                        vacuum_log_cleanup_info(onerel, vacrelstats);
 
@@ -510,6 +522,16 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
                        vacrelstats->num_index_scans++;
                }
 
+               /*
+                * Pin the visibility map page in case we need to mark the page
+                * all-visible.  In most cases this will be very cheap, because we'll
+                * already have the correct page pinned anyway.  However, it's possible
+                * that (a) next_not_all_visible_block is covered by a different VM page
+                * than the current block or (b) we released our pin and did a cycle of
+                * index vacuuming.
+                */
+               visibilitymap_pin(onerel, blkno, &vmbuffer);
+
                buf = ReadBufferExtended(onerel, MAIN_FORKNUM, blkno,
                                                                 RBM_NORMAL, vac_strategy);
 
@@ -600,26 +622,15 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
                        empty_pages++;
                        freespace = PageGetHeapFreeSpace(page);
 
+                       /* empty pages are always all-visible */
                        if (!PageIsAllVisible(page))
                        {
                                PageSetAllVisible(page);
                                MarkBufferDirty(buf);
+                               visibilitymap_set(onerel, blkno, InvalidXLogRecPtr, vmbuffer);
                        }
 
-                       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
-
-                       /* Update the visibility map */
-                       if (!all_visible_according_to_vm)
-                       {
-                               visibilitymap_pin(onerel, blkno, &vmbuffer);
-                               LockBuffer(buf, BUFFER_LOCK_SHARE);
-                               if (PageIsAllVisible(page))
-                                       visibilitymap_set(onerel, blkno, InvalidXLogRecPtr,
-                                                                         vmbuffer);
-                               LockBuffer(buf, BUFFER_LOCK_UNLOCK);
-                       }
-
-                       ReleaseBuffer(buf);
+                       UnlockReleaseBuffer(buf);
                        RecordPageWithFreeSpace(onerel, blkno, freespace);
                        continue;
                }
@@ -834,11 +845,26 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
 
                freespace = PageGetHeapFreeSpace(page);
 
-               /* Update the all-visible flag on the page */
-               if (!PageIsAllVisible(page) && all_visible)
+               /* mark page all-visible, if appropriate */
+               if (all_visible && !all_visible_according_to_vm)
                {
-                       PageSetAllVisible(page);
-                       MarkBufferDirty(buf);
+                       if (!PageIsAllVisible(page))
+                       {
+                               PageSetAllVisible(page);
+                               MarkBufferDirty(buf);
+                       }
+                       visibilitymap_set(onerel, blkno, InvalidXLogRecPtr, vmbuffer);
+               }
+
+               /*
+                * As of PostgreSQL 9.2, the visibility map bit should never be set if
+                * the page-level bit is clear.
+                */
+               else if (all_visible_according_to_vm && !PageIsAllVisible(page))
+               {
+                       elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u",
+                                relname, blkno);
+                       visibilitymap_clear(onerel, blkno, vmbuffer);
                }
 
                /*
@@ -859,30 +885,11 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
                        elog(WARNING, "page containing dead tuples is marked as all-visible in relation \"%s\" page %u",
                                 relname, blkno);
                        PageClearAllVisible(page);
-                       SetBufferCommitInfoNeedsSave(buf);
-
-                       /*
-                        * Normally, we would drop the lock on the heap page before
-                        * updating the visibility map, but since this case shouldn't
-                        * happen anyway, don't worry about that.
-                        */
-                       visibilitymap_pin(onerel, blkno, &vmbuffer);
+                       MarkBufferDirty(buf);
                        visibilitymap_clear(onerel, blkno, vmbuffer);
                }
 
-               LockBuffer(buf, BUFFER_LOCK_UNLOCK);
-
-               /* Update the visibility map */
-               if (!all_visible_according_to_vm && all_visible)
-               {
-                       visibilitymap_pin(onerel, blkno, &vmbuffer);
-                       LockBuffer(buf, BUFFER_LOCK_SHARE);
-                       if (PageIsAllVisible(page))
-                               visibilitymap_set(onerel, blkno, InvalidXLogRecPtr, vmbuffer);
-                       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
-               }
-
-               ReleaseBuffer(buf);
+               UnlockReleaseBuffer(buf);
 
                /* Remember the location of the last page with nonremovable tuples */
                if (hastup)