From 2890154420f7f9a785d98a6b172e93e82fee10f7 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Mon, 18 Jun 2018 14:56:55 +0300 Subject: [PATCH] Make sure that TRUNCATE always opens a DDL access --- .../executor/multi_router_executor.c | 17 ++++----- .../master/master_modify_multiple_shards.c | 16 ++++++-- .../regress/expected/multi_partitioning_0.out | 4 +- .../expected/relation_access_tracking.out | 37 +++++++++++++++---- .../regress/sql/relation_access_tracking.sql | 15 +++++++- 5 files changed, 63 insertions(+), 26 deletions(-) diff --git a/src/backend/distributed/executor/multi_router_executor.c b/src/backend/distributed/executor/multi_router_executor.c index ea07d541b..47dee258d 100644 --- a/src/backend/distributed/executor/multi_router_executor.c +++ b/src/backend/distributed/executor/multi_router_executor.c @@ -1316,17 +1316,14 @@ ExecuteModifyTasks(List *taskList, bool expectResults, ParamListInfo paramListIn /* * Ensure that there are no concurrent modifications on the same - * shards. For DDL commands, we already obtained the appropriate - * locks in ProcessUtility. - * - * We don't need to acquire lock for TRUNCATE_TASK since it already - * acquires AccessExclusiveLock on the relation, and blocks any - * concurrent operation. + * shards. In general, for DDL commands, we already obtained the + * appropriate locks in ProcessUtility. However, we still prefer to + * acquire the executor locks for DDLs specifically for TRUNCATE + * command on a partition table since AcquireExecutorMultiShardLocks() + * ensures that no concurrent modifications happens on the parent + * tables. */ - if (firstTask->taskType == MODIFY_TASK) - { - AcquireExecutorMultiShardLocks(taskList); - } + AcquireExecutorMultiShardLocks(taskList); BeginOrContinueCoordinatedTransaction(); diff --git a/src/backend/distributed/master/master_modify_multiple_shards.c b/src/backend/distributed/master/master_modify_multiple_shards.c index 8b4409d7f..b7b774594 100644 --- a/src/backend/distributed/master/master_modify_multiple_shards.c +++ b/src/backend/distributed/master/master_modify_multiple_shards.c @@ -55,7 +55,8 @@ #include "utils/memutils.h" -static List * ModifyMultipleShardsTaskList(Query *query, List *shardIntervalList); +static List * ModifyMultipleShardsTaskList(Query *query, List *shardIntervalList, TaskType + taskType); PG_FUNCTION_INFO_V1(master_modify_multiple_shards); @@ -85,6 +86,7 @@ master_modify_multiple_shards(PG_FUNCTION_ARGS) List *taskList = NIL; int32 affectedTupleCount = 0; CmdType operation = CMD_UNKNOWN; + TaskType taskType = TASK_TYPE_INVALID_FIRST; #if (PG_VERSION_NUM >= 100000) RawStmt *rawStmt = (RawStmt *) ParseTreeRawStmt(queryString); queryTreeNode = rawStmt->stmt; @@ -159,6 +161,12 @@ master_modify_multiple_shards(PG_FUNCTION_ARGS) { RaiseDeferredError(error, ERROR); } + + taskType = MODIFY_TASK; + } + else + { + taskType = DDL_TASK; } /* reject queries with a returning list */ @@ -179,7 +187,7 @@ master_modify_multiple_shards(PG_FUNCTION_ARGS) CHECK_FOR_INTERRUPTS(); taskList = - ModifyMultipleShardsTaskList(modifyQuery, prunedShardIntervalList); + ModifyMultipleShardsTaskList(modifyQuery, prunedShardIntervalList, taskType); if (MultiShardConnectionType == SEQUENTIAL_CONNECTION) { @@ -200,7 +208,7 @@ master_modify_multiple_shards(PG_FUNCTION_ARGS) * given list of shards. */ static List * -ModifyMultipleShardsTaskList(Query *query, List *shardIntervalList) +ModifyMultipleShardsTaskList(Query *query, List *shardIntervalList, TaskType taskType) { List *taskList = NIL; ListCell *shardIntervalCell = NULL; @@ -223,7 +231,7 @@ ModifyMultipleShardsTaskList(Query *query, List *shardIntervalList) task = CitusMakeNode(Task); task->jobId = jobId; task->taskId = taskId++; - task->taskType = MODIFY_TASK; + task->taskType = taskType; task->queryString = shardQueryString->data; task->dependedTaskList = NULL; task->replicationModel = REPLICATION_MODEL_INVALID; diff --git a/src/test/regress/expected/multi_partitioning_0.out b/src/test/regress/expected/multi_partitioning_0.out index 7fc346729..0ed0e0792 100644 --- a/src/test/regress/expected/multi_partitioning_0.out +++ b/src/test/regress/expected/multi_partitioning_0.out @@ -1122,13 +1122,11 @@ SELECT relation::regclass, locktype, mode FROM pg_locks WHERE relation::regclass partitioning_locks | relation | AccessShareLock partitioning_locks_2009 | relation | AccessExclusiveLock partitioning_locks_2009 | relation | AccessShareLock - partitioning_locks_2009 | relation | RowExclusiveLock partitioning_locks_2009 | relation | ShareLock partitioning_locks_2010 | relation | AccessExclusiveLock partitioning_locks_2010 | relation | AccessShareLock - partitioning_locks_2010 | relation | RowExclusiveLock partitioning_locks_2010 | relation | ShareLock -(10 rows) +(8 rows) COMMIT; -- test shard resource locks with master_modify_multiple_shards diff --git a/src/test/regress/expected/relation_access_tracking.out b/src/test/regress/expected/relation_access_tracking.out index 69ec80fbd..2a0f367d7 100644 --- a/src/test/regress/expected/relation_access_tracking.out +++ b/src/test/regress/expected/relation_access_tracking.out @@ -584,13 +584,34 @@ BEGIN; (2 rows) ROLLBACK; --- FIXME: TRUNCATE should be DDL +-- TRUNCATE should be DDL BEGIN; TRUNCATE table_1; SELECT * FROM relation_acesses WHERE table_name IN ('table_1') ORDER BY 1; - table_name | select_access | dml_access | ddl_access -------------+---------------+-----------------+-------------- - table_1 | not_accessed | parallel_access | not_accessed + table_name | select_access | dml_access | ddl_access +------------+---------------+--------------+----------------- + table_1 | not_accessed | not_accessed | parallel_access +(1 row) + +ROLLBACK; +-- TRUNCATE can be a sequential DDL +BEGIN; + SET LOCAL citus.multi_shard_modify_mode = 'sequential'; + TRUNCATE table_1; + SELECT * FROM relation_acesses WHERE table_name IN ('table_1') ORDER BY 1; + table_name | select_access | dml_access | ddl_access +------------+---------------+--------------+------------------- + table_1 | not_accessed | not_accessed | sequential_access +(1 row) + +ROLLBACK; +-- TRUNCATE on a reference table should be sequential +BEGIN; + TRUNCATE table_6; + SELECT * FROM relation_acesses WHERE table_name IN ('table_6') ORDER BY 1; + table_name | select_access | dml_access | ddl_access +------------+---------------+--------------+------------------- + table_6 | not_accessed | not_accessed | sequential_access (1 row) ROLLBACK; @@ -673,10 +694,10 @@ BEGIN; TRUNCATE table_1 CASCADE; NOTICE: truncate cascades to table "table_2" SELECT * FROM relation_acesses WHERE table_name IN ('table_1', 'table_2') ORDER BY 1; - table_name | select_access | dml_access | ddl_access -------------+---------------+-----------------+-------------- - table_1 | not_accessed | parallel_access | not_accessed - table_2 | not_accessed | parallel_access | not_accessed + table_name | select_access | dml_access | ddl_access +------------+---------------+--------------+----------------- + table_1 | not_accessed | not_accessed | parallel_access + table_2 | not_accessed | not_accessed | parallel_access (2 rows) ROLLBACK; diff --git a/src/test/regress/sql/relation_access_tracking.sql b/src/test/regress/sql/relation_access_tracking.sql index d3af36403..5b5e1c0df 100644 --- a/src/test/regress/sql/relation_access_tracking.sql +++ b/src/test/regress/sql/relation_access_tracking.sql @@ -351,12 +351,25 @@ BEGIN; SELECT * FROM relation_acesses WHERE table_name IN ('table_6', 'table_1'); ROLLBACK; --- FIXME: TRUNCATE should be DDL +-- TRUNCATE should be DDL BEGIN; TRUNCATE table_1; SELECT * FROM relation_acesses WHERE table_name IN ('table_1') ORDER BY 1; ROLLBACK; +-- TRUNCATE can be a sequential DDL +BEGIN; + SET LOCAL citus.multi_shard_modify_mode = 'sequential'; + TRUNCATE table_1; + SELECT * FROM relation_acesses WHERE table_name IN ('table_1') ORDER BY 1; +ROLLBACK; + +-- TRUNCATE on a reference table should be sequential +BEGIN; + TRUNCATE table_6; + SELECT * FROM relation_acesses WHERE table_name IN ('table_6') ORDER BY 1; +ROLLBACK; + -- creating foreign keys should consider adding the placement accesses for the referenced table ALTER TABLE table_1 ADD CONSTRAINT table_1_u UNIQUE (key); BEGIN;