Remove MyProc inDropTransaction flag (#5301)

The MyProc inDropTransaction flag was used to make sure concurrent AO vacuum
would not conflict with each other during drop phase. Two concurrent AO vacuum
on same relation was possible back in 4.3 where the different AO vacuum phases
(prepare, compaction, drop, cleanup) would interleave with each other, and
having two AO vacuum drop phases concurrently on the same AO relation was
dangerous. We now hold the ShareUpdateExclusiveLock through the entire AO
vacuum which renders the inDropTransaction flag useless and disallows the
interleaving mechanism.
Co-authored-by: NJimmy Yih <jyih@pivotal.io>

src/backend/access/transam/twophase.c

@@ -585,7 +585,6 @@ MarkAsPreparing(TransactionId xid,
 	gxact->proc.inCommit = false;
 	gxact->proc.vacuumFlags = 0;
 	gxact->proc.serializableIsoLevel = false;
-	gxact->proc.inDropTransaction = false;
 	gxact->proc.lwWaiting = false;
 	gxact->proc.lwExclusive = false;
 	gxact->proc.lwWaitLink = NULL;

src/backend/commands/vacuum.c

@@ -1303,8 +1303,6 @@ vacuumStatement_Relation(VacuumStmt *vacstmt, Oid relid,
 
 		if (vacstmt->full)
 			lmode = AccessExclusiveLock;
-		else if (RelationIsAoRows(onerel) || RelationIsAoCols(onerel))
-			lmode = AccessShareLock;
 		else
 			lmode = ShareUpdateExclusiveLock;
 
@@ -5414,37 +5412,6 @@ open_relation_and_check_permission(VacuumStmt *vacstmt,
 	LOCKMODE lmode;
 	bool dontWait = false;
 
-	/*
-	 * If this is a drop transaction and there is another parallel drop transaction
-	 * (on any relation) active. We drop out there. The other drop transaction
-	 * might be on the same relation and that would be upgrade deadlock.
-	 *
-	 * Note: By the time we would have reached try_relation_open the other
-	 * drop transaction might already be completed, but we don't take that
-	 * risk here.
-	 *
-	 * My marking the drop transaction as busy before checking, the worst
-	 * thing that can happen is that both transaction see each other and
-	 * both cancel the drop.
-	 *
-	 * The upgrade deadlock is not applicable to vacuum full because
-	 * it begins with an AccessExclusive lock and doesn't need to
-	 * upgrade it.
-	 */
-
-	if (isDropTransaction && !vacstmt->full)
-	{
-		MyProc->inDropTransaction = true;
-		SIMPLE_FAULT_INJECTOR(VacuumRelationOpenRelationDuringDropPhase);
-		if (HasDropTransaction(false))
-		{
-			elogif(Debug_appendonly_print_compaction, LOG,
-					"Skip drop because of concurrent drop transaction");
-
-			return NULL;
-		}
-	}
-
 	/*
 	 * Determine the type of lock we want --- hard exclusive lock for a FULL
 	 * vacuum, but just ShareUpdateExclusiveLock for concurrent vacuum. Either
@@ -5453,8 +5420,22 @@ open_relation_and_check_permission(VacuumStmt *vacstmt,
 	 */
 	if (isDropTransaction)
 	{
+		/*
+		 * Upgrade to AccessExclusiveLock from SharedAccessExclusive here
+		 * before doing the drops. We set the dontwait flag here to prevent
+		 * deadlock scenarios such as a concurrent transaction holding
+		 * AccessShareLock and then upgrading to ExclusiveLock to run
+		 * DELETE/UPDATE while VACUUM is waiting here for AccessExclusiveLock.
+		 *
+		 * Skipping when we are not able to upgrade to AccessExclusivelock can
+		 * be an issue though because it is possible to accumulate a large
+		 * amount of segfiles marked AOSEG_STATE_AWAITING_DROP.  However, we do
+		 * not expect this to happen too frequently such that all segfiles are
+		 * marked.
+		 */
 		lmode = AccessExclusiveLock;
 		dontWait = true;
+		SIMPLE_FAULT_INJECTOR(VacuumRelationOpenRelationDuringDropPhase);
 	}
 	else if (!vacstmt->vacuum)
 		lmode = ShareUpdateExclusiveLock;
@@ -5470,7 +5451,12 @@ open_relation_and_check_permission(VacuumStmt *vacstmt,
 	onerel = try_relation_open(relid, lmode, dontWait);
 
 	if (!RelationIsValid(onerel))
+	{
+		elogif(Debug_appendonly_print_compaction && isDropTransaction, LOG,
+				"drop phase skipped for relation %d because we are unable to upgrade to AccessExclusiveLock",
+				relid);
 		return NULL;
+	}
 
 	/*
 	 * Check permissions.

src/backend/storage/ipc/procarray.c

@@ -310,7 +310,6 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid, bool isCommit)
 			proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
 			proc->inCommit = false; /* be sure this is cleared in abort */
 			proc->serializableIsoLevel = false;
-			proc->inDropTransaction = false;
 
 			/* Clear the subtransaction-XID cache too while holding the lock */
 			proc->subxids.nxids = 0;
@@ -346,7 +345,6 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid, bool isCommit)
 		proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
 		proc->inCommit = false; /* be sure this is cleared in abort */
 		proc->serializableIsoLevel = false;
-		proc->inDropTransaction = false;
 
 		Assert(proc->subxids.nxids == 0);
 		Assert(proc->subxids.overflowed == false);
@@ -382,7 +380,6 @@ ProcArrayClearTransaction(PGPROC *proc, bool commit)
 	/* redundant, but just in case */
 	proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
 	proc->serializableIsoLevel = false;
-	proc->inDropTransaction = false;
 
 	/* Clear the subtransaction-XID cache too */
 	proc->subxids.nxids = 0;
@@ -648,45 +645,6 @@ TransactionIdIsActive(TransactionId xid)
 	return result;
 }
 
-/*
- * Returns true if there are any UAO drop transaction active (except the current
- * one).
- *
- * If allDbs is TRUE then all backends are considered; if allDbs is FALSE
- * then only backends running in my own database are considered.
- */
-bool
-HasDropTransaction(bool allDbs)
-{
-	ProcArrayStruct *arrayP = procArray;
-	bool result = false; /* Assumes */
-	int			index;
-
-	LWLockAcquire(ProcArrayLock, LW_SHARED);
-
-	for (index = 0; index < arrayP->numProcs; index++)
-	{
-		volatile PGPROC *proc = arrayP->procs[index];
-		if (proc->pid == 0)
-			continue;			/* do not count prepared xacts */
-
-		if (allDbs || proc->databaseId == MyDatabaseId)
-		{
-			if (proc->inDropTransaction && proc != MyProc)
-			{
-				ereport((Debug_print_snapshot_dtm ? LOG : DEBUG3),
-						(errmsg("Found drop transaction: database %d, pid %d, xid %d, xmin %d",
-								proc->databaseId, proc->pid, proc->xid, proc->xmin)));
-				result = true;
-			}
-		}
-	}
-
-	LWLockRelease(ProcArrayLock);
-
-	return result;
-}
-
 /*
  * Returns true if there are of serializable backends (except the current
  * one).

src/backend/storage/lmgr/proc.c

@@ -366,7 +366,6 @@ InitProcess(void)
 	MyProc->localDistribXactData.state = LOCALDISTRIBXACT_STATE_NONE;
 	MyProc->xmin = InvalidTransactionId;
 	MyProc->serializableIsoLevel = false;
-	MyProc->inDropTransaction = false;
 	MyProc->pid = MyProcPid;
 	/* backendId, databaseId and roleId will be filled in later */
 	MyProc->backendId = InvalidBackendId;
@@ -556,7 +555,6 @@ InitAuxiliaryProcess(void)
 	MyProc->localDistribXactData.state = LOCALDISTRIBXACT_STATE_NONE;
 	MyProc->xmin = InvalidTransactionId;
 	MyProc->serializableIsoLevel = false;
-	MyProc->inDropTransaction = false;
 	MyProc->databaseId = InvalidOid;
 	MyProc->roleId = InvalidOid;
     MyProc->mppLocalProcessSerial = 0;

src/include/storage/proc.h

@@ -164,8 +164,6 @@ struct PGPROC
 
 	bool serializableIsoLevel; /* true if proc has serializable isolation level set */
 
-	bool inDropTransaction; /* true if proc is in vacuum drop transaction */
-
 	/*
 	 * Information for resource group
 	 */

src/include/storage/procarray.h

@@ -47,7 +47,6 @@ extern int	CountDBBackends(Oid databaseid);
 extern int	CountUserBackends(Oid roleid);
 extern bool CheckOtherDBBackends(Oid databaseId);
 extern bool HasSerializableBackends(bool allDbs);
-extern bool HasDropTransaction(bool allDbs);
 
 extern void XidCacheRemoveRunningXids(TransactionId xid,
 						  int nxids, const TransactionId *xids,

src/test/isolation2/input/uao/vacuum_cleanup.source

--- @Description Test that when there is a parallel vacuum going on in drop phase, the age of
+-- @Description Test that when AO vacuum skips drop phase, the age of
 -- the AO/AOCS table gets reduced correctly.
 
 CREATE EXTENSION IF NOT EXISTS gp_inject_fault;
 
-1: create table ao_@orientation@_vacuum_cleanup1(a int, b int) with(appendonly=true, orientation=@orientation@);
-1: insert into ao_@orientation@_vacuum_cleanup1 select 1, i from generate_series(1, 100) i;
-1: update ao_@orientation@_vacuum_cleanup1 set b = b + 1;
-
 -- The age of the table is 1 after the following statement
 2: create table ao_@orientation@_vacuum_cleanup2(a int, b int) with(appendonly=true, orientation=@orientation@);
 -- The age of the table is 2 after the following statement
@@ -14,25 +10,20 @@ CREATE EXTENSION IF NOT EXISTS gp_inject_fault;
 -- The age of the table is 3 after the following statement
 2: update ao_@orientation@_vacuum_cleanup2 set b = b + 1;
 
-
-1: select gp_inject_fault('vacuum_relation_open_relation_during_drop_phase', 'suspend', 2);
-
-2&: vacuum ao_@orientation@_vacuum_cleanup1;
-
-1: select wait_for_trigger_fault((select current_database()), 'vacuum_relation_open_relation_during_drop_phase', 2);
-
 1: set vacuum_freeze_min_age = 0;
 -- Check the age of the table just before vacuum
 1: select age(relfrozenxid), regexp_replace(replace(relname, 'ao_@orientation@_vacuum_cleanup2'::regclass::oid::text, '<oid>'), 'ao.*seg', '<seg>') from gp_dist_random('pg_class') where relkind in ('r','t','o','b','m') and relstorage not in ('x','f','v') and (relname like '%' || 'ao_@orientation@_vacuum_cleanup2'::regclass::oid || '%') and gp_segment_id = 0;
+
+-- Hold AccessShareLock to make AO VACUUM skip drop phase
+2: begin;
+2: select count(*) from ao_@orientation@_vacuum_cleanup2;
 1: vacuum ao_@orientation@_vacuum_cleanup2;
 
 -- We expect the age to be 3. This is because all the xids before the first
 -- vacuum will be frozen. The relfrozenxid will be the xid of the last
 -- transaction before the vacuum (in this case it is the update statement)
 1: select age(relfrozenxid), regexp_replace(replace(relname, 'ao_@orientation@_vacuum_cleanup2'::regclass::oid::text, '<oid>'), 'ao.*seg', '<seg>') from gp_dist_random('pg_class') where relkind in ('r','t','o','b','m') and relstorage not in ('x','f','v') and (relname like '%' || 'ao_@orientation@_vacuum_cleanup2'::regclass::oid || '%') and gp_segment_id = 0;
-
-1: select gp_inject_fault('vacuum_relation_open_relation_during_drop_phase', 'reset', 2);
-2<:
+2: end;
 
 -- Check that drop phase is skipped, but still the cleanup phase is performed
 -- when there are concurrent serializable transactions

src/test/isolation2/output/uao/vacuum_cleanup.source

--- @Description Test that when there is a parallel vacuum going on in drop phase, the age of
+-- @Description Test that when AO vacuum skips drop phase, the age of
 -- the AO/AOCS table gets reduced correctly.
 
 CREATE EXTENSION IF NOT EXISTS gp_inject_fault;
 CREATE
 
-1: create table ao_@orientation@_vacuum_cleanup1(a int, b int) with(appendonly=true, orientation=@orientation@);
-CREATE
-1: insert into ao_@orientation@_vacuum_cleanup1 select 1, i from generate_series(1, 100) i;
-INSERT 100
-1: update ao_@orientation@_vacuum_cleanup1 set b = b + 1;
-UPDATE 100
-
 -- The age of the table is 1 after the following statement
 2: create table ao_@orientation@_vacuum_cleanup2(a int, b int) with(appendonly=true, orientation=@orientation@);
 CREATE
@@ -21,30 +14,24 @@ INSERT 100
 2: update ao_@orientation@_vacuum_cleanup2 set b = b + 1;
 UPDATE 100
 
-
-1: select gp_inject_fault('vacuum_relation_open_relation_during_drop_phase', 'suspend', 2);
-gp_inject_fault
----------------
-t              
-(1 row)
-
-2&: vacuum ao_@orientation@_vacuum_cleanup1;  <waiting ...>
-
-1: select wait_for_trigger_fault((select current_database()), 'vacuum_relation_open_relation_during_drop_phase', 2);
-wait_for_trigger_fault
-----------------------
-t                     
-(1 row)
-
 1: set vacuum_freeze_min_age = 0;
 SET
 -- Check the age of the table just before vacuum
 1: select age(relfrozenxid), regexp_replace(replace(relname, 'ao_@orientation@_vacuum_cleanup2'::regclass::oid::text, '<oid>'), 'ao.*seg', '<seg>') from gp_dist_random('pg_class') where relkind in ('r','t','o','b','m') and relstorage not in ('x','f','v') and (relname like '%' || 'ao_@orientation@_vacuum_cleanup2'::regclass::oid || '%') and gp_segment_id = 0;
 age|regexp_replace    
 ---+------------------
-5  |pg_<seg>_<oid>    
-5  |pg_aovisimap_<oid>
+3  |pg_<seg>_<oid>    
+3  |pg_aovisimap_<oid>
 (2 rows)
+
+-- Hold AccessShareLock to make AO VACUUM skip drop phase
+2: begin;
+BEGIN
+2: select count(*) from ao_@orientation@_vacuum_cleanup2;
+count
+-----
+100  
+(1 row)
 1: vacuum ao_@orientation@_vacuum_cleanup2;
 VACUUM
 
@@ -57,14 +44,8 @@ age|regexp_replace
 3  |pg_<seg>_<oid>    
 3  |pg_aovisimap_<oid>
 (2 rows)
-
-1: select gp_inject_fault('vacuum_relation_open_relation_during_drop_phase', 'reset', 2);
-gp_inject_fault
----------------
-t              
-(1 row)
-2<:  <... completed>
-VACUUM
+2: end;
+END
 
 -- Check that drop phase is skipped, but still the cleanup phase is performed
 -- when there are concurrent serializable transactions