diff --git a/src/backend/distributed/commands/foreign_constraint.c b/src/backend/distributed/commands/foreign_constraint.c index 20057505d..cf1e43fd4 100644 --- a/src/backend/distributed/commands/foreign_constraint.c +++ b/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. diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 2d4906dc0..ad2593bc1 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/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; } diff --git a/src/backend/distributed/executor/multi_executor.c b/src/backend/distributed/executor/multi_executor.c index a41e5b374..a0063adc8 100644 --- a/src/backend/distributed/executor/multi_executor.c +++ b/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() diff --git a/src/include/distributed/commands.h b/src/include/distributed/commands.h index 075e574c3..c3ec4fafb 100644 --- a/src/include/distributed/commands.h +++ b/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); diff --git a/src/test/regress/enterprise_isolation_logicalrep_2_schedule b/src/test/regress/enterprise_isolation_logicalrep_2_schedule index a350ea2f1..e8915cb27 100644 --- a/src/test/regress/enterprise_isolation_logicalrep_2_schedule +++ b/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 diff --git a/src/test/regress/expected/isolation_logical_replication_skip_fk_validation.out b/src/test/regress/expected/isolation_logical_replication_skip_fk_validation.out new file mode 100644 index 000000000..0f653e0c6 --- /dev/null +++ b/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); + +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) + diff --git a/src/test/regress/spec/isolation_logical_replication_skip_fk_validation.spec b/src/test/regress/spec/isolation_logical_replication_skip_fk_validation.spec new file mode 100644 index 000000000..253b1303c --- /dev/null +++ b/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"