Fix longstanding problems in VACUUM caused by untimely interruptions

In VACUUM FULL, an interrupt after the initial transaction has been recorded
as committed can cause postmaster to restart with the following error message:
PANIC: cannot abort transaction NNNN, it was already committed
This problem has been reported many times.

In lazy VACUUM, an interrupt after the table has been truncated by
lazy_truncate_heap causes other backends' relcache to still point to the
removed pages; this can cause future INSERT and UPDATE queries to error out
with the following error message:
could not read block XX of relation 1663/NNN/MMMM: read only 0 of 8192 bytes
The window to this race condition is extremely narrow, but it has been seen in
the wild involving a cancelled autovacuum process.

The solution for both problems is to inhibit interrupts in both operations
until after the respective transactions have been committed.  It's not a
complete solution, because the transaction could theoretically be aborted by
some other error, but at least fixes the most common causes of both problems.

3 files changed, 52 insertions, 13 deletions

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 84a654799c..afb2ad7516 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -13,7 +13,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.263.2.5 2008/02/11 19:15:00 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.263.2.6 2009/11/10 18:01:46 alvherre Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -114,10 +114,10 @@ static void vac_update_dbstats(Oid dbid,
 static void vac_truncate_clog(TransactionId vacuumXID,
 				  TransactionId frozenXID);
 static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind);
-static void full_vacuum_rel(Relation onerel, VacuumStmt *vacstmt);
+static bool full_vacuum_rel(Relation onerel, VacuumStmt *vacstmt);
 static void scan_heap(VRelStats *vacrelstats, Relation onerel,
 		  VacPageList vacuum_pages, VacPageList fraged_pages);
-static void repair_frag(VRelStats *vacrelstats, Relation onerel,
+static bool repair_frag(VRelStats *vacrelstats, Relation onerel,
 			VacPageList vacuum_pages, VacPageList fraged_pages,
 			int nindexes, Relation *Irel);
 static void vacuum_heap(VRelStats *vacrelstats, Relation onerel,
@@ -744,6 +744,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	bool		result;
 	AclId		save_userid;
 	bool		save_secdefcxt;
+	bool		heldoff;
 
 	/* Begin a transaction for vacuuming this relation */
 	StartTransactionCommand();
@@ -860,9 +861,9 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	 * Do the actual work --- either FULL or "lazy" vacuum
 	 */
 	if (vacstmt->full)
-		full_vacuum_rel(onerel, vacstmt);
+		heldoff = full_vacuum_rel(onerel, vacstmt);
 	else
-		lazy_vacuum_rel(onerel, vacstmt);
+		heldoff = lazy_vacuum_rel(onerel, vacstmt);
 
 	result = true;				/* did the vacuum */
 
@@ -877,6 +878,10 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
 	 */
 	CommitTransactionCommand();
 
+	/* now we can allow interrupts again, if disabled */
+	if (heldoff)
+		RESUME_INTERRUPTS();
+
 	/*
 	 * If the relation has a secondary toast rel, vacuum that too while we
 	 * still hold the session lock on the master table.  Note however that
@@ -915,8 +920,11 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind)
  *
  *		At entry, we have already established a transaction and opened
  *		and locked the relation.
+ *
+ *		The return value indicates whether this function has held off
+ *		interrupts -- caller must RESUME_INTERRUPTS() after commit if true.
  */
-static void
+static bool
 full_vacuum_rel(Relation onerel, VacuumStmt *vacstmt)
 {
 	VacPageListData vacuum_pages;		/* List of pages to vacuum and/or
@@ -927,6 +935,7 @@ full_vacuum_rel(Relation onerel, VacuumStmt *vacstmt)
 	int			nindexes,
 				i;
 	VRelStats  *vacrelstats;
+	bool		heldoff = false;
 
 	vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared,
 						  &OldestXmin, &FreezeLimit);
@@ -968,8 +977,8 @@ full_vacuum_rel(Relation onerel, VacuumStmt *vacstmt)
 	if (fraged_pages.num_pages > 0)
 	{
 		/* Try to shrink heap */
-		repair_frag(vacrelstats, onerel, &vacuum_pages, &fraged_pages,
-					nindexes, Irel);
+		heldoff = repair_frag(vacrelstats, onerel, &vacuum_pages, &fraged_pages,
+						  	  nindexes, Irel);
 		vac_close_indexes(nindexes, Irel);
 	}
 	else
@@ -1000,6 +1009,8 @@ full_vacuum_rel(Relation onerel, VacuumStmt *vacstmt)
 	/* update statistics in pg_class */
 	vac_update_relstats(RelationGetRelid(onerel), vacrelstats->rel_pages,
 						vacrelstats->rel_tuples, vacrelstats->hasindex);
+
+	return heldoff;
 }
 
 
@@ -1412,8 +1423,11 @@ scan_heap(VRelStats *vacrelstats, Relation onerel,
  *		for them after committing (in hack-manner - without losing locks
  *		and freeing memory!) current transaction. It truncates relation
  *		if some end-blocks are gone away.
+ *
+ *		The return value indicates whether this function has held off
+ *		interrupts -- caller must RESUME_INTERRUPTS() after commit if true.
  */
-static void
+static bool
 repair_frag(VRelStats *vacrelstats, Relation onerel,
 			VacPageList vacuum_pages, VacPageList fraged_pages,
 			int nindexes, Relation *Irel)
@@ -1459,6 +1473,7 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
 				dowrite,
 				chain_tuple_moved;
 	VacRUsage	ru0;
+	bool		heldoff = false;
 
 	vac_init_rusage(&ru0);
 
@@ -2358,7 +2373,13 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
 		 * a lot of extra code to close and re-open the relation, indexes,
 		 * etc.  For now, a quick hack: record status of current
 		 * transaction as committed, and continue.
+		 *
+		 * We prevent cancel interrupts after this point to mitigate the
+		 * problem that you can't abort the transaction now; caller is
+		 * responsible for re-enabling them after committing the transaction.
 		 */
+		HOLD_INTERRUPTS();
+		heldoff = true;
 		RecordTransactionCommit();
 	}
 
@@ -2591,6 +2612,8 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
 	ExecCloseIndices(resultRelInfo);
 
 	FreeExecutorState(estate);
+
+	return heldoff;
 }
 
 /*
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index d7daf1ba3c..724f9ec46c 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -31,7 +31,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/commands/vacuumlazy.c,v 1.32.2.3 2009/01/06 14:56:13 heikki Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/commands/vacuumlazy.c,v 1.32.2.4 2009/11/10 18:01:46 alvherre Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -120,8 +120,11 @@ static int	vac_cmp_page_spaces(const void *left, const void *right);
  *
  *		At entry, we have already established a transaction and opened
  *		and locked the relation.
+ *
+ *		The return value indicates whether this function has held off
+ *		interrupts -- caller must RESUME_INTERRUPTS() after commit if true.
  */
