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.

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,
+static List * GetModifyConnections(Task *task, bool markCritical);
-								   bool startedInTransaction);
 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,
+	connectionList = GetModifyConnections(task, taskRequiresTwoPhaseCommit);
-										  taskRequiresTwoPhaseCommit,
-										  startedInTransaction);
 
 	/* 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
+ * If markCritical is true remote transactions are marked as critical.
- * noNewTransactions is true, this function errors out if there's no
- * transaction in progress.
  */
 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
+		 * If we're expanding the set nodes that participate in the distributed
-		 * transactions. That prevents some forms of distributed deadlocks.
+		 * transaction, conform to MultiShardCommitProtocol.
 		 */
-		if (noNewTransactions)
+		if (MultiShardCommitProtocol == COMMIT_PROTOCOL_2PC &&
+			InCoordinatedTransaction() &&
+			XactModificationLevel == XACT_MODIFICATION_DATA)
 		{
 			RemoteTransaction *transaction = &multiConnection->remoteTransaction;
-
+			if (transaction->transactionState == REMOTE_TRANS_INVALID)
-			if (EnableDeadlockPrevention &&
-				transaction->transactionState == REMOTE_TRANS_INVALID)
 			{
-				ereport(ERROR, (errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
+				CoordinatedTransactionUse2PC();
-								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.")));
 			}
 		}
 

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

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;
@@ -205,8 +205,9 @@ SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='public.labs'::
 
 SELECT * FROM labs WHERE id = 6;
  id |   name    
-----+------
+----+-----------
-(0 rows)
+  6 | Bell Labs
+(1 row)
 
 -- COPY can happen after single row INSERT
 BEGIN;
@@ -273,8 +274,9 @@ COMMIT;
 -- verify rollback
 SELECT * FROM researchers WHERE lab_id = 6;
  id | lab_id |      name      
-----+--------+------
+----+--------+----------------
-(0 rows)
+  9 |      6 | Leslie Lamport
+(1 row)
 
 SELECT count(*) FROM pg_dist_transaction;
  count 
@@ -300,8 +302,9 @@ COMMIT;
 -- verify rollback
 SELECT * FROM researchers WHERE lab_id = 6;
  id | lab_id |      name      
-----+--------+------
+----+--------+----------------
-(0 rows)
+  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;

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;

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;

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');