Remove pg_vlock locking from VACUUM, allowing multiple VACUUMs to run in

parallel --- and, not incidentally, removing a common reason for needing
manual cleanup by the DB admin after a crash.  Remove initial global
delete of pg_statistics rows in VACUUM ANALYZE; this was not only bad
for performance of other backends that had to run without stats for a
while, but it was fundamentally broken because it was done outside any
transaction.  Surprising we didn't see more consequences of that.
Detect attempt to run VACUUM inside a transaction block.  Check for
query cancel request before starting vacuum of each table.  Clean up
vacuum's private portal storage if vacuum is aborted.

src/backend/commands/vacuum.c

@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.126 1999/11/25 00:15:57 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.127 1999/11/28 02:10:01 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -21,6 +21,7 @@
 
 #include "access/genam.h"
 #include "access/heapam.h"
+#include "access/xact.h"
 #include "catalog/catalog.h"
 #include "catalog/catname.h"
 #include "catalog/index.h"
@@ -33,6 +34,7 @@
 #include "parser/parse_oper.h"
 #include "storage/sinval.h"
 #include "storage/smgr.h"
+#include "tcop/tcopprot.h"
 #include "utils/builtins.h"
 #include "utils/inval.h"
 #include "utils/portal.h"
@@ -46,7 +48,6 @@
 #include <sys/resource.h>
 #endif
 
- /* #include <port-protos.h> *//* Why? */
 
 bool		VacuumRunning = false;
 
@@ -80,11 +81,10 @@ static void vc_scanoneind(Relation indrel, int num_tuples);
 static void vc_attrstats(Relation onerel, VRelStats *vacrelstats, HeapTuple tuple);
 static void vc_bucketcpy(Form_pg_attribute attr, Datum value, Datum *bucket, int *bucket_len);
 static void vc_updstats(Oid relid, int num_pages, int num_tuples, bool hasindex, VRelStats *vacrelstats);
-static void vc_delhilowstats(Oid relid, int attcnt, int *attnums);
+static void vc_delstats(Oid relid, int attcnt, int *attnums);
 static VPageDescr vc_tidreapped(ItemPointer itemptr, VPageList vpl);
 static void vc_reappage(VPageList vpl, VPageDescr vpc);
 static void vc_vpinsert(VPageList vpl, VPageDescr vpnew);
-static void vc_free(VRelList vrl);
 static void vc_getindices(Oid relid, int *nindices, Relation **Irel);
 static void vc_clsindices(int nindices, Relation *Irel);
 static void vc_mkindesc(Relation onerel, int nindices, Relation *Irel, IndDesc **Idesc);
@@ -98,61 +98,55 @@ static bool vc_enough_space(VPageDescr vpd, Size len);
 void
 vacuum(char *vacrel, bool verbose, bool analyze, List *va_spec)
 {
-	char	   *pname;
-	MemoryContext old;
-	PortalVariableMemory pmem;
 	NameData	VacRel;
+	PortalVariableMemory pmem;
+	MemoryContext old;
 	List	   *le;
 	List	   *va_cols = NIL;
 
+	if (va_spec != NIL && !analyze)
+		elog(ERROR, "Can't vacuum columns, only tables.  You can 'vacuum analyze' columns.");
+
 	/*
-	 * Create a portal for safe memory across transctions.	We need to
+	 * We cannot run VACUUM inside a user transaction block; if we were
-	 * palloc the name space for it because our hash function expects the
+	 * inside a transaction, then our commit- and start-transaction-command
-	 * name to be on a longword boundary.  CreatePortal copies the name to
+	 * calls would not have the intended effect!  Furthermore, the forced
-	 * safe storage for us.
+	 * commit that occurs before truncating the relation's file would have
+	 * the effect of committing the rest of the user's transaction too,
+	 * which would certainly not be the desired behavior.
 	 */
-	pname = (char *) palloc(strlen(VACPNAME) + 1);
+	if (IsTransactionBlock())
-	strcpy(pname, VACPNAME);
+		elog(ERROR, "VACUUM cannot run inside a BEGIN/END block");
-	vc_portal = CreatePortal(pname);
+
-	pfree(pname);
+	/* initialize vacuum cleaner, particularly vc_portal */
+	vc_init();
 
 	if (verbose)
 		MESSAGE_LEVEL = NOTICE;
 	else
 		MESSAGE_LEVEL = DEBUG;
 
-	/* vacrel gets de-allocated on transaction commit */
+	/* vacrel gets de-allocated on transaction commit, so copy it */
 	if (vacrel)
 		strcpy(NameStr(VacRel), vacrel);
 
+	/* must also copy the column list, if any, to safe storage */
 	pmem = PortalGetVariableMemory(vc_portal);
 	old = MemoryContextSwitchTo((MemoryContext) pmem);
-
-	if (va_spec != NIL && !analyze)
-		elog(ERROR, "Can't vacuum columns, only tables.  You can 'vacuum analyze' columns.");
-
 	foreach(le, va_spec)
 	{
 		char	   *col = (char *) lfirst(le);
-		char	   *dest;
 
-		dest = (char *) palloc(strlen(col) + 1);
+		va_cols = lappend(va_cols, pstrdup(col));
-		strcpy(dest, col);
-		va_cols = lappend(va_cols, dest);
 	}
 	MemoryContextSwitchTo(old);
 
-	/* initialize vacuum cleaner */
-	vc_init();
-
 	/* vacuum the database */
 	if (vacrel)
 		vc_vacuum(&VacRel, analyze, va_cols);
 	else
 		vc_vacuum(NULL, analyze, NIL);
 
-	PortalDestroy(&vc_portal);
-
 	/* clean up */
 	vc_shutdown();
 }
