From d41f88b6f8a8440a4c2a3d5018dfd73ac127a677 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Fri, 27 Jan 2023 13:08:05 +0100 Subject: [PATCH] Actually skip constraint validation on shards after shard move (#6640) DESCRIPTION: Fix foreign key validation skip at the end of shard move 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 #6550 showed us that this is the better place to skip it, because it will also skip the planning phase and not just the execution. (cherry picked from commit 81dcddd1ef2a268d95f1aa71be81e31192207f7b) --- .../distributed/commands/foreign_constraint.c | 13 ---- .../distributed/commands/utility_hook.c | 29 +++++++- .../distributed/executor/multi_executor.c | 5 -- src/include/distributed/commands.h | 1 - ...enterprise_isolation_logicalrep_2_schedule | 1 + ...logical_replication_skip_fk_validation.out | 52 ++++++++++++++ ...ogical_replication_skip_fk_validation.spec | 72 +++++++++++++++++++ 7 files changed, 151 insertions(+), 22 deletions(-) create mode 100644 src/test/regress/expected/isolation_logical_replication_skip_fk_validation.out create mode 100644 src/test/regress/spec/isolation_logical_replication_skip_fk_validation.spec diff --git a/src/backend/distributed/commands/foreign_constraint.c b/src/backend/distributed/commands/foreign_constraint.c index a9cca4c5d..cd10449ba 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 dbf942196..1b8d8dd4c 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -375,7 +375,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)) @@ -620,6 +619,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)) { @@ -889,8 +914,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 802e3734f..3df08eb74 100644 --- a/src/include/distributed/commands.h +++ b/src/include/distributed/commands.h @@ -285,7 +285,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"