Fix a violation of WAL coding rules in the recent patch to include an

"all tuples visible" flag in heap page headers.  The flag update *must*
be applied before calling XLogInsert, but heap_update and the tuple
moving routines in VACUUM FULL were ignoring this rule.  A crash and
replay could therefore leave the flag incorrectly set, causing rows
to appear visible in seqscans when they should not be.  This might explain
recent reports of data corruption from Jeff Ross and others.

In passing, do a bit of editorialization on comments in visibilitymap.c.

1 files changed, 8 insertions, 7 deletions

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 01aa11a6d21..03a0daa0d4b 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -29,7 +29,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/vacuumlazy.c,v 1.121 2009/06/11 14:48:56 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/vacuumlazy.c,v 1.121.2.1 2009/08/24 02:18:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -425,8 +425,8 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
 
 			if (!PageIsAllVisible(page))
 			{
-				SetBufferCommitInfoNeedsSave(buf);
 				PageSetAllVisible(page);
+				SetBufferCommitInfoNeedsSave(buf);
 			}
 
 			LockBuffer(buf, BUFFER_LOCK_UNLOCK);
@@ -652,19 +652,20 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
 		/* Update the all-visible flag on the page */
 		if (!PageIsAllVisible(page) && all_visible)
 		{
-			SetBufferCommitInfoNeedsSave(buf);
 			PageSetAllVisible(page);
+			SetBufferCommitInfoNeedsSave(buf);
 		}
 		else if (PageIsAllVisible(page) && !all_visible)
 		{
-			elog(WARNING, "PD_ALL_VISIBLE flag was incorrectly set");
-			SetBufferCommitInfoNeedsSave(buf);
+			elog(WARNING, "PD_ALL_VISIBLE flag was incorrectly set 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 is a can't-happen
-			 * case anyway, don't bother.
+			 * updating the visibility map, but since this case shouldn't
+			 * happen anyway, don't worry about that.
 			 */
 			visibilitymap_clear(onerel, blkno);
 		}