From 64661cb378d37cac4a813fd85321aef0bf2da364 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Fri, 16 Dec 2022 17:06:46 +0100 Subject: [PATCH] Revert "Introduce GUC citus.skip_constraint_validation (#6281)" This reverts commit eadc88a800cf9c45a11c22a954e27c186156e461. --- ..._table_operation_for_connected_relations.c | 8 ++-- .../distributed/commands/foreign_constraint.c | 15 +------ src/backend/distributed/commands/table.c | 30 ++++++++------ .../distributed/commands/utility_hook.c | 10 ++--- .../distributed/executor/multi_executor.c | 10 ----- src/backend/distributed/shared_library_init.c | 15 ------- src/include/distributed/commands.h | 4 +- src/include/distributed/multi_executor.h | 1 - ...reign_key_to_reference_shard_rebalance.out | 39 +++++-------------- .../regress/expected/multi_foreign_key.out | 4 +- ...reign_key_to_reference_shard_rebalance.sql | 20 ---------- src/test/regress/sql/multi_foreign_key.sql | 4 +- 12 files changed, 43 insertions(+), 117 deletions(-) diff --git a/src/backend/distributed/commands/cascade_table_operation_for_connected_relations.c b/src/backend/distributed/commands/cascade_table_operation_for_connected_relations.c index 126e64325..091345356 100644 --- a/src/backend/distributed/commands/cascade_table_operation_for_connected_relations.c +++ b/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; } diff --git a/src/backend/distributed/commands/foreign_constraint.c b/src/backend/distributed/commands/foreign_constraint.c index 20057505d..22b28628a 100644 --- a/src/backend/distributed/commands/foreign_constraint.c +++ b/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. diff --git a/src/backend/distributed/commands/table.c b/src/backend/distributed/commands/table.c index 8101dc964..12213f4a0 100644 --- a/src/backend/distributed/commands/table.c +++ b/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 -SkipForeignKeyValidationIfConstraintIsFkey(AlterTableStmt *alterTableStatement) +Node * +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 */ - EnableSkippingConstraintValidation(); - return; + /* foreign constraint validations will be done in shards. */ + constraint->skip_validation = true; } } } + + return (Node *) alterTableStatement; } diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 4aba31468..204f8a36e 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)) @@ -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; } diff --git a/src/backend/distributed/executor/multi_executor.c b/src/backend/distributed/executor/multi_executor.c index a41e5b374..6acf6169b 100644 --- a/src/backend/distributed/executor/multi_executor.c +++ b/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() diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index c15268056..f7d79559b 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/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 " diff --git a/src/include/distributed/commands.h b/src/include/distributed/commands.h index fa2691fee..be4bb795f 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); @@ -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); diff --git a/src/include/distributed/multi_executor.h b/src/include/distributed/multi_executor.h index b9f272d0a..c8254bf44 100644 --- a/src/include/distributed/multi_executor.h +++ b/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; diff --git a/src/test/regress/expected/foreign_key_to_reference_shard_rebalance.out b/src/test/regress/expected/foreign_key_to_reference_shard_rebalance.out index 54054d99b..f5b0bb9d5 100644 --- a/src/test/regress/expected/foreign_key_to_reference_shard_rebalance.out +++ b/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 diff --git a/src/test/regress/expected/multi_foreign_key.out b/src/test/regress/expected/multi_foreign_key.out index 3702e3782..806b09e7d 100644 --- a/src/test/regress/expected/multi_foreign_key.out +++ b/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; diff --git a/src/test/regress/sql/foreign_key_to_reference_shard_rebalance.sql b/src/test/regress/sql/foreign_key_to_reference_shard_rebalance.sql index dc70f8563..fbf28e8b2 100644 --- a/src/test/regress/sql/foreign_key_to_reference_shard_rebalance.sql +++ b/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; diff --git a/src/test/regress/sql/multi_foreign_key.sql b/src/test/regress/sql/multi_foreign_key.sql index 041202dff..f39f0ace4 100644 --- a/src/test/regress/sql/multi_foreign_key.sql +++ b/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;