From 99bb79745a1d0b4f399e832eaa48c8976d975505 Mon Sep 17 00:00:00 2001 From: Sait Talha Nisanci Date: Wed, 7 Oct 2020 17:34:39 +0300 Subject: [PATCH 1/2] 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; From dc40758355405bdeea4bba9f43b7952946e2d4b8 Mon Sep 17 00:00:00 2001 From: Sait Talha Nisanci Date: Thu, 8 Oct 2020 21:17:16 +0300 Subject: [PATCH 2/2] Return early if there is no citus table in VACUUM --- src/backend/distributed/commands/vacuum.c | 88 ++++++++++++++++------- src/test/regress/expected/single_node.out | 3 + src/test/regress/sql/single_node.sql | 3 + 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/src/backend/distributed/commands/vacuum.c b/src/backend/distributed/commands/vacuum.c index accef1e0c..55b30ebfb 100644 --- a/src/backend/distributed/commands/vacuum.c +++ b/src/backend/distributed/commands/vacuum.c @@ -52,7 +52,7 @@ typedef struct CitusVacuumParams } CitusVacuumParams; /* Local functions forward declarations for processing distributed table commands */ -static bool IsDistributedVacuumStmt(int vacuumOptions, List *vacuumRelationIdList); +static bool IsDistributedVacuumStmt(int vacuumOptions, List *VacuumCitusRelationIdList); static List * VacuumTaskList(Oid relationId, CitusVacuumParams vacuumParams, List *vacuumColumnList); static char * DeparseVacuumStmtPrefix(CitusVacuumParams vacuumParams); @@ -62,6 +62,8 @@ static List * ExtractVacuumTargetRels(VacuumStmt *vacuumStmt); static void ExecuteVacuumOnDistributedTables(VacuumStmt *vacuumStmt, List *relationIdList, CitusVacuumParams vacuumParams); static CitusVacuumParams VacuumStmtParams(VacuumStmt *vacstmt); +static List * VacuumCitusRelationIdList(VacuumStmt *vacuumStmt, CitusVacuumParams + vacuumParams); /* * PostprocessVacuumStmt processes vacuum statements that may need propagation to @@ -77,39 +79,82 @@ void PostprocessVacuumStmt(VacuumStmt *vacuumStmt, const char *vacuumCommand) { CitusVacuumParams vacuumParams = VacuumStmtParams(vacuumStmt); - LOCKMODE lockMode = (vacuumParams.options & VACOPT_FULL) ? AccessExclusiveLock : - ShareUpdateExclusiveLock; + const char *stmtName = (vacuumParams.options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE"; + /* + * No table in the vacuum statement means vacuuming all relations + * which is not supported by citus. + */ + if (list_length(vacuumStmt->rels) == 0) + { + /* WARN for unqualified VACUUM commands */ + ereport(WARNING, (errmsg("not propagating %s command to worker nodes", stmtName), + errhint("Provide a specific table in order to %s " + "distributed tables.", stmtName))); + } + + + List *citusRelationIdList = VacuumCitusRelationIdList(vacuumStmt, vacuumParams); + if (list_length(citusRelationIdList) == 0) + { + return; + } 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. + * of shard placements. We don't do this in case of "ANALYZE " command because + * its semantics are different than VACUUM and it doesn't acquire the global lock. */ CommitTransactionCommand(); StartTransactionCommand(); } + /* + * Here we get the relation list again because we might have + * closed the current transaction and the memory context got reset. + * Vacuum's context is PortalContext, which lasts for the whole session + * so committing/starting a new transaction doesn't affect it. + */ + citusRelationIdList = VacuumCitusRelationIdList(vacuumStmt, vacuumParams); + bool distributedVacuumStmt = IsDistributedVacuumStmt(vacuumParams.options, + citusRelationIdList); + if (!distributedVacuumStmt) + { + return; + } + + ExecuteVacuumOnDistributedTables(vacuumStmt, citusRelationIdList, vacuumParams); +} + + +/* + * VacuumCitusRelationIdList returns the oid of the relations in the given vacuum statement. + */ +static List * +VacuumCitusRelationIdList(VacuumStmt *vacuumStmt, CitusVacuumParams vacuumParams) +{ + LOCKMODE lockMode = (vacuumParams.options & VACOPT_FULL) ? AccessExclusiveLock : + ShareUpdateExclusiveLock; + List *vacuumRelationList = ExtractVacuumTargetRels(vacuumStmt); + List *relationIdList = NIL; RangeVar *vacuumRelation = NULL; foreach_ptr(vacuumRelation, vacuumRelationList) { Oid relationId = RangeVarGetRelid(vacuumRelation, lockMode, false); + if (!IsCitusTable(relationId)) + { + continue; + } relationIdList = lappend_oid(relationIdList, relationId); } - bool distributedVacuumStmt = IsDistributedVacuumStmt(vacuumParams.options, - relationIdList); - if (!distributedVacuumStmt) - { - return; - } - - ExecuteVacuumOnDistributedTables(vacuumStmt, relationIdList, vacuumParams); + return relationIdList; } @@ -165,27 +210,16 @@ ExecuteVacuumOnDistributedTables(VacuumStmt *vacuumStmt, List *relationIdList, * false otherwise. */ static bool -IsDistributedVacuumStmt(int vacuumOptions, List *vacuumRelationIdList) +IsDistributedVacuumStmt(int vacuumOptions, List *VacuumCitusRelationIdList) { - const char *stmtName = (vacuumOptions & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE"; bool distributeStmt = false; int distributedRelationCount = 0; - /* - * No table in the vacuum statement means vacuuming all relations - * which is not supported by citus. - */ - int vacuumedRelationCount = list_length(vacuumRelationIdList); - if (vacuumedRelationCount == 0) - { - /* WARN for unqualified VACUUM commands */ - ereport(WARNING, (errmsg("not propagating %s command to worker nodes", stmtName), - errhint("Provide a specific table in order to %s " - "distributed tables.", stmtName))); - } + const char *stmtName = (vacuumOptions & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE"; + Oid relationId = InvalidOid; - foreach_oid(relationId, vacuumRelationIdList) + foreach_oid(relationId, VacuumCitusRelationIdList) { if (OidIsValid(relationId) && IsCitusTable(relationId)) { diff --git a/src/test/regress/expected/single_node.out b/src/test/regress/expected/single_node.out index 4093e7c76..75b7eec03 100644 --- a/src/test/regress/expected/single_node.out +++ b/src/test/regress/expected/single_node.out @@ -117,6 +117,9 @@ VACUUM ref, test; VACUUM ANALYZE test(x); ANALYZE ref; ANALYZE test_2; +VACUUM local; +VACUUM local, ref, test, test_2; +VACUUM FULL test, ref; 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 afe4f0f44..c6a734e8a 100644 --- a/src/test/regress/sql/single_node.sql +++ b/src/test/regress/sql/single_node.sql @@ -63,6 +63,9 @@ VACUUM ref, test; VACUUM ANALYZE test(x); ANALYZE ref; ANALYZE test_2; +VACUUM local; +VACUUM local, ref, test, test_2; +VACUUM FULL test, ref; BEGIN; ALTER TABLE test ADD COLUMN z INT DEFAULT 66;