Fix for bug 3525691: autovacuum process issue

This commit fixes 2 issues:
1) On Coordinator, autovacuum processes were using local snapshots
while it needs to fetch global snapshot data from GTM. This made several
PostgreSQL internal processes going mad with visibility like statistics or
catalogs in an environment using a lot of DDL.
2) On Datanodes, autovacuum non-analyze processes did not fetch a global
transaction ID from GTM and this transaction ID was not included in global
snapshot. Hence, global data consistency and visibility was compromised
on local nodes. This issue made autovacuum and pg_toast going crazy if the
system was put under heavy load (transaction timeout occuring).

It is believed that this bug is here since Postgres-XC 0.9.4, far before the
code was merged with PostgreSQL 9.1, and was causing numerous issues with
long and short-time runs.
For a reason I do not completely understand, this bug has become really easy
to reproduce since some race conditions in pg_toast code (tuptoaster.c) have
been fixed by Tom Lane in 9.1 stable.

2 files changed, 18 insertions, 12 deletions

diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index 2f6b9fad42..0b534243f4 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -134,7 +134,7 @@ GetNewTransactionId(bool isSubXact)
 	LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
 
 #ifdef PGXC
-	/* Only remote Coordinator can go a GXID */
+	/* Only remote Coordinator can get a GXID */
 	if (IS_PGXC_COORDINATOR && !IsConnFromCoord())
 	{
 		if (TransactionIdIsValid(xid)) 
@@ -164,11 +164,15 @@ GetNewTransactionId(bool isSubXact)
 		if (IsAutoVacuumWorkerProcess())
 		{
 			/*
-			 * Get gxid directly from GTM.
-			 * We use a separate function so that GTM knows to exclude it from
-			 * other snapshots.
+			 * For an autovacuum worker process, get transaction ID directly from GTM.
+			 * If this vacuum process is a vacuum analyze, its GXID has to be excluded
+			 * from snapshots so use a special function for this purpose.
+			 * For a simple worker get transaction ID like a normal transaction would do.
 			 */
-			next_xid = (TransactionId) BeginTranAutovacuumGTM();
+			if (MyProc->vacuumFlags & PROC_IN_VACUUM)
+				next_xid = (TransactionId) BeginTranAutovacuumGTM();
+			else
+				next_xid = (TransactionId) BeginTranGTM(timestamp);
 		}
 		else if (GetForceXidFromGTM())
 		{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 8dca1fa735..2fa18c6d63 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1252,12 +1252,15 @@ GetSnapshotData(Snapshot snapshot)
 
 #ifdef PGXC  /* PGXC_DATANODE */
 	/*
- 	 * The typical case is that the coordinator passes down the snapshot to the
- 	 * data nodes to use, while it itselfs obtains them from GTM.
- 	 * The data nodes may however connect directly to GTM themselves to obtain
- 	 * XID and snapshot information for autovacuum worker threads.
+ 	 * The typical case is that the local Coordinator passes down the snapshot to the
+ 	 * remote nodes to use, while it itself obtains it from GTM. Autovacuum processes
+	 * need however to connect directly to GTM themselves to obtain XID and snapshot
+	 * information for autovacuum worker threads.
+	 * A vacuum analyze uses a special function to get a transaction ID and signal
+	 * GTM not to include this transaction ID in snapshot.
+	 * A vacuum worker starts as a normal transaction would.
  	 */
-	if (IS_PGXC_DATANODE || IsConnFromCoord())
+	if (IS_PGXC_DATANODE || IsConnFromCoord() || IsAutoVacuumWorkerProcess())
 	{
 		if (GetSnapshotDataDataNode(snapshot))
 			return snapshot;
@@ -2487,8 +2490,7 @@ UnsetGlobalSnapshotData(void)
 static bool 
 GetSnapshotDataDataNode(Snapshot snapshot)
 {
-	Assert(IS_PGXC_DATANODE || IsConnFromCoord());
-
+	Assert(IS_PGXC_DATANODE || IsConnFromCoord() || IsAutoVacuumWorkerProcess());
 
 	if (IsAutoVacuumWorkerProcess() || GetForceXidFromGTM())
 	{