@@ -160,14 +154,17 @@ vacuum(char *vacrel, bool verbose, bool analyze, List *va_spec)
 /*
  *	vc_init(), vc_shutdown() -- start up and shut down the vacuum cleaner.
  *
- *		We run exactly one vacuum cleaner at a time.  We use the file system
+ *		Formerly, there was code here to prevent more than one VACUUM from
- *		to guarantee an exclusive lock on vacuuming, since a single vacuum
+ *		executing concurrently in the same database.  However, there's no
- *		cleaner instantiation crosses transaction boundaries, and we'd lose
+ *		good reason to prevent that, and manually removing lockfiles after
- *		postgres-style locks at the end of every transaction.
+ *		a vacuum crash was a pain for dbadmins.  So, forget about lockfiles,
+ *		and just rely on the exclusive lock we grab on each target table
+ *		to ensure that there aren't two VACUUMs running on the same table
+ *		at the same time.
  *
  *		The strangeness with committing and starting transactions in the
  *		init and shutdown routines is due to the fact that the vacuum cleaner
- *		is invoked via a sql command, and so is already executing inside
+ *		is invoked via an SQL command, and so is already executing inside
  *		a transaction.	We need to leave ourselves in a predictable state
  *		on entry and exit to the vacuum cleaner.  We commit the transaction
  *		started in PostgresMain() inside vc_init(), and start one in
@@ -177,27 +174,23 @@ vacuum(char *vacrel, bool verbose, bool analyze, List *va_spec)
 static void
 vc_init()
 {
-	int			fd;
+	char	   *pname;
-
-#ifndef __CYGWIN32__
-	if ((fd = open("pg_vlock", O_CREAT | O_EXCL, 0600)) < 0)
-#else
-	if ((fd = open("pg_vlock", O_CREAT | O_EXCL | O_BINARY, 0600)) < 0)
-#endif
-	{
-		elog(ERROR, "Can't create lock file.  Is another vacuum cleaner running?\n\
-\tIf not, you may remove the pg_vlock file in the %s\n\
-\tdirectory", DatabasePath);
-	}
-	close(fd);
 
 	/*
-	 * By here, exclusive open on the lock file succeeded.	If we abort
+	 * Create a portal for safe memory across transactions. We need to
-	 * for any reason during vacuuming, we need to remove the lock file.
+	 * palloc the name space for it because our hash function expects the
+	 * name to be on a longword boundary.  CreatePortal copies the name to
+	 * safe storage for us.
+	 */
+	pname = pstrdup(VACPNAME);
+	vc_portal = CreatePortal(pname);
+	pfree(pname);
+
+	/*
+	 * Set flag to indicate that vc_portal must be removed after an error.
 	 * This global variable is checked in the transaction manager on xact
 	 * abort, and the routine vc_abort() is called if necessary.
 	 */
-
 	VacuumRunning = true;
 
 	/* matches the StartTransaction in PostgresMain() */
@@ -221,25 +214,28 @@ vc_shutdown()
 	 */
 	unlink(RELCACHE_INIT_FILENAME);
 
-	/* remove the vacuum cleaner lock file */
+	/*
-	if (unlink("pg_vlock") < 0)
+	 * Release our portal for cross-transaction memory.
-		elog(ERROR, "vacuum: can't destroy lock file!");
+	 */
+	PortalDestroy(&vc_portal);
 
 	/* okay, we're done */
 	VacuumRunning = false;
 
 	/* matches the CommitTransaction in PostgresMain() */
 	StartTransactionCommand();
