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.
2022-11-02 16:27:31 +03:00 · 2022-11-02 16:27:31 +03:00 · a5f7f001b0
parent deeacfee04
commit a5f7f001b0
12 changed files with 160 additions and 29 deletions
--- 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
--- 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);
--- 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.
--- 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)
--- 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,
--- 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);
--- 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
--- 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;
--- 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;
--- 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;');
--- 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
--- 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;');