Revert "Introduce GUC citus.skip_constraint_validation (#6281)"

This reverts commit eadc88a800.

src/backend/distributed/commands/cascade_table_operation_for_connected_relations.c

@@ -623,11 +623,11 @@ ExecuteForeignKeyCreateCommand(const char *commandString, bool skip_validation)
 	 */
 	Assert(IsA(parseTree, AlterTableStmt));
 
-	bool oldSkipConstraintsValidationValue = SkipConstraintValidation;
-
 	if (skip_validation && IsA(parseTree, AlterTableStmt))
 	{
-		EnableSkippingConstraintValidation();
+		parseTree =
+			SkipForeignKeyValidationIfConstraintIsFkey((AlterTableStmt *) parseTree,
+													   true);
 
 		ereport(DEBUG4, (errmsg("skipping validation for foreign key create "
 								"command \"%s\"", commandString)));
@@ -635,6 +635,4 @@ ExecuteForeignKeyCreateCommand(const char *commandString, bool skip_validation)
 
 	ProcessUtilityParseTree(parseTree, commandString, PROCESS_UTILITY_QUERY,
 							NULL, None_Receiver, NULL);
-
-	SkipConstraintValidation = oldSkipConstraintsValidationValue;
 }

src/backend/distributed/commands/foreign_constraint.c

@@ -32,7 +32,6 @@
 #include "distributed/reference_table_utils.h"
 #include "distributed/utils/array_type.h"
 #include "distributed/version_compat.h"
-#include "miscadmin.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/inval.h"
@@ -83,6 +82,7 @@ static List * GetForeignKeyIdsForColumn(char *columnName, Oid relationId,
 										int searchForeignKeyColumnFlags);
 static List * GetForeignKeysWithLocalTables(Oid relationId);
 static bool IsTableTypeIncluded(Oid relationId, int flags);
+static void UpdateConstraintIsValid(Oid constraintId, bool isValid);
 
 
 /*
@@ -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/table.c

@@ -1872,44 +1872,52 @@ PreprocessAlterTableSchemaStmt(Node *node, const char *queryString,
  * statement to be worked on the distributed table. Currently, it only processes
  * ALTER TABLE ... ADD FOREIGN KEY command to skip the validation step.
  */
-void
+Node *
-SkipForeignKeyValidationIfConstraintIsFkey(AlterTableStmt *alterTableStatement)
+SkipForeignKeyValidationIfConstraintIsFkey(AlterTableStmt *alterTableStatement,
+										   bool processLocalRelation)
 {
 	/* first check whether a distributed relation is affected */
 	if (alterTableStatement->relation == NULL)
 	{
-		return;
+		return (Node *) alterTableStatement;
 	}
 
 	LOCKMODE lockmode = AlterTableGetLockLevel(alterTableStatement->cmds);
 	Oid leftRelationId = AlterTableLookupRelation(alterTableStatement, lockmode);
 	if (!OidIsValid(leftRelationId))
 	{
-		return;
+		return (Node *) alterTableStatement;
 	}
 
-	if (!IsCitusTable(leftRelationId))
+	if (!IsCitusTable(leftRelationId) && !processLocalRelation)
 	{
-		return;
+		return (Node *) alterTableStatement;
 	}
 
+	/*
+	 * We check if there is a ADD FOREIGN CONSTRAINT command in sub commands list.
+	 * If there is we assign referenced releation id to rightRelationId and we also
+	 * set skip_validation to true to prevent PostgreSQL to verify validity of the
+	 * foreign constraint in master. Validity will be checked in workers anyway.
+	 */
+	List *commandList = alterTableStatement->cmds;
 	AlterTableCmd *command = NULL;
-	foreach_ptr(command, alterTableStatement->cmds)
+	foreach_ptr(command, commandList)
 	{
 		AlterTableType alterTableType = command->subtype;
 
 		if (alterTableType == AT_AddConstraint)
 		{
-			/* skip only if the constraint is a foreign key */
 			Constraint *constraint = (Constraint *) command->def;
 			if (constraint->contype == CONSTR_FOREIGN)
 			{
-				/* set the GUC skip_constraint_validation to on */
+				/* foreign constraint validations will be done in shards. */
-				EnableSkippingConstraintValidation();
+				constraint->skip_validation = true;
-				return;
 			}
 		}
 	}
+
+	return (Node *) alterTableStatement;
 }
 
 

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))
@@ -608,7 +607,9 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 		 * Citus intervening. The only exception is partition column drop, in
 		 * which case we error out. Advanced Citus users use this to implement their
 		 * own DDL propagation. We also use it to avoid re-propagating DDL commands
-		 * when changing MX tables on workers.
+		 * when changing MX tables on workers. Below, we also make sure that DDL
+		 * commands don't run queries that might get intercepted by Citus and error
+		 * out, specifically we skip validation in foreign keys.
 		 */
 
 		if (IsA(parsetree, AlterTableStmt))
