From c2c0b97a5b577bdbae9169d2d268b4cb6475c44f Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Thu, 6 Oct 2022 14:51:07 +0300 Subject: [PATCH] Retain trigger settings when re-creating the triggers (on shards) (#6398) Fixes https://github.com/citusdata/citus/issues/6394. DESCRIPTION: Fixes a bug that causes creating disabled-triggers on shards as enabled Since CREATE TRIGGER doesn't have syntax support to specify whether the trigger should be enabled/disabled, the underlying PG function (`pg_get_triggerdef()`) that we use to generate the command to create the trigger is not enough. For this reason, we append a second command to enable/disable trigger, right after creating it. We don't retain explicit extension dependencies set by using `ALTER trigger DEPENDS ON EXTENSION` commands too, but apparently right fix for that is to throw an error as in `PreprocessAlterTriggerDependsStmt()`; so, opened a separate PR to fix that #6399. (cherry picked from commit 86e186f671d05f0dce9d4fb4f27a91c182f1354f) --- src/backend/distributed/commands/trigger.c | 74 ++++++++ .../expected/citus_local_table_triggers.out | 24 +++ .../regress/expected/citus_table_triggers.out | 5 +- .../regress/expected/distributed_triggers.out | 162 +++++++++++++++--- .../sql/citus_local_table_triggers.sql | 10 ++ src/test/regress/sql/distributed_triggers.sql | 83 ++++++++- 6 files changed, 333 insertions(+), 25 deletions(-) diff --git a/src/backend/distributed/commands/trigger.c b/src/backend/distributed/commands/trigger.c index 3bb2417e7..6f46cf0dc 100644 --- a/src/backend/distributed/commands/trigger.c +++ b/src/backend/distributed/commands/trigger.c @@ -43,6 +43,7 @@ /* local function forward declarations */ +static char * GetAlterTriggerStateCommand(Oid triggerId); static bool IsCreateCitusTruncateTriggerStmt(CreateTrigStmt *createTriggerStmt); static String * GetAlterTriggerDependsTriggerNameValue(AlterObjectDependsStmt * alterTriggerDependsStmt); @@ -99,6 +100,18 @@ GetExplicitTriggerCommandList(Oid relationId) createTriggerCommandList = lappend( createTriggerCommandList, makeTableDDLCommandString(createTriggerCommand)); + + /* + * Appends the commands for the trigger settings that are not covered + * by CREATE TRIGGER command, such as ALTER TABLE ENABLE/DISABLE . + */ + + char *alterTriggerStateCommand = + GetAlterTriggerStateCommand(triggerId); + + createTriggerCommandList = lappend( + createTriggerCommandList, + makeTableDDLCommandString(alterTriggerStateCommand)); } /* revert back to original search_path */ @@ -108,6 +121,67 @@ GetExplicitTriggerCommandList(Oid relationId) } +/* + * GetAlterTriggerStateCommand returns the DDL command to set enable/disable + * state for given trigger. Throws an error if no such trigger exists. + */ +static char * +GetAlterTriggerStateCommand(Oid triggerId) +{ + StringInfo alterTriggerStateCommand = makeStringInfo(); + + bool missingOk = false; + HeapTuple triggerTuple = GetTriggerTupleById(triggerId, missingOk); + + Form_pg_trigger triggerForm = (Form_pg_trigger) GETSTRUCT(triggerTuple); + + char *qualifiedRelName = generate_qualified_relation_name(triggerForm->tgrelid); + const char *quotedTrigName = quote_identifier(NameStr(triggerForm->tgname)); + char enableDisableState = triggerForm->tgenabled; + + heap_freetuple(triggerTuple); + + const char *alterTriggerStateStr = NULL; + switch (enableDisableState) + { + case TRIGGER_FIRES_ON_ORIGIN: + { + /* default mode */ + alterTriggerStateStr = "ENABLE"; + break; + } + + case TRIGGER_FIRES_ALWAYS: + { + alterTriggerStateStr = "ENABLE ALWAYS"; + break; + } + + case TRIGGER_FIRES_ON_REPLICA: + { + alterTriggerStateStr = "ENABLE REPLICA"; + break; + } + + case TRIGGER_DISABLED: + { + alterTriggerStateStr = "DISABLE"; + break; + } + + default: + { + elog(ERROR, "unexpected trigger state"); + } + } + + appendStringInfo(alterTriggerStateCommand, "ALTER TABLE %s %s TRIGGER %s;", + qualifiedRelName, alterTriggerStateStr, quotedTrigName); + + return alterTriggerStateCommand->data; +} + + /* * GetTriggerTupleById returns copy of the heap tuple from pg_trigger for * the trigger with triggerId. If no such trigger exists, this function returns diff --git a/src/test/regress/expected/citus_local_table_triggers.out b/src/test/regress/expected/citus_local_table_triggers.out index 97b93e756..983b916e1 100644 --- a/src/test/regress/expected/citus_local_table_triggers.out +++ b/src/test/regress/expected/citus_local_table_triggers.out @@ -232,6 +232,30 @@ SELECT * FROM citus_local_table_triggers truncate_trigger_xxxxxxx | "interesting!schema"."citus_local!_table" | O (3 rows) +-- ALTER TABLE ENABLE REPLICA trigger +ALTER TABLE "interesting!schema"."citus_local!_table" ENABLE REPLICA TRIGGER "trigger\'name22"; +NOTICE: executing the command locally: SELECT worker_apply_shard_ddl_command (1507008, 'interesting!schema', E'ALTER TABLE "interesting!schema"."citus_local!_table" ENABLE REPLICA TRIGGER "trigger\\''name22";') +SELECT * FROM citus_local_table_triggers + WHERE tgname NOT LIKE 'RI_ConstraintTrigger%'; + tgname | tgrelid | tgenabled +--------------------------------------------------------------------- + trigger\'name22 | "interesting!schema"."citus_local!_table" | R + trigger\'name22_1507008 | "interesting!schema"."citus_local!_table_1507008" | R + truncate_trigger_xxxxxxx | "interesting!schema"."citus_local!_table" | O +(3 rows) + +-- ALTER TABLE ENABLE ALWAYS trigger +ALTER TABLE "interesting!schema"."citus_local!_table" ENABLE ALWAYS TRIGGER "trigger\'name22"; +NOTICE: executing the command locally: SELECT worker_apply_shard_ddl_command (1507008, 'interesting!schema', E'ALTER TABLE "interesting!schema"."citus_local!_table" ENABLE ALWAYS TRIGGER "trigger\\''name22";') +SELECT * FROM citus_local_table_triggers + WHERE tgname NOT LIKE 'RI_ConstraintTrigger%'; + tgname | tgrelid | tgenabled +--------------------------------------------------------------------- + trigger\'name22 | "interesting!schema"."citus_local!_table" | A + trigger\'name22_1507008 | "interesting!schema"."citus_local!_table_1507008" | A + truncate_trigger_xxxxxxx | "interesting!schema"."citus_local!_table" | O +(3 rows) + -- ALTER TABLE DISABLE trigger ALTER TABLE "interesting!schema"."citus_local!_table" DISABLE TRIGGER "trigger\'name22"; NOTICE: executing the command locally: SELECT worker_apply_shard_ddl_command (1507008, 'interesting!schema', E'ALTER TABLE "interesting!schema"."citus_local!_table" DISABLE TRIGGER "trigger\\''name22";') diff --git a/src/test/regress/expected/citus_table_triggers.out b/src/test/regress/expected/citus_table_triggers.out index bc61e863b..ef1229163 100644 --- a/src/test/regress/expected/citus_table_triggers.out +++ b/src/test/regress/expected/citus_table_triggers.out @@ -146,9 +146,12 @@ SELECT master_get_table_ddl_events('test_table'); 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() + ALTER TABLE table_triggers_schema.test_table ENABLE TRIGGER test_table_delete; CREATE CONSTRAINT TRIGGER test_table_insert AFTER INSERT ON table_triggers_schema.test_table DEFERRABLE INITIALLY IMMEDIATE FOR EACH ROW WHEN (((new.id > 5) OR ((new.text_col IS NOT NULL) AND ((new.id)::numeric < to_number(new.text_number, '9999'::text))))) EXECUTE FUNCTION table_triggers_schema.test_table_trigger_function() + ALTER TABLE table_triggers_schema.test_table ENABLE TRIGGER test_table_insert; 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) + ALTER TABLE table_triggers_schema.test_table ENABLE TRIGGER test_table_update; +(8 rows) -- cleanup at exit DROP SCHEMA table_triggers_schema CASCADE; diff --git a/src/test/regress/expected/distributed_triggers.out b/src/test/regress/expected/distributed_triggers.out index c53b91acb..80ea09465 100644 --- a/src/test/regress/expected/distributed_triggers.out +++ b/src/test/regress/expected/distributed_triggers.out @@ -855,7 +855,145 @@ SELECT run_command_on_workers($$SELECT count(*) FROM pg_trigger WHERE tgname lik (localhost,57638,t,1) (2 rows) -RESET client_min_messages; +CREATE TABLE "dist_\'table"(a int); +CREATE FUNCTION trigger_func() + RETURNS trigger + LANGUAGE plpgsql +AS $function$ +BEGIN + RETURN NULL; +END; +$function$; +CREATE TRIGGER default_mode_trigger +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); +CREATE TRIGGER "disabled_trigger\'" +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); +ALTER TABLE "dist_\'table" DISABLE trigger "disabled_trigger\'"; +CREATE TRIGGER replica_trigger +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); +ALTER TABLE "dist_\'table" ENABLE REPLICA trigger replica_trigger; +CREATE TRIGGER always_enabled_trigger +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); +ALTER TABLE "dist_\'table" ENABLE ALWAYS trigger always_enabled_trigger; +CREATE TRIGGER noop_enabled_trigger +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); +ALTER TABLE "dist_\'table" ENABLE trigger noop_enabled_trigger; +SELECT create_distributed_table('dist_\''table', 'a'); + create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +SELECT bool_and(tgenabled = 'O') FROM pg_trigger WHERE tgname LIKE 'default_mode_trigger%'; + bool_and +--------------------------------------------------------------------- + t +(1 row) + +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'O') FROM pg_trigger WHERE tgname LIKE 'default_mode_trigger%'$$); + run_command_on_workers +--------------------------------------------------------------------- + (localhost,57637,t,t) + (localhost,57638,t,t) +(2 rows) + +SELECT bool_and(tgenabled = 'D') FROM pg_trigger WHERE tgname LIKE 'disabled_trigger%'; + bool_and +--------------------------------------------------------------------- + t +(1 row) + +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'D') FROM pg_trigger WHERE tgname LIKE 'disabled_trigger%'$$); + run_command_on_workers +--------------------------------------------------------------------- + (localhost,57637,t,t) + (localhost,57638,t,t) +(2 rows) + +SELECT bool_and(tgenabled = 'R') FROM pg_trigger WHERE tgname LIKE 'replica_trigger%'; + bool_and +--------------------------------------------------------------------- + t +(1 row) + +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'R') FROM pg_trigger WHERE tgname LIKE 'replica_trigger%'$$); + run_command_on_workers +--------------------------------------------------------------------- + (localhost,57637,t,t) + (localhost,57638,t,t) +(2 rows) + +SELECT bool_and(tgenabled = 'A') FROM pg_trigger WHERE tgname LIKE 'always_enabled_trigger%'; + bool_and +--------------------------------------------------------------------- + t +(1 row) + +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'A') FROM pg_trigger WHERE tgname LIKE 'always_enabled_trigger%'$$); + run_command_on_workers +--------------------------------------------------------------------- + (localhost,57637,t,t) + (localhost,57638,t,t) +(2 rows) + +SELECT bool_and(tgenabled = 'O') FROM pg_trigger WHERE tgname LIKE 'noop_enabled_trigger%'; + bool_and +--------------------------------------------------------------------- + t +(1 row) + +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'O') FROM pg_trigger WHERE tgname LIKE 'noop_enabled_trigger%'$$); + run_command_on_workers +--------------------------------------------------------------------- + (localhost,57637,t,t) + (localhost,57638,t,t) +(2 rows) + +CREATE TABLE citus_local(a int); +CREATE FUNCTION citus_local_trig_func() + RETURNS trigger + LANGUAGE plpgsql +AS $function$ +BEGIN + RETURN NULL; +END; +$function$; +CREATE TRIGGER citus_local_trig +AFTER UPDATE OR DELETE ON citus_local +FOR STATEMENT EXECUTE FUNCTION citus_local_trig_func(); +-- make sure that trigger is initially not disabled +SELECT tgenabled = 'D' FROM pg_trigger WHERE tgname LIKE 'citus_local_trig%'; + ?column? +--------------------------------------------------------------------- + f +(1 row) + +ALTER TABLE citus_local DISABLE trigger citus_local_trig; +SELECT citus_add_local_table_to_metadata('citus_local'); + citus_add_local_table_to_metadata +--------------------------------------------------------------------- + +(1 row) + +SELECT bool_and(tgenabled = 'D') FROM pg_trigger WHERE tgname LIKE 'citus_local_trig%'; + bool_and +--------------------------------------------------------------------- + t +(1 row) + +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'D') FROM pg_trigger WHERE tgname LIKE 'citus_local_trig%'$$); + run_command_on_workers +--------------------------------------------------------------------- + (localhost,57637,t,t) + (localhost,57638,t,t) +(2 rows) + +SET client_min_messages TO ERROR; RESET citus.enable_unsafe_triggers; SELECT run_command_on_workers('ALTER SYSTEM RESET citus.enable_unsafe_triggers;'); run_command_on_workers @@ -873,25 +1011,3 @@ SELECT run_command_on_workers('SELECT pg_reload_conf();'); SET citus.log_remote_commands TO off; DROP SCHEMA distributed_triggers CASCADE; -NOTICE: drop cascades to 21 other objects -DETAIL: drop cascades to table data -drop cascades to function record_change() -drop cascades to function insert_delete_document(text,text) -drop cascades to function bad_shardkey_record_change() -drop cascades to function remote_shardkey_record_change() -drop cascades to function insert_document(text,text) -drop cascades to table emptest -drop cascades to table emptest_audit -drop cascades to function process_emp_audit() -drop cascades to view emp_triggers -drop cascades to table record_op -drop cascades to function record_emp() -drop cascades to table data_changes -drop cascades to table sale -drop cascades to table record_sale -drop cascades to function record_sale() -drop cascades to view sale_triggers -drop cascades to extension seg -drop cascades to table distributed_table -drop cascades to table distributed_table_change -drop cascades to function insert_99() diff --git a/src/test/regress/sql/citus_local_table_triggers.sql b/src/test/regress/sql/citus_local_table_triggers.sql index ed53fefad..93427d063 100644 --- a/src/test/regress/sql/citus_local_table_triggers.sql +++ b/src/test/regress/sql/citus_local_table_triggers.sql @@ -185,6 +185,16 @@ ALTER TRIGGER "trigger\'name" ON "interesting!schema"."citus_local!_table" RENAM SELECT * FROM citus_local_table_triggers WHERE tgname NOT LIKE 'RI_ConstraintTrigger%'; +-- ALTER TABLE ENABLE REPLICA trigger +ALTER TABLE "interesting!schema"."citus_local!_table" ENABLE REPLICA TRIGGER "trigger\'name22"; +SELECT * FROM citus_local_table_triggers + WHERE tgname NOT LIKE 'RI_ConstraintTrigger%'; + +-- ALTER TABLE ENABLE ALWAYS trigger +ALTER TABLE "interesting!schema"."citus_local!_table" ENABLE ALWAYS TRIGGER "trigger\'name22"; +SELECT * FROM citus_local_table_triggers + WHERE tgname NOT LIKE 'RI_ConstraintTrigger%'; + -- ALTER TABLE DISABLE trigger ALTER TABLE "interesting!schema"."citus_local!_table" DISABLE TRIGGER "trigger\'name22"; SELECT * FROM citus_local_table_triggers diff --git a/src/test/regress/sql/distributed_triggers.sql b/src/test/regress/sql/distributed_triggers.sql index 2194d94d8..c6b4a2054 100644 --- a/src/test/regress/sql/distributed_triggers.sql +++ b/src/test/regress/sql/distributed_triggers.sql @@ -473,7 +473,88 @@ SELECT * FROM distributed_table_change; SELECT tgrelid::regclass::text, tgname FROM pg_trigger WHERE tgname like 'insert_99_trigger%' ORDER BY 1,2; SELECT run_command_on_workers($$SELECT count(*) FROM pg_trigger WHERE tgname like 'insert_99_trigger%'$$); -RESET client_min_messages; +CREATE TABLE "dist_\'table"(a int); + +CREATE FUNCTION trigger_func() + RETURNS trigger + LANGUAGE plpgsql +AS $function$ +BEGIN + RETURN NULL; +END; +$function$; + +CREATE TRIGGER default_mode_trigger +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); + +CREATE TRIGGER "disabled_trigger\'" +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); + +ALTER TABLE "dist_\'table" DISABLE trigger "disabled_trigger\'"; + +CREATE TRIGGER replica_trigger +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); + +ALTER TABLE "dist_\'table" ENABLE REPLICA trigger replica_trigger; + +CREATE TRIGGER always_enabled_trigger +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); + +ALTER TABLE "dist_\'table" ENABLE ALWAYS trigger always_enabled_trigger; + +CREATE TRIGGER noop_enabled_trigger +AFTER UPDATE OR DELETE ON "dist_\'table" +FOR STATEMENT EXECUTE FUNCTION trigger_func(); + +ALTER TABLE "dist_\'table" ENABLE trigger noop_enabled_trigger; + +SELECT create_distributed_table('dist_\''table', 'a'); + +SELECT bool_and(tgenabled = 'O') FROM pg_trigger WHERE tgname LIKE 'default_mode_trigger%'; +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'O') FROM pg_trigger WHERE tgname LIKE 'default_mode_trigger%'$$); + +SELECT bool_and(tgenabled = 'D') FROM pg_trigger WHERE tgname LIKE 'disabled_trigger%'; +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'D') FROM pg_trigger WHERE tgname LIKE 'disabled_trigger%'$$); + +SELECT bool_and(tgenabled = 'R') FROM pg_trigger WHERE tgname LIKE 'replica_trigger%'; +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'R') FROM pg_trigger WHERE tgname LIKE 'replica_trigger%'$$); + +SELECT bool_and(tgenabled = 'A') FROM pg_trigger WHERE tgname LIKE 'always_enabled_trigger%'; +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'A') FROM pg_trigger WHERE tgname LIKE 'always_enabled_trigger%'$$); + +SELECT bool_and(tgenabled = 'O') FROM pg_trigger WHERE tgname LIKE 'noop_enabled_trigger%'; +SELECT run_command_on_workers($$SELECT bool_and(tgenabled = 'O') FROM pg_trigger WHERE tgname LIKE 'noop_enabled_trigger%'$$); + +CREATE TABLE citus_local(a int); + +CREATE FUNCTION citus_local_trig_func() + RETURNS trigger + LANGUAGE plpgsql +AS $function$ +BEGIN + RETURN NULL; +END; +$function$; + +CREATE TRIGGER citus_local_trig +AFTER UPDATE OR DELETE ON citus_local +FOR STATEMENT EXECUTE FUNCTION citus_local_trig_func(); + +-- make sure that trigger is initially not disabled +SELECT tgenabled = 'D' FROM pg_trigger WHERE tgname LIKE 'citus_local_trig%'; + +ALTER TABLE citus_local DISABLE trigger citus_local_trig; + +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%'$$); + +SET client_min_messages TO ERROR; RESET citus.enable_unsafe_triggers; SELECT run_command_on_workers('ALTER SYSTEM RESET citus.enable_unsafe_triggers;'); SELECT run_command_on_workers('SELECT pg_reload_conf();');