-void
+bool
 lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt)
 {
 	LVRelStats *vacrelstats;
@@ -129,6 +132,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt)
 	int			nindexes;
 	bool		hasindex;
 	BlockNumber possibly_freeable;
+	bool		heldoff = false;
 
 	if (vacstmt->verbose)
 		elevel = INFO;
@@ -159,12 +163,22 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt)
 	 *
 	 * Don't even think about it unless we have a shot at releasing a goodly
 	 * number of pages.  Otherwise, the time taken isn't worth it.
+	 *
+	 * Note that after we've truncated the heap, it's too late to abort the
+	 * transaction; doing so would lose the sinval messages needed to tell the
+	 * other backends about the table being shrunk.  We prevent interrupts in
+	 * that case; caller is responsible for re-enabling them after
+	 * committing the transaction.
 	 */
 	possibly_freeable = vacrelstats->rel_pages - vacrelstats->nonempty_pages;
 	if (possibly_freeable > 0 &&
 		(possibly_freeable >= REL_TRUNCATE_MINIMUM ||
 		 possibly_freeable >= vacrelstats->rel_pages / REL_TRUNCATE_FRACTION))
+	{
+		HOLD_INTERRUPTS();
+		heldoff = true;
 		lazy_truncate_heap(onerel, vacrelstats);
+	}
 
 	/* Update shared free space map with final free space info */
 	lazy_update_fsm(onerel, vacrelstats);
@@ -172,6 +186,8 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt)
 	/* Update statistics in pg_class */
 	vac_update_relstats(RelationGetRelid(onerel), vacrelstats->rel_pages,
 						vacrelstats->rel_tuples, hasindex);
+
+	return heldoff;
 }
 
 
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 194adb17c8..99dd9c0c9b 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: vacuum.h,v 1.46 2003/08/04 02:40:13 momjian Exp $
+ * $Id: vacuum.h,v 1.46.4.1 2009/11/10 18:01:46 alvherre Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -55,7 +55,7 @@ extern void vac_init_rusage(VacRUsage *ru0);
 extern const char *vac_show_rusage(VacRUsage *ru0);
 
 /* in commands/vacuumlazy.c */
-extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt);
+extern bool lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt);
 
 /* in commands/analyze.c */
 extern void analyze_rel(Oid relid, VacuumStmt *vacstmt);