-
 }
 
 void
 vc_abort()
 {
-	/* on abort, remove the vacuum cleaner lock file */
+	/* Clear flag first, to avoid recursion if PortalDestroy elog's */
-	unlink("pg_vlock");
-
 	VacuumRunning = false;
+
+	/*
+	 * Release our portal for cross-transaction memory.
+	 */
+	PortalDestroy(&vc_portal);
 }
 
 /*
@@ -259,14 +255,9 @@ vc_vacuum(NameData *VacRelP, bool analyze, List *va_cols)
 	/* get list of relations */
 	vrl = vc_getrels(VacRelP);
 
-	if (analyze && VacRelP == NULL && vrl != NULL)
-		vc_delhilowstats(InvalidOid, 0, NULL);
-
 	/* vacuum each heap relation */
 	for (cur = vrl; cur != (VRelList) NULL; cur = cur->vrl_next)
 		vc_vacone(cur->vrl_relid, analyze, va_cols);
-
-	vc_free(vrl);
 }
 
 static VRelList
@@ -381,6 +372,13 @@ vc_vacone(Oid relid, bool analyze, List *va_cols)
 
 	StartTransactionCommand();
 
+	/*
+	 * Check for user-requested abort.  Note we want this to be inside
+	 * a transaction, so xact.c doesn't issue useless NOTICE.
+	 */
+	if (QueryCancel)
+		CancelQuery();
+
 	/*
 	 * Race condition -- if the pg_class tuple has gone away since the
 	 * last time we saw it, we don't need to vacuum it.
@@ -500,7 +498,8 @@ vc_vacone(Oid relid, bool analyze, List *va_cols)
 				stats->outfunc = InvalidOid;
 		}
 		vacrelstats->va_natts = attr_cnt;
-		vc_delhilowstats(relid, ((attnums) ? attr_cnt : 0), attnums);
+		/* delete existing pg_statistics rows for relation */
+		vc_delstats(relid, ((attnums) ? attr_cnt : 0), attnums);
 		if (attnums)
 			pfree(attnums);
 	}
@@ -2478,11 +2477,12 @@ vc_updstats(Oid relid, int num_pages, int num_tuples, bool hasindex, VRelStats *
 }
 
 /*
- *	vc_delhilowstats() -- delete pg_statistics rows
+ *	vc_delstats() -- delete pg_statistics rows for a relation
  *
+ *	If a list of attribute numbers is given, only zap stats for those attrs.
  */
 static void
-vc_delhilowstats(Oid relid, int attcnt, int *attnums)
+vc_delstats(Oid relid, int attcnt, int *attnums)
 {
 	Relation	pgstatistic;
 	HeapScanDesc scan;
@@ -2491,15 +2491,9 @@ vc_delhilowstats(Oid relid, int attcnt, int *attnums)
 
 	pgstatistic = heap_openr(StatisticRelationName, RowExclusiveLock);
 
-	if (relid != InvalidOid)
+	ScanKeyEntryInitialize(&key, 0x0, Anum_pg_statistic_starelid,
-	{
+						   F_OIDEQ, ObjectIdGetDatum(relid));
-		ScanKeyEntryInitialize(&key, 0x0, Anum_pg_statistic_starelid,
+	scan = heap_beginscan(pgstatistic, false, SnapshotNow, 1, &key);
-							   F_OIDEQ,
-							   ObjectIdGetDatum(relid));
-		scan = heap_beginscan(pgstatistic, false, SnapshotNow, 1, &key);
-	}
-	else
-		scan = heap_beginscan(pgstatistic, false, SnapshotNow, 0, NULL);
 
 	while (HeapTupleIsValid(tuple = heap_getnext(scan, 0)))
 	{
@@ -2572,28 +2566,6 @@ vc_vpinsert(VPageList vpl, VPageDescr vpnew)
 
 }
 
-static void
-vc_free(VRelList vrl)
-{
-	VRelList	p_vrl;
-	MemoryContext old;
-	PortalVariableMemory pmem;
-
-	pmem = PortalGetVariableMemory(vc_portal);
-	old = MemoryContextSwitchTo((MemoryContext) pmem);
-
-	while (vrl != (VRelList) NULL)
-	{
-
-		/* free rel list entry */
-		p_vrl = vrl;
-		vrl = vrl->vrl_next;
-		pfree(p_vrl);
-	}
-
-	MemoryContextSwitchTo(old);
-}
-
 static void *
 vc_find_eq(void *bot, int nelem, int size, void *elm,
 		   int (*compar) (const void *, const void *))