From dfcc18468cf4b24bedab7aaaf602f12ac374538f Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Sun, 31 May 2020 15:28:24 +0300 Subject: [PATCH] Error out for unsupported trigger objects Error out if creating a citus table from a table having triggers. Error out for CREATE TRIGGER commands that are run on citus tables. --- .../commands/create_distributed_table.c | 29 ++++- src/backend/distributed/commands/trigger.c | 45 ++++++++ .../distributed/commands/utility_hook.c | 7 ++ src/include/distributed/commands.h | 3 + .../expected/create_table_triggers.out | 101 ++++++++++++++++++ .../expected/create_table_triggers_0.out | 101 ++++++++++++++++++ src/test/regress/multi_schedule | 2 +- .../regress/sql/create_table_triggers.sql | 100 +++++++++++++++++ 8 files changed, 386 insertions(+), 2 deletions(-) create mode 100644 src/test/regress/expected/create_table_triggers.out create mode 100644 src/test/regress/expected/create_table_triggers_0.out create mode 100644 src/test/regress/sql/create_table_triggers.sql diff --git a/src/backend/distributed/commands/create_distributed_table.c b/src/backend/distributed/commands/create_distributed_table.c index 44bcb0d45..7f10a40fb 100644 --- a/src/backend/distributed/commands/create_distributed_table.c +++ b/src/backend/distributed/commands/create_distributed_table.c @@ -99,6 +99,7 @@ static void EnsureTableCanBeColocatedWith(Oid relationId, char replicationModel, Oid sourceRelationId); static void EnsureLocalTableEmpty(Oid relationId); static void EnsureTableNotDistributed(Oid relationId); +static void EnsureRelationHasNoTriggers(Oid relationId); static Oid SupportFunctionForColumn(Var *partitionColumn, Oid accessMethodId, int16 supportFunctionNumber); static void EnsureLocalTableEmptyIfNecessary(Oid relationId, char distributionMethod, @@ -650,6 +651,7 @@ EnsureRelationCanBeDistributed(Oid relationId, Var *distributionColumn, EnsureTableNotDistributed(relationId); EnsureLocalTableEmptyIfNecessary(relationId, distributionMethod, viaDeprecatedAPI); EnsureReplicationSettings(InvalidOid, replicationModel); + EnsureRelationHasNoTriggers(relationId); /* we assume callers took necessary locks */ Relation relation = relation_open(relationId, NoLock); @@ -972,6 +974,31 @@ EnsureReplicationSettings(Oid relationId, char replicationModel) } +/* + * EnsureRelationHasNoTriggers errors out if the given table has triggers on + * it. See also GetExplicitTriggerIdList function's comment for the triggers this + * function errors out. + */ +static void +EnsureRelationHasNoTriggers(Oid relationId) +{ + List *explicitTriggerIds = GetExplicitTriggerIdList(relationId); + + if (list_length(explicitTriggerIds) > 0) + { + char *relationName = get_rel_name(relationId); + + Assert(relationName != NULL); + ereport(ERROR, (errmsg("cannot distribute relation \"%s\" because it has " + "triggers ", relationName), + errdetail("Citus does not support distributing tables with " + "triggers."), + errhint("Drop all the triggers on \"%s\" and retry.", + relationName))); + } +} + + /* * LookupDistributionMethod maps the oids of citus.distribution_type enum * values to pg_dist_partition.partmethod values. @@ -1176,7 +1203,7 @@ CreateTruncateTrigger(Oid relationId) CreateTrigStmt *trigger = makeNode(CreateTrigStmt); trigger->trigname = triggerName->data; trigger->relation = NULL; - trigger->funcname = SystemFuncName("citus_truncate_trigger"); + trigger->funcname = SystemFuncName(CITUS_TRUNCATE_TRIGGER_NAME); trigger->args = NIL; trigger->row = false; trigger->timing = TRIGGER_TYPE_AFTER; diff --git a/src/backend/distributed/commands/trigger.c b/src/backend/distributed/commands/trigger.c index 1a1aaca18..a2bb1cf3c 100644 --- a/src/backend/distributed/commands/trigger.c +++ b/src/backend/distributed/commands/trigger.c @@ -139,3 +139,48 @@ get_relation_trigger_oid_compat(HeapTuple heapTuple) return triggerOid; } + + +/* + * ErrorIfUnsupportedCreateTriggerCommand errors out for the CREATE TRIGGER + * command that is run for a citus table if it is not citus_truncate_trigger. + * + * Note that internal triggers that are created implicitly by postgres for + * foreign key validation already wouldn't be executed via process utility, + * hence there is no need to check that case here. + */ +void +ErrorIfUnsupportedCreateTriggerCommand(CreateTrigStmt *createTriggerStmt) +{ + RangeVar *triggerRelation = createTriggerStmt->relation; + + bool missingOk = true; + Oid relationId = RangeVarGetRelid(triggerRelation, AccessShareLock, missingOk); + + if (!OidIsValid(relationId)) + { + /* + * standard_ProcessUtility would already error out if the given table + * does not exist + */ + return; + } + + if (!IsCitusTable(relationId)) + { + return; + } + + char *functionName = makeRangeVarFromNameList(createTriggerStmt->funcname)->relname; + if (strncmp(functionName, CITUS_TRUNCATE_TRIGGER_NAME, NAMEDATALEN) == 0) + { + return; + } + + char *relationName = triggerRelation->relname; + + Assert(relationName != NULL); + ereport(ERROR, (errmsg("cannot create trigger on relation \"%s\" because it " + "is either a distributed table or a reference table", + relationName))); +} diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index fbed95a5f..b8ad9bdb6 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -208,6 +208,13 @@ multi_ProcessUtility(PlannedStmt *pstmt, parsetree = ProcessCreateSubscriptionStmt(createSubStmt); } + if (IsA(parsetree, CreateTrigStmt)) + { + CreateTrigStmt *createTriggerStmt = (CreateTrigStmt *) parsetree; + + ErrorIfUnsupportedCreateTriggerCommand(createTriggerStmt); + } + if (IsA(parsetree, CallStmt)) { CallStmt *callStmt = (CallStmt *) parsetree; diff --git a/src/include/distributed/commands.h b/src/include/distributed/commands.h index 3efb64db5..95ac1c094 100644 --- a/src/include/distributed/commands.h +++ b/src/include/distributed/commands.h @@ -44,6 +44,8 @@ typedef struct DistributeObjectOps ObjectAddress (*address)(Node *, bool); } DistributeObjectOps; +#define CITUS_TRUNCATE_TRIGGER_NAME "citus_truncate_trigger" + const DistributeObjectOps * GetDistributeObjectOps(Node *node); /* cluster.c - forward declarations */ @@ -278,6 +280,7 @@ extern void PostprocessVacuumStmt(VacuumStmt *vacuumStmt, const char *vacuumComm extern List * GetExplicitTriggerCommandList(Oid relationId); extern List * GetExplicitTriggerIdList(Oid relationId); extern Oid get_relation_trigger_oid_compat(HeapTuple heapTuple); +extern void ErrorIfUnsupportedCreateTriggerCommand(CreateTrigStmt *createTriggerStmt); extern bool ShouldPropagateSetCommand(VariableSetStmt *setStmt); extern void PostprocessVariableSetStmt(VariableSetStmt *setStmt, const char *setCommand); diff --git a/src/test/regress/expected/create_table_triggers.out b/src/test/regress/expected/create_table_triggers.out new file mode 100644 index 000000000..b2eda9363 --- /dev/null +++ b/src/test/regress/expected/create_table_triggers.out @@ -0,0 +1,101 @@ +-- This test file includes tests to show that we do not allow triggers +-- on citus tables. Note that in other regression tests, we already test +-- the successfull citus table creation cases. +\set VERBOSITY terse +SET citus.next_shard_id TO 1505000; +CREATE SCHEMA table_triggers_schema; +SET search_path TO table_triggers_schema; +--------------------------------------------------------------------- +-- show that we do not allow trigger creation on citus tables +--------------------------------------------------------------------- +-- create a simple function to be invoked by triggers +CREATE FUNCTION update_value() RETURNS trigger AS $update_value$ +BEGIN + NEW.value := value+1 ; + RETURN NEW; +END; +$update_value$ LANGUAGE plpgsql; +CREATE TABLE distributed_table (value int); +SELECT create_distributed_table('distributed_table', 'value'); + create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +CREATE TABLE reference_table (value int); +SELECT create_reference_table('reference_table'); + create_reference_table +--------------------------------------------------------------------- + +(1 row) + +-- below two should fail +CREATE TRIGGER update_value_dist +AFTER INSERT ON distributed_table +FOR EACH ROW EXECUTE PROCEDURE update_value(); +ERROR: cannot create trigger on relation "distributed_table" because it is either a distributed table or a reference table +CREATE TRIGGER update_value_ref +AFTER INSERT ON reference_table +FOR EACH ROW EXECUTE PROCEDURE update_value(); +ERROR: cannot create trigger on relation "reference_table" because it is either a distributed table or a reference table +--------------------------------------------------------------------- +-- show that we do not allow creating citus tables if the +-- table has already triggers +--------------------------------------------------------------------- +CREATE TABLE distributed_table_1 (value int); +CREATE TRIGGER update_value_dist +AFTER INSERT ON distributed_table_1 +FOR EACH ROW EXECUTE PROCEDURE update_value(); +CREATE TABLE reference_table_1 (value int); +CREATE TRIGGER update_value_ref +AFTER INSERT ON reference_table_1 +FOR EACH ROW EXECUTE PROCEDURE update_value(); +-- below two should fail +SELECT create_distributed_table('distributed_table_1', 'value'); +ERROR: cannot distribute relation "distributed_table_1" because it has triggers +SELECT create_reference_table('reference_table_1'); +ERROR: cannot distribute relation "reference_table_1" because it has triggers +--------------------------------------------------------------------- +-- test deparse logic for CREATE TRIGGER commands +-- via master_get_table_ddl_events +--------------------------------------------------------------------- +CREATE TABLE test_table ( + id int, + text_number text, + text_col text +); +CREATE FUNCTION test_table_trigger_function() RETURNS trigger AS $test_table_trigger_function$ +BEGIN + RAISE EXCEPTION 'a meaningless exception'; +END; +$test_table_trigger_function$ LANGUAGE plpgsql; +-- in below two, use constraint triggers to test DEFERRABLE | NOT DEFERRABLE syntax +CREATE CONSTRAINT TRIGGER test_table_update + AFTER UPDATE OF id ON test_table + NOT DEFERRABLE + FOR EACH ROW + WHEN (OLD.* IS NOT DISTINCT FROM NEW.* AND OLD.text_number IS NOT NULL) + EXECUTE FUNCTION test_table_trigger_function(); +CREATE CONSTRAINT TRIGGER test_table_insert + AFTER INSERT ON test_table + DEFERRABLE INITIALLY IMMEDIATE + FOR EACH ROW + WHEN (NEW.id > 5 OR NEW.text_col IS NOT NULL AND NEW.id < to_number(NEW.text_number, '9999')) + EXECUTE FUNCTION test_table_trigger_function(); +CREATE TRIGGER test_table_delete + AFTER DELETE ON test_table + FOR EACH STATEMENT + EXECUTE FUNCTION test_table_trigger_function(); +SELECT master_get_table_ddl_events('test_table'); + master_get_table_ddl_events +--------------------------------------------------------------------- + CREATE TABLE table_triggers_schema.test_table (id integer, text_number text, text_col text) + ALTER TABLE table_triggers_schema.test_table OWNER TO postgres + CREATE TRIGGER test_table_delete AFTER DELETE ON table_triggers_schema.test_table FOR EACH STATEMENT EXECUTE FUNCTION table_triggers_schema.test_table_trigger_function() + CREATE CONSTRAINT TRIGGER test_table_insert AFTER INSERT ON table_triggers_schema.test_table DEFERRABLE INITIALLY IMMEDIATE FOR EACH ROW WHEN (((new.id OPERATOR(pg_catalog.>) 5) OR ((new.text_col IS NOT NULL) AND ((new.id)::numeric OPERATOR(pg_catalog.<) to_number(new.text_number, '9999'::text))))) EXECUTE FUNCTION table_triggers_schema.test_table_trigger_function() + CREATE CONSTRAINT TRIGGER test_table_update AFTER UPDATE OF id ON table_triggers_schema.test_table NOT DEFERRABLE INITIALLY IMMEDIATE FOR EACH ROW WHEN (((NOT (old.* IS DISTINCT FROM new.*)) AND (old.text_number IS NOT NULL))) EXECUTE FUNCTION table_triggers_schema.test_table_trigger_function() +(5 rows) + +-- cleanup at exit +DROP SCHEMA table_triggers_schema CASCADE; +NOTICE: drop cascades to 7 other objects diff --git a/src/test/regress/expected/create_table_triggers_0.out b/src/test/regress/expected/create_table_triggers_0.out new file mode 100644 index 000000000..780bb6b4b --- /dev/null +++ b/src/test/regress/expected/create_table_triggers_0.out @@ -0,0 +1,101 @@ +-- This test file includes tests to show that we do not allow triggers +-- on citus tables. Note that in other regression tests, we already test +-- the successfull citus table creation cases. +\set VERBOSITY terse +SET citus.next_shard_id TO 1505000; +CREATE SCHEMA table_triggers_schema; +SET search_path TO table_triggers_schema; +--------------------------------------------------------------------- +-- show that we do not allow trigger creation on citus tables +--------------------------------------------------------------------- +-- create a simple function to be invoked by triggers +CREATE FUNCTION update_value() RETURNS trigger AS $update_value$ +BEGIN + NEW.value := value+1 ; + RETURN NEW; +END; +$update_value$ LANGUAGE plpgsql; +CREATE TABLE distributed_table (value int); +SELECT create_distributed_table('distributed_table', 'value'); + create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +CREATE TABLE reference_table (value int); +SELECT create_reference_table('reference_table'); + create_reference_table +--------------------------------------------------------------------- + +(1 row) + +-- below two should fail +CREATE TRIGGER update_value_dist +AFTER INSERT ON distributed_table +FOR EACH ROW EXECUTE PROCEDURE update_value(); +ERROR: cannot create trigger on relation "distributed_table" because it is either a distributed table or a reference table +CREATE TRIGGER update_value_ref +AFTER INSERT ON reference_table +FOR EACH ROW EXECUTE PROCEDURE update_value(); +ERROR: cannot create trigger on relation "reference_table" because it is either a distributed table or a reference table +--------------------------------------------------------------------- +-- show that we do not allow creating citus tables if the +-- table has already triggers +--------------------------------------------------------------------- +CREATE TABLE distributed_table_1 (value int); +CREATE TRIGGER update_value_dist +AFTER INSERT ON distributed_table_1 +FOR EACH ROW EXECUTE PROCEDURE update_value(); +CREATE TABLE reference_table_1 (value int); +CREATE TRIGGER update_value_ref +AFTER INSERT ON reference_table_1 +FOR EACH ROW EXECUTE PROCEDURE update_value(); +-- below two should fail +SELECT create_distributed_table('distributed_table_1', 'value'); +ERROR: cannot distribute relation "distributed_table_1" because it has triggers +SELECT create_reference_table('reference_table_1'); +ERROR: cannot distribute relation "reference_table_1" because it has triggers +--------------------------------------------------------------------- +-- test deparse logic for CREATE TRIGGER commands +-- via master_get_table_ddl_events +--------------------------------------------------------------------- +CREATE TABLE test_table ( + id int, + text_number text, + text_col text +); +CREATE FUNCTION test_table_trigger_function() RETURNS trigger AS $test_table_trigger_function$ +BEGIN + RAISE EXCEPTION 'a meaningless exception'; +END; +$test_table_trigger_function$ LANGUAGE plpgsql; +-- in below two, use constraint triggers to test DEFERRABLE | NOT DEFERRABLE syntax +CREATE CONSTRAINT TRIGGER test_table_update + AFTER UPDATE OF id ON test_table + NOT DEFERRABLE + FOR EACH ROW + WHEN (OLD.* IS NOT DISTINCT FROM NEW.* AND OLD.text_number IS NOT NULL) + EXECUTE FUNCTION test_table_trigger_function(); +CREATE CONSTRAINT TRIGGER test_table_insert + AFTER INSERT ON test_table + DEFERRABLE INITIALLY IMMEDIATE + FOR EACH ROW + WHEN (NEW.id > 5 OR NEW.text_col IS NOT NULL AND NEW.id < to_number(NEW.text_number, '9999')) + EXECUTE FUNCTION test_table_trigger_function(); +CREATE TRIGGER test_table_delete + AFTER DELETE ON test_table + FOR EACH STATEMENT + EXECUTE FUNCTION test_table_trigger_function(); +SELECT master_get_table_ddl_events('test_table'); + master_get_table_ddl_events +--------------------------------------------------------------------- + CREATE TABLE table_triggers_schema.test_table (id integer, text_number text, text_col text) + ALTER TABLE table_triggers_schema.test_table OWNER TO postgres + CREATE TRIGGER test_table_delete AFTER DELETE ON table_triggers_schema.test_table FOR EACH STATEMENT EXECUTE PROCEDURE table_triggers_schema.test_table_trigger_function() + CREATE CONSTRAINT TRIGGER test_table_insert AFTER INSERT ON table_triggers_schema.test_table DEFERRABLE INITIALLY IMMEDIATE FOR EACH ROW WHEN (((new.id OPERATOR(pg_catalog.>) 5) OR ((new.text_col IS NOT NULL) AND ((new.id)::numeric OPERATOR(pg_catalog.<) to_number(new.text_number, '9999'::text))))) EXECUTE PROCEDURE table_triggers_schema.test_table_trigger_function() + CREATE CONSTRAINT TRIGGER test_table_update AFTER UPDATE OF id ON table_triggers_schema.test_table NOT DEFERRABLE INITIALLY IMMEDIATE FOR EACH ROW WHEN (((NOT (old.* IS DISTINCT FROM new.*)) AND (old.text_number IS NOT NULL))) EXECUTE PROCEDURE table_triggers_schema.test_table_trigger_function() +(5 rows) + +-- cleanup at exit +DROP SCHEMA table_triggers_schema CASCADE; +NOTICE: drop cascades to 7 other objects diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index 1d6ce622e..0b327b394 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -45,7 +45,7 @@ test: multi_create_table_constraints multi_master_protocol multi_load_data multi test: multi_behavioral_analytics_basics multi_behavioral_analytics_single_shard_queries multi_insert_select_non_pushable_queries multi_insert_select multi_behavioral_analytics_create_table_superuser test: multi_shard_update_delete recursive_dml_with_different_planners_executors test: insert_select_repartition window_functions dml_recursive multi_insert_select_window -test: multi_insert_select_conflict +test: multi_insert_select_conflict create_table_triggers test: multi_row_insert # following should not run in parallel because it relies on connection counts to workers diff --git a/src/test/regress/sql/create_table_triggers.sql b/src/test/regress/sql/create_table_triggers.sql new file mode 100644 index 000000000..8b5964fa1 --- /dev/null +++ b/src/test/regress/sql/create_table_triggers.sql @@ -0,0 +1,100 @@ +-- This test file includes tests to show that we do not allow triggers +-- on citus tables. Note that in other regression tests, we already test +-- the successfull citus table creation cases. + +\set VERBOSITY terse + +SET citus.next_shard_id TO 1505000; + +CREATE SCHEMA table_triggers_schema; +SET search_path TO table_triggers_schema; + +------------------------------------------------------------- +-- show that we do not allow trigger creation on citus tables +------------------------------------------------------------- + +-- create a simple function to be invoked by triggers +CREATE FUNCTION update_value() RETURNS trigger AS $update_value$ +BEGIN + NEW.value := value+1 ; + RETURN NEW; +END; +$update_value$ LANGUAGE plpgsql; + +CREATE TABLE distributed_table (value int); +SELECT create_distributed_table('distributed_table', 'value'); + +CREATE TABLE reference_table (value int); +SELECT create_reference_table('reference_table'); + +-- below two should fail +CREATE TRIGGER update_value_dist +AFTER INSERT ON distributed_table +FOR EACH ROW EXECUTE PROCEDURE update_value(); + +CREATE TRIGGER update_value_ref +AFTER INSERT ON reference_table +FOR EACH ROW EXECUTE PROCEDURE update_value(); + +--------------------------------------------------------- +-- show that we do not allow creating citus tables if the +-- table has already triggers +--------------------------------------------------------- + +CREATE TABLE distributed_table_1 (value int); + +CREATE TRIGGER update_value_dist +AFTER INSERT ON distributed_table_1 +FOR EACH ROW EXECUTE PROCEDURE update_value(); + +CREATE TABLE reference_table_1 (value int); + +CREATE TRIGGER update_value_ref +AFTER INSERT ON reference_table_1 +FOR EACH ROW EXECUTE PROCEDURE update_value(); + +-- below two should fail +SELECT create_distributed_table('distributed_table_1', 'value'); +SELECT create_reference_table('reference_table_1'); + +------------------------------------------------- +-- test deparse logic for CREATE TRIGGER commands +-- via master_get_table_ddl_events +------------------------------------------------- + +CREATE TABLE test_table ( + id int, + text_number text, + text_col text +); + +CREATE FUNCTION test_table_trigger_function() RETURNS trigger AS $test_table_trigger_function$ +BEGIN + RAISE EXCEPTION 'a meaningless exception'; +END; +$test_table_trigger_function$ LANGUAGE plpgsql; + +-- in below two, use constraint triggers to test DEFERRABLE | NOT DEFERRABLE syntax +CREATE CONSTRAINT TRIGGER test_table_update + AFTER UPDATE OF id ON test_table + NOT DEFERRABLE + FOR EACH ROW + WHEN (OLD.* IS NOT DISTINCT FROM NEW.* AND OLD.text_number IS NOT NULL) + EXECUTE FUNCTION test_table_trigger_function(); + +CREATE CONSTRAINT TRIGGER test_table_insert + AFTER INSERT ON test_table + DEFERRABLE INITIALLY IMMEDIATE + FOR EACH ROW + WHEN (NEW.id > 5 OR NEW.text_col IS NOT NULL AND NEW.id < to_number(NEW.text_number, '9999')) + EXECUTE FUNCTION test_table_trigger_function(); + +CREATE TRIGGER test_table_delete + AFTER DELETE ON test_table + FOR EACH STATEMENT + EXECUTE FUNCTION test_table_trigger_function(); + +SELECT master_get_table_ddl_events('test_table'); + +-- cleanup at exit +DROP SCHEMA table_triggers_schema CASCADE;