From be4fc45c036f1ded563432ba618c0a6b12143f1d Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Fri, 11 Aug 2017 11:36:31 +0300 Subject: [PATCH] Deprecate enable_deadlock_prevention flag Now that we already have the necessary infrastructure for detecting distributed deadlocks. Thus, we don't need enable_deadlock_prevention which is purely intended for preventing some forms of distributed deadlocks. --- .../executor/multi_router_executor.c | 36 ++++++--------- src/backend/distributed/shared_library_init.c | 15 +++++- .../expected/multi_modifying_xacts.out | 46 ++++++++++--------- .../expected/multi_mx_modifying_xacts.out | 10 ++-- .../regress/sql/multi_modifying_xacts.sql | 7 +-- .../regress/sql/multi_mx_modifying_xacts.sql | 4 +- 6 files changed, 58 insertions(+), 60 deletions(-) diff --git a/src/backend/distributed/executor/multi_router_executor.c b/src/backend/distributed/executor/multi_router_executor.c index 3eada7eb7..5b6def0fc 100644 --- a/src/backend/distributed/executor/multi_router_executor.c +++ b/src/backend/distributed/executor/multi_router_executor.c @@ -70,6 +70,8 @@ /* controls use of locks to enforce safe commutativity */ bool AllModificationsCommutative = false; + +/* we've deprecated this flag, keeping here for some time not to break existing users */ bool EnableDeadlockPrevention = true; /* functions needed during run phase */ @@ -79,8 +81,7 @@ static ShardPlacementAccess * CreatePlacementAccess(ShardPlacement *placement, static void ExecuteSingleModifyTask(CitusScanState *scanState, Task *task, bool expectResults); static void ExecuteSingleSelectTask(CitusScanState *scanState, Task *task); -static List * GetModifyConnections(Task *task, bool markCritical, - bool startedInTransaction); +static List * GetModifyConnections(Task *task, bool markCritical); static void ExecuteMultipleTasks(CitusScanState *scanState, List *taskList, bool isModificationQuery, bool expectResults); static int64 ExecuteModifyTasks(List *taskList, bool expectResults, @@ -680,8 +681,6 @@ ExecuteSingleModifyTask(CitusScanState *scanState, Task *task, bool expectResult char *queryString = task->queryString; bool taskRequiresTwoPhaseCommit = (task->replicationModel == REPLICATION_MODEL_2PC); - bool startedInTransaction = - InCoordinatedTransaction() && XactModificationLevel == XACT_MODIFICATION_DATA; /* * Modifications for reference tables are always done using 2PC. First @@ -711,9 +710,7 @@ ExecuteSingleModifyTask(CitusScanState *scanState, Task *task, bool expectResult * establish the connection, mark as critical (when modifying reference * table) and start a transaction (when in a transaction). */ - connectionList = GetModifyConnections(task, - taskRequiresTwoPhaseCommit, - startedInTransaction); + connectionList = GetModifyConnections(task, taskRequiresTwoPhaseCommit); /* prevent replicas of the same shard from diverging */ AcquireExecutorShardLock(task, operation); @@ -809,12 +806,10 @@ ExecuteSingleModifyTask(CitusScanState *scanState, Task *task, bool expectResult * modify commands on the placements in tasPlacementList. If necessary remote * transactions are started. * - * If markCritical is true remote transactions are marked as critical. If - * noNewTransactions is true, this function errors out if there's no - * transaction in progress. + * If markCritical is true remote transactions are marked as critical. */ static List * -GetModifyConnections(Task *task, bool markCritical, bool noNewTransactions) +GetModifyConnections(Task *task, bool markCritical) { List *taskPlacementList = task->taskPlacementList; ListCell *taskPlacementCell = NULL; @@ -844,22 +839,17 @@ GetModifyConnections(Task *task, bool markCritical, bool noNewTransactions) NULL); /* - * If already in a transaction, disallow expanding set of remote - * transactions. That prevents some forms of distributed deadlocks. + * If we're expanding the set nodes that participate in the distributed + * transaction, conform to MultiShardCommitProtocol. */ - if (noNewTransactions) + if (MultiShardCommitProtocol == COMMIT_PROTOCOL_2PC && + InCoordinatedTransaction() && + XactModificationLevel == XACT_MODIFICATION_DATA) { RemoteTransaction *transaction = &multiConnection->remoteTransaction; - - if (EnableDeadlockPrevention && - transaction->transactionState == REMOTE_TRANS_INVALID) + if (transaction->transactionState == REMOTE_TRANS_INVALID) { - ereport(ERROR, (errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST), - errmsg("no transaction participant matches %s:%d", - taskPlacement->nodeName, taskPlacement->nodePort), - errdetail("Transactions which modify distributed tables " - "may only target nodes affected by the " - "modification command which began the transaction."))); + CoordinatedTransactionUse2PC(); } } diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index fe842faca..359caf1f4 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -59,6 +59,7 @@ void _PG_init(void); static void CreateRequiredDirectories(void); static void RegisterCitusConfigVariables(void); +static void WarningForEnableDeadlockPrevention(bool newval, void *extra); static void NormalizeWorkerListPath(void); @@ -379,7 +380,7 @@ RegisterCitusConfigVariables(void) true, PGC_USERSET, GUC_NO_SHOW_ALL, - NULL, NULL, NULL); + NULL, WarningForEnableDeadlockPrevention, NULL); DefineCustomBoolVariable( "citus.enable_ddl_propagation", @@ -737,6 +738,18 @@ RegisterCitusConfigVariables(void) } +/* + * Inform the users about the deprecated flag. + */ +static void +WarningForEnableDeadlockPrevention(bool newval, void *extra) +{ + ereport(WARNING, (errcode(ERRCODE_WARNING_DEPRECATED_FEATURE), + errmsg("citus.enable_deadlock_prevention is deprecated and it has " + "no effect. The flag will be removed in the next release."))); +} + + /* * NormalizeWorkerListPath converts the path configured via * citus.worker_list_file into an absolute path, falling back to the default diff --git a/src/test/regress/expected/multi_modifying_xacts.out b/src/test/regress/expected/multi_modifying_xacts.out index 1262e0ec8..986007d02 100644 --- a/src/test/regress/expected/multi_modifying_xacts.out +++ b/src/test/regress/expected/multi_modifying_xacts.out @@ -145,18 +145,18 @@ SELECT * FROM researchers, labs WHERE labs.id = researchers.lab_id; 8 | 5 | Douglas Engelbart | 5 | Los Alamos (1 row) --- but not the other way around (would require expanding xact participants)... +-- and the other way around is also allowed BEGIN; INSERT INTO labs VALUES (6, 'Bell Labs'); INSERT INTO researchers VALUES (9, 6, 'Leslie Lamport'); -ERROR: no transaction participant matches localhost:57638 -DETAIL: Transactions which modify distributed tables may only target nodes affected by the modification command which began the transaction. COMMIT; --- unless we disable deadlock prevention +-- we should be able to expand the transaction participants BEGIN; -SET citus.enable_deadlock_prevention TO off; INSERT INTO labs VALUES (6, 'Bell Labs'); INSERT INTO researchers VALUES (9, 6, 'Leslie Lamport'); +ERROR: duplicate key value violates unique constraint "avoid_name_confusion_idx_1200001" +DETAIL: Key (lab_id, name)=(6, Leslie Lamport) already exists. +CONTEXT: while executing command on localhost:57638 ABORT; -- SELECTs may occur after a modification: First check that selecting -- from the modified node works. @@ -165,7 +165,7 @@ INSERT INTO labs VALUES (6, 'Bell Labs'); SELECT count(*) FROM researchers WHERE lab_id = 6; count ------- - 0 + 1 (1 row) ABORT; @@ -181,7 +181,7 @@ INSERT INTO labs VALUES (6, 'Bell Labs'); SELECT count(*) FROM researchers WHERE lab_id = 6; count ------- - 0 + 1 (1 row) ABORT; @@ -204,9 +204,10 @@ SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='public.labs':: (2 rows) SELECT * FROM labs WHERE id = 6; - id | name -----+------ -(0 rows) + id | name +----+----------- + 6 | Bell Labs +(1 row) -- COPY can happen after single row INSERT BEGIN; @@ -272,9 +273,10 @@ DETAIL: Key (lab_id, name)=(6, 'Bjarne Stroustrup') already exists. COMMIT; -- verify rollback SELECT * FROM researchers WHERE lab_id = 6; - id | lab_id | name -----+--------+------ -(0 rows) + id | lab_id | name +----+--------+---------------- + 9 | 6 | Leslie Lamport +(1 row) SELECT count(*) FROM pg_dist_transaction; count @@ -299,9 +301,10 @@ DETAIL: Key (lab_id, name)=(6, 'Bjarne Stroustrup') already exists. COMMIT; -- verify rollback SELECT * FROM researchers WHERE lab_id = 6; - id | lab_id | name -----+--------+------ -(0 rows) + id | lab_id | name +----+--------+---------------- + 9 | 6 | Leslie Lamport +(1 row) SELECT count(*) FROM pg_dist_transaction; count @@ -317,9 +320,10 @@ COMMIT; SELECT * FROM researchers WHERE lab_id = 6; id | lab_id | name ----+--------+---------------------- + 9 | 6 | Leslie Lamport 17 | 6 | 'Bjarne Stroustrup' 18 | 6 | 'Dennis Ritchie' -(2 rows) +(3 rows) -- verify 2pc SELECT count(*) FROM pg_dist_transaction; @@ -376,9 +380,10 @@ ERROR: could not commit transaction on any active node SELECT * FROM researchers WHERE lab_id = 6; id | lab_id | name ----+--------+---------------------- + 9 | 6 | Leslie Lamport 17 | 6 | 'Bjarne Stroustrup' 18 | 6 | 'Dennis Ritchie' -(2 rows) +(3 rows) -- cleanup triggers and the function SELECT * from run_command_on_placements('researchers', 'drop trigger reject_large_researcher_id on %s') @@ -429,7 +434,7 @@ ALTER TABLE labs ADD COLUMN motto text; SELECT master_modify_multiple_shards('DELETE FROM labs'); master_modify_multiple_shards ------------------------------- - 7 + 8 (1 row) ALTER TABLE labs ADD COLUMN score float; @@ -909,16 +914,13 @@ SELECT create_distributed_table('hash_modifying_xacts', 'key'); BEGIN; INSERT INTO hash_modifying_xacts VALUES (1, 1); INSERT INTO reference_modifying_xacts VALUES (10, 10); -ERROR: no transaction participant matches localhost:57638 COMMIT; -- it is allowed when turning off deadlock prevention BEGIN; -SET citus.enable_deadlock_prevention TO off; INSERT INTO hash_modifying_xacts VALUES (1, 1); INSERT INTO reference_modifying_xacts VALUES (10, 10); ABORT; BEGIN; -SET citus.enable_deadlock_prevention TO off; INSERT INTO hash_modifying_xacts VALUES (1, 1); INSERT INTO hash_modifying_xacts VALUES (2, 2); ABORT; diff --git a/src/test/regress/expected/multi_mx_modifying_xacts.out b/src/test/regress/expected/multi_mx_modifying_xacts.out index 433fddc0d..40d879eb6 100644 --- a/src/test/regress/expected/multi_mx_modifying_xacts.out +++ b/src/test/regress/expected/multi_mx_modifying_xacts.out @@ -136,12 +136,10 @@ SELECT * FROM researchers_mx, labs_mx WHERE labs_mx.id = researchers_mx.lab_id; 8 | 5 | Douglas Engelbart | 5 | Los Alamos (1 row) --- but not the other way around (would require expanding xact participants)... +-- and the other way around is also allowed BEGIN; INSERT INTO labs_mx VALUES (6, 'Bell labs_mx'); INSERT INTO researchers_mx VALUES (9, 6, 'Leslie Lamport'); -ERROR: no transaction participant matches localhost:57638 -DETAIL: Transactions which modify distributed tables may only target nodes affected by the modification command which began the transaction. COMMIT; -- have the same test on the other worker node \c - - - :worker_2_port @@ -159,12 +157,10 @@ SELECT * FROM researchers_mx, labs_mx WHERE labs_mx.id = researchers_mx.lab_id; 8 | 5 | Douglas Engelbart | 5 | Los Alamos (4 rows) --- but not the other way around (would require expanding xact participants)... +-- and the other way around is also allowed BEGIN; INSERT INTO labs_mx VALUES (6, 'Bell labs_mx'); INSERT INTO researchers_mx VALUES (9, 6, 'Leslie Lamport'); -ERROR: no transaction participant matches localhost:57638 -DETAIL: Transactions which modify distributed tables may only target nodes affected by the modification command which began the transaction. COMMIT; -- switch back to the worker node \c - - - :worker_1_port @@ -175,7 +171,7 @@ INSERT INTO labs_mx VALUES (6, 'Bell labs_mx'); SELECT count(*) FROM researchers_mx WHERE lab_id = 6; count ------- - 0 + 2 (1 row) ABORT; diff --git a/src/test/regress/sql/multi_modifying_xacts.sql b/src/test/regress/sql/multi_modifying_xacts.sql index 395a1ed98..1b4d84b9b 100644 --- a/src/test/regress/sql/multi_modifying_xacts.sql +++ b/src/test/regress/sql/multi_modifying_xacts.sql @@ -113,15 +113,14 @@ COMMIT; SELECT * FROM researchers, labs WHERE labs.id = researchers.lab_id; --- but not the other way around (would require expanding xact participants)... +-- and the other way around is also allowed BEGIN; INSERT INTO labs VALUES (6, 'Bell Labs'); INSERT INTO researchers VALUES (9, 6, 'Leslie Lamport'); COMMIT; --- unless we disable deadlock prevention +-- we should be able to expand the transaction participants BEGIN; -SET citus.enable_deadlock_prevention TO off; INSERT INTO labs VALUES (6, 'Bell Labs'); INSERT INTO researchers VALUES (9, 6, 'Leslie Lamport'); ABORT; @@ -703,13 +702,11 @@ COMMIT; -- it is allowed when turning off deadlock prevention BEGIN; -SET citus.enable_deadlock_prevention TO off; INSERT INTO hash_modifying_xacts VALUES (1, 1); INSERT INTO reference_modifying_xacts VALUES (10, 10); ABORT; BEGIN; -SET citus.enable_deadlock_prevention TO off; INSERT INTO hash_modifying_xacts VALUES (1, 1); INSERT INTO hash_modifying_xacts VALUES (2, 2); ABORT; diff --git a/src/test/regress/sql/multi_mx_modifying_xacts.sql b/src/test/regress/sql/multi_mx_modifying_xacts.sql index 508fe7e6d..5182ca3fb 100644 --- a/src/test/regress/sql/multi_mx_modifying_xacts.sql +++ b/src/test/regress/sql/multi_mx_modifying_xacts.sql @@ -118,7 +118,7 @@ COMMIT; SELECT * FROM researchers_mx, labs_mx WHERE labs_mx.id = researchers_mx.lab_id; --- but not the other way around (would require expanding xact participants)... +-- and the other way around is also allowed BEGIN; INSERT INTO labs_mx VALUES (6, 'Bell labs_mx'); INSERT INTO researchers_mx VALUES (9, 6, 'Leslie Lamport'); @@ -134,7 +134,7 @@ COMMIT; SELECT * FROM researchers_mx, labs_mx WHERE labs_mx.id = researchers_mx.lab_id; --- but not the other way around (would require expanding xact participants)... +-- and the other way around is also allowed BEGIN; INSERT INTO labs_mx VALUES (6, 'Bell labs_mx'); INSERT INTO researchers_mx VALUES (9, 6, 'Leslie Lamport');