Actually skip constraint validation on shards

In eadc88a we started completely skipping foreign key constraint
validation at the end of a non blocking shard move, instead of only for
foreign keys to reference tables. However, it turns out that this didn't
work at all because of a hard to notice bug: By resetting the
SkipConstraintValidation flag at the end of our utility hook, we
actually make the SET command that sets it a no-op.

This fixes that bug by removing the code that resets it. This is fine
because #6543 removed the only place where we set the flag in C code. So
the resetting of the flag has no purpose anymore. This PR also adds a
regression test, because it turned out we didn't have any otherwise we
would have caught that the feature was completely broken.

It also moves the constraint validation skipping to the utility hook.
The reason is that #6543 showed us that this is the better place to skip
it, because it will also skip the planning phase and not just the
execution.

src/backend/distributed/commands/foreign_constraint.c

@@ -1311,19 +1311,6 @@ IsTableTypeIncluded(Oid relationId, int flags)
 }
 
 
-/*
- * EnableSkippingConstraintValidation is simply a C interface for setting the following:
- *      SET LOCAL citus.skip_constraint_validation TO on;
- */
-void
-EnableSkippingConstraintValidation()
-{
-	set_config_option("citus.skip_constraint_validation", "true",
-					  PGC_SUSET, PGC_S_SESSION,
-					  GUC_ACTION_LOCAL, true, 0, false);
-}
-
-
 /*
  * RelationInvolvedInAnyNonInheritedForeignKeys returns true if relation involved
  * in a foreign key that is not inherited from its parent relation.

src/backend/distributed/commands/utility_hook.c

@@ -389,7 +389,6 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 	Node *parsetree = pstmt->utilityStmt;
 	List *ddlJobs = NIL;
 	DistOpsValidationState distOpsValidationState = HasNoneValidObject;
-	bool oldSkipConstraintsValidationValue = SkipConstraintValidation;
 
 	if (IsA(parsetree, ExplainStmt) &&
 		IsA(((ExplainStmt *) parsetree)->query, Query))
@@ -634,6 +633,32 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 		}
 	}
 
+	/*
+	 * If we've explicitly set the citus.skip_constraint_validation GUC, then
+	 * we skip validation of any added constraints.
+	 */
+	if (IsA(parsetree, AlterTableStmt) && SkipConstraintValidation)
+	{
+		AlterTableStmt *alterTableStmt = (AlterTableStmt *) parsetree;
+		AlterTableCmd *command = NULL;
+		foreach_ptr(command, alterTableStmt->cmds)
+		{
+			AlterTableType alterTableType = command->subtype;
+
+			/*
+			 * XXX: In theory we could probably use this GUC to skip validation
+			 * of VALIDATE CONSTRAINT and ALTER CONSTRAINT too. But currently
+			 * this is not needed, so we make its behaviour only apply to ADD
+			 * CONSTRAINT.
+			 */
+			if (alterTableType == AT_AddConstraint)
+			{
+				Constraint *constraint = (Constraint *) command->def;
+				constraint->skip_validation = true;
+			}
+		}
+	}
+
 	/* inform the user about potential caveats */
 	if (IsA(parsetree, CreatedbStmt))
 	{
@@ -915,8 +940,6 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 		 */
 		CitusHasBeenLoaded(); /* lgtm[cpp/return-value-ignored] */
 	}
-
-	SkipConstraintValidation = oldSkipConstraintsValidationValue;
 }
 
 

src/backend/distributed/executor/multi_executor.c

@@ -861,11 +861,6 @@ AlterTableConstraintCheck(QueryDesc *queryDesc)
 		return false;
 	}
 