@@ -627,7 +628,8 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 				 * Note validation is done on the shard level when DDL propagation
 				 * is enabled. The following eagerly executes some tasks on workers.
 				 */
-				SkipForeignKeyValidationIfConstraintIsFkey(alterTableStmt);
+				parsetree =
+					SkipForeignKeyValidationIfConstraintIsFkey(alterTableStmt, false);
 			}
 		}
 	}
@@ -913,8 +915,6 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 		 */
 		CitusHasBeenLoaded(); /* lgtm[cpp/return-value-ignored] */
 	}
-
-	SkipConstraintValidation = oldSkipConstraintsValidationValue;
 }
 
 

src/backend/distributed/executor/multi_executor.c

@@ -64,11 +64,6 @@ int MultiShardConnectionType = PARALLEL_CONNECTION;
 bool WritableStandbyCoordinator = false;
 bool AllowModificationsFromWorkersToReplicatedTables = true;
 
-/*
- * Controlled by the GUC citus.skip_constraint_validation
- */
-bool SkipConstraintValidation = false;
-
 /*
  * Setting that controls whether distributed queries should be
  * allowed within a task execution.
@@ -861,11 +856,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/backend/distributed/shared_library_init.c

@@ -2188,21 +2188,6 @@ RegisterCitusConfigVariables(void)
 		GUC_NO_SHOW_ALL,
 		NULL, NULL, NULL);
 
-	DefineCustomBoolVariable(
-		"citus.skip_constraint_validation",
-		gettext_noop("Skip validation of constraints"),
-		gettext_noop("Validating constraints is a costly operation which effects Citus' "
-					 "performance negatively. With this GUC set to true, we skip "
-					 "validating them. Constraint validation can be redundant for some "
-					 "cases. For instance, when moving a shard, which has already "
-					 "validated constraints at the source; we don't need to validate "
-					 "the constraints again at the destination."),
-		&SkipConstraintValidation,
-		false,
-		PGC_SUSET,
-		0,
-		NULL, NULL, NULL);
-
 	DefineCustomBoolVariable(
 		"citus.skip_jsonb_validation_in_copy",
 		gettext_noop("Skip validation of JSONB columns on the coordinator during COPY "

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);
 
 
@@ -546,7 +545,8 @@ extern List * PreprocessAlterTableMoveAllStmt(Node *node, const char *queryStrin
 											  ProcessUtilityContext processUtilityContext);
 extern List * PreprocessAlterTableSchemaStmt(Node *node, const char *queryString,
 											 ProcessUtilityContext processUtilityContext);
-extern void SkipForeignKeyValidationIfConstraintIsFkey(AlterTableStmt *alterTableStmt);
+extern Node * SkipForeignKeyValidationIfConstraintIsFkey(AlterTableStmt *alterTableStmt,
+														 bool processLocalRelation);
 extern bool IsAlterTableRenameStmt(RenameStmt *renameStmt);
 extern void ErrorIfAlterDropsPartitionColumn(AlterTableStmt *alterTableStatement);
 extern void PostprocessAlterTableStmt(AlterTableStmt *pStmt);

src/include/distributed/multi_executor.h

@@ -62,7 +62,6 @@ typedef struct TransactionProperties
 
 
 extern bool AllowNestedDistributedExecution;
-extern bool SkipConstraintValidation;
 extern int MultiShardConnectionType;
 extern bool WritableStandbyCoordinator;
 extern bool AllowModificationsFromWorkersToReplicatedTables;

src/test/regress/expected/foreign_key_to_reference_shard_rebalance.out

@@ -182,34 +182,13 @@ SELECT conname, contype, convalidated FROM pg_constraint WHERE conrelid = 'fkey_
 (3 rows)
 
 \c - - - :master_port
-SET search_path TO fkey_to_reference_shard_rebalance;
-SET citus.shard_replication_factor to 1;
--- test moving a shard with foreign key
-create table ref_table_with_fkey (id int primary key);
-select create_reference_table('ref_table_with_fkey');
- create_reference_table
----------------------------------------------------------------------
-
-(1 row)
-
-insert into ref_table_with_fkey select s from generate_series(0,9) s;
-create table partitioned_tbl_with_fkey (x int, y int, t timestamptz default now()) partition by range (t);
-select create_distributed_table('partitioned_tbl_with_fkey','x');
- create_distributed_table
----------------------------------------------------------------------
-
-(1 row)
-
-create table partition_1_with_fkey partition of partitioned_tbl_with_fkey for values from ('2022-01-01') to ('2022-12-31');
-create table partition_2_with_fkey partition of partitioned_tbl_with_fkey for values from ('2023-01-01') to ('2023-12-31');
-insert into partitioned_tbl_with_fkey (x,y) select s,s%10 from generate_series(1,100) s;
-ALTER TABLE partitioned_tbl_with_fkey ADD CONSTRAINT fkey_to_ref_tbl FOREIGN KEY (y) REFERENCES ref_table_with_fkey(id);
-WITH shardid AS (SELECT shardid FROM pg_dist_shard where logicalrelid = 'partitioned_tbl_with_fkey'::regclass ORDER BY shardid LIMIT 1)
-SELECT citus_move_shard_placement(shardid.shardid, 'localhost', 57637, 'localhost', 57638, shard_transfer_mode := 'force_logical') FROM shardid;
- citus_move_shard_placement
----------------------------------------------------------------------
-
-(1 row)
-
-SET client_min_messages TO WARNING;
 DROP SCHEMA fkey_to_reference_shard_rebalance CASCADE;
+NOTICE:  drop cascades to 8 other objects
+DETAIL:  drop cascades to type fkey_to_reference_shard_rebalance.foreign_details
+drop cascades to view fkey_to_reference_shard_rebalance.table_fkeys_in_workers
+drop cascades to table fkey_to_reference_shard_rebalance.referenced_table
+drop cascades to table fkey_to_reference_shard_rebalance.referencing_table
+drop cascades to table fkey_to_reference_shard_rebalance.referencing_table2
+drop cascades to function fkey_to_reference_shard_rebalance.get_foreign_key_to_reference_table_commands(oid)
+drop cascades to table fkey_to_reference_shard_rebalance.reference_table_commands
+drop cascades to table fkey_to_reference_shard_rebalance.referenceing_dist_table

src/test/regress/expected/multi_foreign_key.out

@@ -431,10 +431,10 @@ SELECT create_distributed_table('referencing_table', 'ref_id', 'hash');
 
 (1 row)
 
--- verify that we skip foreign key validation when citus.skip_constraint_validation is set to ON
+-- verify that we skip foreign key validation when propagation is turned off
 -- not skipping validation would result in a distributed query, which emits debug messages
 BEGIN;
-SET LOCAL citus.skip_constraint_validation TO on;
+SET LOCAL citus.enable_ddl_propagation TO off;
 SET LOCAL client_min_messages TO DEBUG1;
 ALTER TABLE referencing_table ADD CONSTRAINT test_constraint FOREIGN KEY (ref_id) REFERENCES referenced_table (id);
 ABORT;

src/test/regress/sql/foreign_key_to_reference_shard_rebalance.sql

@@ -72,24 +72,4 @@ SELECT conname, contype, convalidated FROM pg_constraint WHERE conrelid = 'fkey_
 
 \c - - - :master_port
 
-SET search_path TO fkey_to_reference_shard_rebalance;
-SET citus.shard_replication_factor to 1;
-
--- test moving a shard with foreign key
-create table ref_table_with_fkey (id int primary key);
-select create_reference_table('ref_table_with_fkey');
-insert into ref_table_with_fkey select s from generate_series(0,9) s;
-
-create table partitioned_tbl_with_fkey (x int, y int, t timestamptz default now()) partition by range (t);
-select create_distributed_table('partitioned_tbl_with_fkey','x');
-create table partition_1_with_fkey partition of partitioned_tbl_with_fkey for values from ('2022-01-01') to ('2022-12-31');
-create table partition_2_with_fkey partition of partitioned_tbl_with_fkey for values from ('2023-01-01') to ('2023-12-31');
-insert into partitioned_tbl_with_fkey (x,y) select s,s%10 from generate_series(1,100) s;
-
-ALTER TABLE partitioned_tbl_with_fkey ADD CONSTRAINT fkey_to_ref_tbl FOREIGN KEY (y) REFERENCES ref_table_with_fkey(id);
-
-WITH shardid AS (SELECT shardid FROM pg_dist_shard where logicalrelid = 'partitioned_tbl_with_fkey'::regclass ORDER BY shardid LIMIT 1)
-SELECT citus_move_shard_placement(shardid.shardid, 'localhost', 57637, 'localhost', 57638, shard_transfer_mode := 'force_logical') FROM shardid;
-
-SET client_min_messages TO WARNING;
 DROP SCHEMA fkey_to_reference_shard_rebalance CASCADE;

src/test/regress/sql/multi_foreign_key.sql

@@ -246,10 +246,10 @@ SELECT create_distributed_table('referenced_table', 'id', 'hash');
 CREATE TABLE referencing_table(id int, ref_id int);
 SELECT create_distributed_table('referencing_table', 'ref_id', 'hash');
 
--- verify that we skip foreign key validation when citus.skip_constraint_validation is set to ON
+-- verify that we skip foreign key validation when propagation is turned off
 -- not skipping validation would result in a distributed query, which emits debug messages
 BEGIN;
-SET LOCAL citus.skip_constraint_validation TO on;
+SET LOCAL citus.enable_ddl_propagation TO off;
 SET LOCAL client_min_messages TO DEBUG1;
 ALTER TABLE referencing_table ADD CONSTRAINT test_constraint FOREIGN KEY (ref_id) REFERENCES referenced_table (id);
 ABORT;