From a5f7f001b0c84e1947678506c87f93bc3f3bc2f6 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 2 Nov 2022 16:27:31 +0300 Subject: [PATCH] Make sure to disallow triggers that depend on extensions (#6399) DESCRIPTION: Makes sure to disallow triggers that depend on extensions We were already doing so for `ALTER trigger DEPENDS ON EXTENSION` commands. However, we also need to disallow creating Citus tables having such triggers already, so this PR fixes that. --- .../citus_add_local_table_to_metadata.c | 1 + .../commands/create_distributed_table.c | 7 +- src/backend/distributed/commands/trigger.c | 68 ++++++++++++++++--- src/backend/distributed/metadata/dependency.c | 39 ++++++++++- src/include/distributed/commands.h | 1 + src/include/distributed/metadata/distobject.h | 1 + .../expected/citus_local_table_triggers.out | 2 +- .../expected/citus_local_tables_mx.out | 2 +- .../regress/expected/citus_table_triggers.out | 22 +++--- .../regress/expected/distributed_triggers.out | 21 +++++- .../expected/multi_alter_table_statements.out | 4 +- src/test/regress/sql/distributed_triggers.sql | 21 ++++++ 12 files changed, 160 insertions(+), 29 deletions(-) diff --git a/src/backend/distributed/commands/citus_add_local_table_to_metadata.c b/src/backend/distributed/commands/citus_add_local_table_to_metadata.c index 6d8fd511b..ddae5f8ee 100644 --- a/src/backend/distributed/commands/citus_add_local_table_to_metadata.c +++ b/src/backend/distributed/commands/citus_add_local_table_to_metadata.c @@ -487,6 +487,7 @@ ErrorIfUnsupportedCreateCitusLocalTable(Relation relation) ErrorIfUnsupportedCitusLocalTableKind(relationId); EnsureTableNotDistributed(relationId); ErrorIfUnsupportedCitusLocalColumnDefinition(relation); + ErrorIfRelationHasUnsupportedTrigger(relationId); /* * When creating other citus table types, we don't need to check that case as diff --git a/src/backend/distributed/commands/create_distributed_table.c b/src/backend/distributed/commands/create_distributed_table.c index 236e505ef..ba2344504 100644 --- a/src/backend/distributed/commands/create_distributed_table.c +++ b/src/backend/distributed/commands/create_distributed_table.c @@ -1667,12 +1667,15 @@ EnsureRelationCanBeDistributed(Oid relationId, Var *distributionColumn, EnsureLocalTableEmptyIfNecessary(relationId, distributionMethod, viaDeprecatedAPI); /* user really wants triggers? */ - if (!EnableUnsafeTriggers) + if (EnableUnsafeTriggers) + { + ErrorIfRelationHasUnsupportedTrigger(relationId); + } + else { EnsureRelationHasNoTriggers(relationId); } - /* we assume callers took necessary locks */ Relation relation = relation_open(relationId, NoLock); TupleDesc relationDesc = RelationGetDescr(relation); diff --git a/src/backend/distributed/commands/trigger.c b/src/backend/distributed/commands/trigger.c index 5d3b92f04..0ddad70f5 100644 --- a/src/backend/distributed/commands/trigger.c +++ b/src/backend/distributed/commands/trigger.c @@ -53,6 +53,7 @@ static void ExtractDropStmtTriggerAndRelationName(DropStmt *dropTriggerStmt, char **triggerName, char **relationName); static void ErrorIfDropStmtDropsMultipleTriggers(DropStmt *dropTriggerStmt); +static char * GetTriggerNameById(Oid triggerId); static int16 GetTriggerTypeById(Oid triggerId); #if (PG_VERSION_NUM < PG_VERSION_15) static void ErrorOutIfCloneTrigger(Oid tgrelid, const char *tgname); @@ -547,13 +548,17 @@ PreprocessAlterTriggerDependsStmt(Node *node, const char *queryString, String *triggerNameValue = GetAlterTriggerDependsTriggerNameValue(alterTriggerDependsStmt); - ereport(ERROR, (errmsg( - "Triggers \"%s\" on distributed tables and local tables added to metadata " - "are not allowed to depend on an extension", strVal( - triggerNameValue)), - errdetail( - "Triggers from extensions are expected to be created on the workers " - "by the extension they depend on."))); + + ereport(ERROR, (errmsg("trigger \"%s\" depends on an extension and this " + "is not supported for distributed tables and " + "local tables added to metadata", + strVal(triggerNameValue)), + errdetail("Triggers from extensions are expected to be " + "created on the workers by the extension they " + "depend on."))); + + /* not reachable, keep compiler happy */ + return NIL; } @@ -718,15 +723,40 @@ ErrorOutForTriggerIfNotSupported(Oid relationId) } else if (IsCitusTableType(relationId, DISTRIBUTED_TABLE)) { - ereport(ERROR, (errmsg("triggers are not supported on distributed tables " - "when \"citus.enable_unsafe_triggers\" is set to " - "\"false\""))); + ereport(ERROR, (errmsg("triggers are not supported on distributed tables"))); } /* we always support triggers on citus local tables */ } +/* + * ErrorIfRelationHasUnsupportedTrigger throws an error if given relation has + * a trigger that is not supported by Citus. + */ +void +ErrorIfRelationHasUnsupportedTrigger(Oid relationId) +{ + List *relationTriggerList = GetExplicitTriggerIdList(relationId); + + Oid triggerId = InvalidOid; + foreach_oid(triggerId, relationTriggerList) + { + ObjectAddress triggerObjectAddress = InvalidObjectAddress; + ObjectAddressSet(triggerObjectAddress, TriggerRelationId, triggerId); + + /* triggers that depend on extensions are not supported */ + if (ObjectAddressDependsOnExtension(&triggerObjectAddress)) + { + ereport(ERROR, (errmsg("trigger \"%s\" depends on an extension and this " + "is not supported for distributed tables and " + "local tables added to metadata", + GetTriggerNameById(triggerId)))); + } + } +} + + #if (PG_VERSION_NUM < PG_VERSION_15) /* @@ -935,6 +965,24 @@ CitusCreateTriggerCommandDDLJob(Oid relationId, char *triggerName, } +/* + * GetTriggerNameById returns name of given trigger if such a trigger exists. + * Otherwise, errors out. + */ +static char * +GetTriggerNameById(Oid triggerId) +{ + bool missingOk = false; + HeapTuple triggerTuple = GetTriggerTupleById(triggerId, missingOk); + + Form_pg_trigger triggerForm = (Form_pg_trigger) GETSTRUCT(triggerTuple); + char *triggerName = pstrdup(NameStr(triggerForm->tgname)); + heap_freetuple(triggerTuple); + + return triggerName; +} + + /* * GetTriggerTypeById returns trigger type (tgtype) of the trigger identified * by triggerId if it exists. Otherwise, errors out. diff --git a/src/backend/distributed/metadata/dependency.c b/src/backend/distributed/metadata/dependency.c index 6387d6ac4..a67c8fed0 100644 --- a/src/backend/distributed/metadata/dependency.c +++ b/src/backend/distributed/metadata/dependency.c @@ -148,6 +148,9 @@ static void CollectObjectAddress(ObjectAddressCollector *collector, static bool IsObjectAddressCollected(ObjectAddress findAddress, ObjectAddressCollector *collector); static ObjectAddress * GetUndistributableDependency(const ObjectAddress *objectAddress); +static bool ObjectAddressHasExtensionDependency(const ObjectAddress *target, + ObjectAddress *extensionAddress, + int extensionDependency); static void MarkObjectVisited(ObjectAddressCollector *collector, ObjectAddress target); static bool TargetObjectVisited(ObjectAddressCollector *collector, @@ -1080,6 +1083,19 @@ IsTableOwnedByExtension(Oid relationId) } +/* + * ObjectAddressDependsOnExtension returns whether or not the object depends + * on an extension. It is assumed that "an object having a dependency of type + * DEPENDENCY_AUTO_EXTENSION to an extension" depends on that extension. + */ +bool +ObjectAddressDependsOnExtension(const ObjectAddress *target) +{ + return ObjectAddressHasExtensionDependency(target, NULL, + DEPENDENCY_AUTO_EXTENSION); +} + + /* * IsObjectAddressOwnedByExtension returns whether or not the object is owned by an * extension. It is assumed that an object having a dependency on an extension is created @@ -1092,6 +1108,27 @@ static bool IsObjectAddressOwnedByExtension(const ObjectAddress *target, ObjectAddress *extensionAddress) { + return ObjectAddressHasExtensionDependency(target, extensionAddress, + DEPENDENCY_EXTENSION); +} + + +/* + * ObjectAddressHasExtensionDependency is a helper function that returns true if + * given object has a dependency record (of type DEPENDENCY_EXTENSION or + * DEPENDENCY_AUTO_EXTENSION) for an extension. + * + * If extensionAddress is not set to a NULL pointer the function will write the + * extension address this function depends on into this location. + */ +static bool +ObjectAddressHasExtensionDependency(const ObjectAddress *target, + ObjectAddress *extensionAddress, + int extensionDependency) +{ + Assert(extensionDependency == DEPENDENCY_EXTENSION || + extensionDependency == DEPENDENCY_AUTO_EXTENSION); + ScanKeyData key[2]; HeapTuple depTup = NULL; bool result = false; @@ -1109,7 +1146,7 @@ IsObjectAddressOwnedByExtension(const ObjectAddress *target, while (HeapTupleIsValid(depTup = systable_getnext(depScan))) { Form_pg_depend pg_depend = (Form_pg_depend) GETSTRUCT(depTup); - if (pg_depend->deptype == DEPENDENCY_EXTENSION) + if (pg_depend->deptype == extensionDependency) { result = true; if (extensionAddress != NULL) diff --git a/src/include/distributed/commands.h b/src/include/distributed/commands.h index 656feec67..d947955f0 100644 --- a/src/include/distributed/commands.h +++ b/src/include/distributed/commands.h @@ -691,6 +691,7 @@ extern void AlterTriggerDependsEventExtendNames( AlterObjectDependsStmt *alterTriggerDependsStmt, char *schemaName, uint64 shardId); extern void ErrorOutForTriggerIfNotSupported(Oid relationId); +extern void ErrorIfRelationHasUnsupportedTrigger(Oid relationId); extern List * PreprocessDropTriggerStmt(Node *node, const char *queryString, ProcessUtilityContext processUtilityContext); extern void DropTriggerEventExtendNames(DropStmt *dropTriggerStmt, char *schemaName, diff --git a/src/include/distributed/metadata/distobject.h b/src/include/distributed/metadata/distobject.h index 900c15590..91f4fb630 100644 --- a/src/include/distributed/metadata/distobject.h +++ b/src/include/distributed/metadata/distobject.h @@ -27,6 +27,7 @@ extern void MarkObjectDistributedViaSuperUser(const ObjectAddress *distAddress); extern void MarkObjectDistributedLocally(const ObjectAddress *distAddress); extern void UnmarkObjectDistributed(const ObjectAddress *address); extern bool IsTableOwnedByExtension(Oid relationId); +extern bool ObjectAddressDependsOnExtension(const ObjectAddress *target); extern bool IsAnyObjectAddressOwnedByExtension(const List *targets, ObjectAddress *extensionAddress); extern bool IsObjectAddressOwnedByCitus(const ObjectAddress *objectAddress); diff --git a/src/test/regress/expected/citus_local_table_triggers.out b/src/test/regress/expected/citus_local_table_triggers.out index 983b916e1..4dc6a3b57 100644 --- a/src/test/regress/expected/citus_local_table_triggers.out +++ b/src/test/regress/expected/citus_local_table_triggers.out @@ -196,7 +196,7 @@ FOR EACH STATEMENT EXECUTE FUNCTION dummy_function();') CREATE EXTENSION seg; -- ALTER TRIGGER DEPENDS ON ALTER TRIGGER "trigger\'name" ON "interesting!schema"."citus_local!_table" DEPENDS ON EXTENSION seg; -ERROR: Triggers "trigger\'name" on distributed tables and local tables added to metadata are not allowed to depend on an extension +ERROR: trigger "trigger\'name" depends on an extension and this is not supported for distributed tables and local tables added to metadata BEGIN; -- show that triggers on both shell relation and shard relation are not depending on seg SELECT tgname FROM pg_depend, pg_trigger, pg_extension diff --git a/src/test/regress/expected/citus_local_tables_mx.out b/src/test/regress/expected/citus_local_tables_mx.out index 3177ed9e9..4593b9dda 100644 --- a/src/test/regress/expected/citus_local_tables_mx.out +++ b/src/test/regress/expected/citus_local_tables_mx.out @@ -54,7 +54,7 @@ SELECT 1 FROM citus_activate_node('localhost', :worker_1_port); -- show that the trigger is not allowed to depend(explicitly) on an extension. CREATE EXTENSION seg; ALTER TRIGGER dummy_function_trigger ON citus_local_table DEPENDS ON EXTENSION seg; -ERROR: Triggers "dummy_function_trigger" on distributed tables and local tables added to metadata are not allowed to depend on an extension +ERROR: trigger "dummy_function_trigger" depends on an extension and this is not supported for distributed tables and local tables added to metadata ALTER TRIGGER dummy_function_trigger ON citus_local_table RENAME TO renamed_trigger; NOTICE: executing the command locally: SELECT worker_apply_shard_ddl_command (1508000, 'citus_local_tables_mx', 'ALTER TRIGGER dummy_function_trigger ON citus_local_table RENAME TO renamed_trigger;') ALTER TABLE citus_local_table DISABLE TRIGGER ALL; diff --git a/src/test/regress/expected/citus_table_triggers.out b/src/test/regress/expected/citus_table_triggers.out index ef1229163..9fce32070 100644 --- a/src/test/regress/expected/citus_table_triggers.out +++ b/src/test/regress/expected/citus_table_triggers.out @@ -34,7 +34,7 @@ SELECT create_reference_table('reference_table'); CREATE TRIGGER update_value_dist AFTER INSERT ON distributed_table FOR EACH ROW EXECUTE FUNCTION update_value(); -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables CREATE TRIGGER update_value_ref AFTER INSERT ON reference_table FOR EACH ROW EXECUTE FUNCTION update_value(); @@ -56,28 +56,28 @@ SET citus.enable_ddl_propagation to ON; CREATE EXTENSION seg; -- below all should error out ALTER TRIGGER update_value_dist ON distributed_table RENAME TO update_value_dist1; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables ALTER TRIGGER update_value_dist ON distributed_table DEPENDS ON EXTENSION seg; -ERROR: Triggers "update_value_dist" on distributed tables and local tables added to metadata are not allowed to depend on an extension +ERROR: trigger "update_value_dist" depends on an extension and this is not supported for distributed tables and local tables added to metadata DROP TRIGGER update_value_dist ON distributed_table; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables ALTER TABLE distributed_table DISABLE TRIGGER ALL; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables ALTER TABLE distributed_table DISABLE TRIGGER USER; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables ALTER TABLE distributed_table DISABLE TRIGGER update_value_dist; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables ALTER TABLE distributed_table ENABLE TRIGGER ALL; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables ALTER TABLE distributed_table ENABLE TRIGGER USER; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables ALTER TABLE distributed_table ENABLE TRIGGER update_value_dist; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables -- below all should error out ALTER TRIGGER update_value_ref ON reference_table RENAME TO update_value_ref1; ERROR: triggers are not supported on reference tables ALTER TRIGGER update_value_ref ON reference_table DEPENDS ON EXTENSION seg; -ERROR: Triggers "update_value_ref" on distributed tables and local tables added to metadata are not allowed to depend on an extension +ERROR: trigger "update_value_ref" depends on an extension and this is not supported for distributed tables and local tables added to metadata DROP TRIGGER update_value_ref ON reference_table; ERROR: triggers are not supported on reference tables ALTER TABLE reference_table DISABLE TRIGGER ALL; diff --git a/src/test/regress/expected/distributed_triggers.out b/src/test/regress/expected/distributed_triggers.out index 23b3e8e0c..842323042 100644 --- a/src/test/regress/expected/distributed_triggers.out +++ b/src/test/regress/expected/distributed_triggers.out @@ -766,7 +766,7 @@ SELECT * FROM sale_triggers ORDER BY 1,2; CREATE EXTENSION seg; ALTER TRIGGER "emptest_audit" ON "emptest" DEPENDS ON EXTENSION seg; -ERROR: Triggers "emptest_audit" on distributed tables and local tables added to metadata are not allowed to depend on an extension +ERROR: trigger "emptest_audit" depends on an extension and this is not supported for distributed tables and local tables added to metadata DETAIL: Triggers from extensions are expected to be created on the workers by the extension they depend on. DROP TABLE data_ref_table; -- @@ -1000,6 +1000,25 @@ SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'D') FROM pg_trigger (localhost,57638,t,t) (2 rows) +CREATE TABLE dist_trigger_depends_on_test(a int); +CREATE FUNCTION dist_trigger_depends_on_test_func() + RETURNS trigger + LANGUAGE plpgsql +AS $function$ +BEGIN + RETURN NULL; +END; +$function$; +CREATE TRIGGER dist_trigger_depends_on_test_trig +AFTER UPDATE OR DELETE ON dist_trigger_depends_on_test +FOR STATEMENT EXECUTE FUNCTION dist_trigger_depends_on_test_func(); +ALTER trigger dist_trigger_depends_on_test_trig ON dist_trigger_depends_on_test DEPENDS ON EXTENSION seg; +SELECT create_distributed_table('dist_trigger_depends_on_test', 'a'); +ERROR: trigger "dist_trigger_depends_on_test_trig" depends on an extension and this is not supported for distributed tables and local tables added to metadata +SELECT create_reference_table('dist_trigger_depends_on_test'); +ERROR: trigger "dist_trigger_depends_on_test_trig" depends on an extension and this is not supported for distributed tables and local tables added to metadata +SELECT citus_add_local_table_to_metadata('dist_trigger_depends_on_test'); +ERROR: trigger "dist_trigger_depends_on_test_trig" depends on an extension and this is not supported for distributed tables and local tables added to metadata SET client_min_messages TO ERROR; RESET citus.enable_unsafe_triggers; SELECT run_command_on_workers('ALTER SYSTEM RESET citus.enable_unsafe_triggers;'); diff --git a/src/test/regress/expected/multi_alter_table_statements.out b/src/test/regress/expected/multi_alter_table_statements.out index 375ae8e97..52fe8d762 100644 --- a/src/test/regress/expected/multi_alter_table_statements.out +++ b/src/test/regress/expected/multi_alter_table_statements.out @@ -986,7 +986,7 @@ SELECT value, count(*) FROM trigger_table GROUP BY value ORDER BY value; (1 row) ALTER TABLE trigger_table DISABLE TRIGGER ALL; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables INSERT INTO trigger_table VALUES (1, 'trigger disabled'); SELECT value, count(*) FROM trigger_table GROUP BY value ORDER BY value; value | count @@ -995,7 +995,7 @@ SELECT value, count(*) FROM trigger_table GROUP BY value ORDER BY value; (1 row) ALTER TABLE trigger_table ENABLE TRIGGER ALL; -ERROR: triggers are not supported on distributed tables when "citus.enable_unsafe_triggers" is set to "false" +ERROR: triggers are not supported on distributed tables INSERT INTO trigger_table VALUES (1, 'trigger disabled'); SELECT value, count(*) FROM trigger_table GROUP BY value ORDER BY value; value | count diff --git a/src/test/regress/sql/distributed_triggers.sql b/src/test/regress/sql/distributed_triggers.sql index fb0e20e6d..da0d908c6 100644 --- a/src/test/regress/sql/distributed_triggers.sql +++ b/src/test/regress/sql/distributed_triggers.sql @@ -557,6 +557,27 @@ SELECT citus_add_local_table_to_metadata('citus_local'); SELECT bool_and(tgenabled = 'D') FROM pg_trigger WHERE tgname LIKE 'citus_local_trig%'; SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'D') FROM pg_trigger WHERE tgname LIKE 'citus_local_trig%'$$); +CREATE TABLE dist_trigger_depends_on_test(a int); + +CREATE FUNCTION dist_trigger_depends_on_test_func() + RETURNS trigger + LANGUAGE plpgsql +AS $function$ +BEGIN + RETURN NULL; +END; +$function$; + +CREATE TRIGGER dist_trigger_depends_on_test_trig +AFTER UPDATE OR DELETE ON dist_trigger_depends_on_test +FOR STATEMENT EXECUTE FUNCTION dist_trigger_depends_on_test_func(); + +ALTER trigger dist_trigger_depends_on_test_trig ON dist_trigger_depends_on_test DEPENDS ON EXTENSION seg; + +SELECT create_distributed_table('dist_trigger_depends_on_test', 'a'); +SELECT create_reference_table('dist_trigger_depends_on_test'); +SELECT citus_add_local_table_to_metadata('dist_trigger_depends_on_test'); + SET client_min_messages TO ERROR; RESET citus.enable_unsafe_triggers; SELECT run_command_on_workers('ALTER SYSTEM RESET citus.enable_unsafe_triggers;');