-	if (SkipConstraintValidation)
-	{
-		return true;
-	}
-
 	/*
 	 * While an ALTER TABLE is in progress, we might do SELECTs on some
 	 * catalog tables too. For example, when dropping a column, citus_drop_trigger()

src/include/distributed/commands.h

@@ -287,7 +287,6 @@ extern bool TableHasExternalForeignKeys(Oid relationId);
 extern List * GetForeignKeyOids(Oid relationId, int flags);
 extern Oid GetReferencedTableId(Oid foreignKeyId);
 extern Oid GetReferencingTableId(Oid foreignKeyId);
-extern void EnableSkippingConstraintValidation(void);
 extern bool RelationInvolvedInAnyNonInheritedForeignKeys(Oid relationId);
 
 

src/test/regress/enterprise_isolation_logicalrep_2_schedule

@@ -7,3 +7,4 @@ test: isolation_cluster_management
 
 test: isolation_logical_replication_single_shard_commands_on_mx
 test: isolation_logical_replication_multi_shard_commands_on_mx
+test: isolation_logical_replication_skip_fk_validation

src/test/regress/expected/isolation_logical_replication_skip_fk_validation.out

@@ -0,0 +1,52 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1-start-session-level-connection s3-acquire-advisory-lock s2-move-placement s1-start-session-level-connection s1-insert-violation-into-shard s3-release-advisory-lock
+step s1-start-session-level-connection:
+ SELECT start_session_level_connection_to_node('localhost', 57638);
+
+start_session_level_connection_to_node
+---------------------------------------------------------------------
+
+(1 row)
+
+step s3-acquire-advisory-lock:
+ SELECT pg_advisory_lock(44000, 55152);
+
+pg_advisory_lock
+---------------------------------------------------------------------
+
+(1 row)
+
+step s2-move-placement:
+ SELECT master_move_shard_placement((SELECT * FROM selected_shard_for_test_table), 'localhost', 57637, 'localhost', 57638);
+ <waiting ...>
+step s1-start-session-level-connection: 
+ SELECT start_session_level_connection_to_node('localhost', 57638);
+
+start_session_level_connection_to_node
+---------------------------------------------------------------------
+
+(1 row)
+
+step s1-insert-violation-into-shard:
+ SELECT run_commands_on_session_level_connection_to_node(format('INSERT INTO t1_%s VALUES (-1, -1)', (SELECT * FROM selected_shard_for_test_table)));
+
+run_commands_on_session_level_connection_to_node
+---------------------------------------------------------------------
+
+(1 row)
+
+step s3-release-advisory-lock:
+ SELECT pg_advisory_unlock(44000, 55152);
+
+pg_advisory_unlock
+---------------------------------------------------------------------
+t
+(1 row)
+
+step s2-move-placement: <... completed>
+master_move_shard_placement
+---------------------------------------------------------------------
+
+(1 row)
+

src/test/regress/spec/isolation_logical_replication_skip_fk_validation.spec

@@ -0,0 +1,72 @@
+#include "isolation_mx_common.include.spec"
+
+setup
+{
+  SET citus.shard_count to 1;
+  SET citus.shard_replication_factor to 1;
+
+  CREATE TABLE t1 (id int PRIMARY KEY, value int);
+  SELECT create_distributed_table('t1', 'id');
+
+  CREATE TABLE t2 (id int PRIMARY KEY, value int);
+  SELECT create_distributed_table('t2', 'id');
+
+  CREATE TABLE r (id int PRIMARY KEY, value int);
+  SELECT create_reference_table('r');
+  SELECT get_shard_id_for_distribution_column('t1', 5) INTO selected_shard_for_test_table;
+}
+
+setup {
+  ALTER TABLE t1 ADD CONSTRAINT t1_t2_fkey FOREIGN KEY (id) REFERENCES t2(id);
+}
+
+setup {
+  ALTER TABLE t1 ADD CONSTRAINT t1_r_fkey FOREIGN KEY (value) REFERENCES r(id);
+}
+
+teardown
+{
+  DROP TABLE t1;
+  DROP TABLE t2;
+  DROP TABLE r;
+  DROP TABLE selected_shard_for_test_table;
+}
+
+session "s1"
+
+step "s1-start-session-level-connection"
+{
+	SELECT start_session_level_connection_to_node('localhost', 57638);
+}
+
+// This inserts a foreign key violation directly into the shard on the target
+// worker. Since we're not validating the foreign key on the new shard on
+// purpose we expect no errors.
+step "s1-insert-violation-into-shard"
+{
+	SELECT run_commands_on_session_level_connection_to_node(format('INSERT INTO t1_%s VALUES (-1, -1)', (SELECT * FROM selected_shard_for_test_table)));
+}
+
+session "s2"
+
+step "s2-move-placement"
+{
+	SELECT master_move_shard_placement((SELECT * FROM selected_shard_for_test_table), 'localhost', 57637, 'localhost', 57638);
+}
+
+session "s3"
+
+// this advisory lock with (almost) random values are only used
+// for testing purposes. For details, check Citus' logical replication
+// source code
+step "s3-acquire-advisory-lock"
+{
+	SELECT pg_advisory_lock(44000, 55152);
+}
+
+step "s3-release-advisory-lock"
+{
+	SELECT pg_advisory_unlock(44000, 55152);
+}
+
+permutation "s1-start-session-level-connection" "s3-acquire-advisory-lock" "s2-move-placement" "s1-start-session-level-connection" "s1-insert-violation-into-shard" "s3-release-advisory-lock"