From 99bb79745a1d0b4f399e832eaa48c8976d975505 Mon Sep 17 00:00:00 2001
From: Sait Talha Nisanci <s.talhanisanci@gmail.com>
Date: Wed, 7 Oct 2020 17:34:39 +0300
Subject: [PATCH] Commit transaction for VACUUM on shell table

With postgres 13, there is a global lock that prevents multiple VACUUMs
happening in the current database. This global lock is taken for a short
time but this creates a problem because of the following:

- We execute the VACUUM for the shell table through the standard process
utility. In this step the global lock is taken for the current database.
- If the current node has shard placements then it tries to execute
VACUUM over a connection to localhost with ExecuteUtilityTaskList.
- the VACUUM on shard placements cannot proceed because it is waiting
for the global lock for the current database to be released.
- The acquired lock from the VACUUM for shell table will not be released
until the transaction is committed.
- So there is a deadlock.

As a solution, we commit the current transaction in case of VACUUM after
the VACUUM is executed for the shell table. Executing the VACUUM on a
shell table is not important because the data there will probably be
truncated. PostprocessVacuumStmt takes the necessary locks on the shell
table so we don't need to take any extra locks after we commit the
current transaction.
---
 src/backend/distributed/commands/vacuum.c | 38 ++++++++++++++++++++---
 src/test/regress/expected/single_node.out |  5 +++
 src/test/regress/sql/single_node.sql      |  5 +++
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/src/backend/distributed/commands/vacuum.c b/src/backend/distributed/commands/vacuum.c
index 706fea00c..accef1e0c 100644
--- a/src/backend/distributed/commands/vacuum.c
+++ b/src/backend/distributed/commands/vacuum.c
@@ -30,6 +30,7 @@
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "postmaster/bgworker_internals.h"
+#include "access/xact.h"
 
 
 #define VACUUM_PARALLEL_NOTSET -2
@@ -58,6 +59,8 @@ static char * DeparseVacuumStmtPrefix(CitusVacuumParams vacuumParams);
 static char * DeparseVacuumColumnNames(List *columnNameList);
 static List * VacuumColumnList(VacuumStmt *vacuumStmt, int relationIndex);
 static List * ExtractVacuumTargetRels(VacuumStmt *vacuumStmt);
+static void ExecuteVacuumOnDistributedTables(VacuumStmt *vacuumStmt, List *relationIdList,
+											 CitusVacuumParams vacuumParams);
 static CitusVacuumParams VacuumStmtParams(VacuumStmt *vacstmt);
 
 /*
@@ -73,13 +76,24 @@ static CitusVacuumParams VacuumStmtParams(VacuumStmt *vacstmt);
 void
 PostprocessVacuumStmt(VacuumStmt *vacuumStmt, const char *vacuumCommand)
 {
-	int relationIndex = 0;
-	List *vacuumRelationList = ExtractVacuumTargetRels(vacuumStmt);
-	List *relationIdList = NIL;
 	CitusVacuumParams vacuumParams = VacuumStmtParams(vacuumStmt);
 	LOCKMODE lockMode = (vacuumParams.options & VACOPT_FULL) ? AccessExclusiveLock :
 						ShareUpdateExclusiveLock;
-	int executedVacuumCount = 0;
+
+
+	if (vacuumParams.options & VACOPT_VACUUM)
+	{
+		/*
+		 * We commit the current transaction here so that the global lock
+		 * taken from the shell table for VACUUM is released, which would block execution
+		 * of shard placements.
+		 */
+		CommitTransactionCommand();
+		StartTransactionCommand();
+	}
+
+	List *vacuumRelationList = ExtractVacuumTargetRels(vacuumStmt);
+	List *relationIdList = NIL;
 
 	RangeVar *vacuumRelation = NULL;
 	foreach_ptr(vacuumRelation, vacuumRelationList)
@@ -95,7 +109,21 @@ PostprocessVacuumStmt(VacuumStmt *vacuumStmt, const char *vacuumCommand)
 		return;
 	}
 
-	/* execute vacuum on distributed tables */
+	ExecuteVacuumOnDistributedTables(vacuumStmt, relationIdList, vacuumParams);
+}
+
+
+/*
+ * ExecuteVacuumOnDistributedTables executes the vacuum for the shard placements of given tables
+ * if they are citus tables.
+ */
+static void
+ExecuteVacuumOnDistributedTables(VacuumStmt *vacuumStmt, List *relationIdList,
+								 CitusVacuumParams vacuumParams)
+{
+	int relationIndex = 0;
+	int executedVacuumCount = 0;
+
 	Oid relationId = InvalidOid;
 	foreach_oid(relationId, relationIdList)
 	{
diff --git a/src/test/regress/expected/single_node.out b/src/test/regress/expected/single_node.out
index d0610d6cf..4093e7c76 100644
--- a/src/test/regress/expected/single_node.out
+++ b/src/test/regress/expected/single_node.out
@@ -112,6 +112,11 @@ BEGIN;
 
 ROLLBACK;
 VACUUM test;
+VACUUM test, test_2;
+VACUUM ref, test;
+VACUUM ANALYZE test(x);
+ANALYZE ref;
+ANALYZE test_2;
 BEGIN;
 	ALTER TABLE test ADD COLUMN z INT DEFAULT 66;
 	SELECT count(*) FROM test WHERE z = 66;
diff --git a/src/test/regress/sql/single_node.sql b/src/test/regress/sql/single_node.sql
index 59463bcce..afe4f0f44 100644
--- a/src/test/regress/sql/single_node.sql
+++ b/src/test/regress/sql/single_node.sql
@@ -58,6 +58,11 @@ BEGIN;
 ROLLBACK;
 
 VACUUM test;
+VACUUM test, test_2;
+VACUUM ref, test;
+VACUUM ANALYZE test(x);
+ANALYZE ref;
+ANALYZE test_2;
 
 BEGIN;
 	ALTER TABLE test ADD COLUMN z INT DEFAULT 66;