From d7f41cacbe52c756279b32ae39c95f47b59261fc Mon Sep 17 00:00:00 2001 From: Naisila Puka <37271756+naisila@users.noreply.github.com> Date: Tue, 6 Sep 2022 12:19:25 +0300 Subject: [PATCH] Prohibit renaming child trigger on distributed partition pre PG15 (#6290) Pre PG15, renaming child triggers on partitions is allowed. When creating a trigger in a distributed parent partitioned table, the triggers on the shards of the partitions have the same name with the triggers on the corresponding parent shards of the parent table. Therefore, they don't have the same appended shard id as the shard id of the partition. Hence, when trying to rename a child trigger on a partition of a distributed table, we can't correctly find the triggers on the shards of the partition in order to rename them since we append a different shard id to the name of the trigger. Since we can't find the trigger we get a misleading error of inexistent trigger. In this commit we prohibit renaming child triggers on distributed partitions altogether. --- .../commands/distribute_object_ops.c | 2 +- src/backend/distributed/commands/trigger.c | 95 +++++++++++++++++++ src/include/distributed/commands.h | 2 + .../upgrade_distributed_triggers_before.out | 3 + .../upgrade_distributed_triggers_before.sql | 3 + 5 files changed, 104 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/commands/distribute_object_ops.c b/src/backend/distributed/commands/distribute_object_ops.c index 198d1df9a..74a7ce69b 100644 --- a/src/backend/distributed/commands/distribute_object_ops.c +++ b/src/backend/distributed/commands/distribute_object_ops.c @@ -1176,7 +1176,7 @@ static DistributeObjectOps View_Rename = { static DistributeObjectOps Trigger_Rename = { .deparse = NULL, .qualify = NULL, - .preprocess = NULL, + .preprocess = PreprocessAlterTriggerRenameStmt, .operationType = DIST_OPS_ALTER, .postprocess = PostprocessAlterTriggerRenameStmt, .address = NULL, diff --git a/src/backend/distributed/commands/trigger.c b/src/backend/distributed/commands/trigger.c index ffb0ab5ae..3bb2417e7 100644 --- a/src/backend/distributed/commands/trigger.c +++ b/src/backend/distributed/commands/trigger.c @@ -53,6 +53,9 @@ static void ExtractDropStmtTriggerAndRelationName(DropStmt *dropTriggerStmt, char **relationName); static void ErrorIfDropStmtDropsMultipleTriggers(DropStmt *dropTriggerStmt); static int16 GetTriggerTypeById(Oid triggerId); +#if (PG_VERSION_NUM < PG_VERSION_15) +static void ErrorOutIfCloneTrigger(Oid tgrelid, const char *tgname); +#endif /* GUC that overrides trigger checks for distributed tables and reference tables */ @@ -319,6 +322,40 @@ CreateTriggerEventExtendNames(CreateTrigStmt *createTriggerStmt, char *schemaNam } +/* + * PreprocessAlterTriggerRenameStmt is called before a ALTER TRIGGER RENAME + * command has been executed by standard process utility. This function errors + * out if we are trying to rename a child trigger on a partition of a distributed + * table. In PG15, this is not allowed anyway. + */ +List * +PreprocessAlterTriggerRenameStmt(Node *node, const char *queryString, + ProcessUtilityContext processUtilityContext) +{ +#if (PG_VERSION_NUM < PG_VERSION_15) + RenameStmt *renameTriggerStmt = castNode(RenameStmt, node); + Assert(renameTriggerStmt->renameType == OBJECT_TRIGGER); + + RangeVar *relation = renameTriggerStmt->relation; + + bool missingOk = false; + Oid relationId = RangeVarGetRelid(relation, ALTER_TRIGGER_LOCK_MODE, missingOk); + + if (!IsCitusTable(relationId)) + { + return NIL; + } + + EnsureCoordinator(); + ErrorOutForTriggerIfNotSupported(relationId); + + ErrorOutIfCloneTrigger(relationId, renameTriggerStmt->subname); +#endif + + return NIL; +} + + /* * PostprocessAlterTriggerRenameStmt is called after a ALTER TRIGGER RENAME * command has been executed by standard process utility. This function errors @@ -611,6 +648,64 @@ ErrorOutForTriggerIfNotSupported(Oid relationId) } +#if (PG_VERSION_NUM < PG_VERSION_15) + +/* + * ErrorOutIfCloneTrigger is a helper function to error + * out if we are trying to rename a child trigger on a + * partition of a distributed table. + * A lot of this code is borrowed from PG15 because + * renaming clone triggers isn't allowed in PG15 anymore. + */ +static void +ErrorOutIfCloneTrigger(Oid tgrelid, const char *tgname) +{ + HeapTuple tuple; + ScanKeyData key[2]; + + Relation tgrel = table_open(TriggerRelationId, RowExclusiveLock); + + /* + * Search for the trigger to modify. + */ + ScanKeyInit(&key[0], + Anum_pg_trigger_tgrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(tgrelid)); + ScanKeyInit(&key[1], + Anum_pg_trigger_tgname, + BTEqualStrategyNumber, F_NAMEEQ, + CStringGetDatum(tgname)); + SysScanDesc tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, + NULL, 2, key); + + if (HeapTupleIsValid(tuple = systable_getnext(tgscan))) + { + Form_pg_trigger trigform = (Form_pg_trigger) GETSTRUCT(tuple); + + /* + * If the trigger descends from a trigger on a parent partitioned + * table, reject the rename. + * Appended shard ids to find the trigger on the partition's shards + * are not correct. Hence we would fail to find the trigger on the + * partition's shard. + */ + if (OidIsValid(trigform->tgparentid)) + { + ereport(ERROR, ( + errmsg( + "cannot rename child triggers on distributed partitions"))); + } + } + + systable_endscan(tgscan); + table_close(tgrel, RowExclusiveLock); +} + + +#endif + + /* * GetDropTriggerStmtRelation takes a DropStmt for a trigger object and returns * RangeVar for the relation that owns the trigger. diff --git a/src/include/distributed/commands.h b/src/include/distributed/commands.h index b7030adee..f660fe19d 100644 --- a/src/include/distributed/commands.h +++ b/src/include/distributed/commands.h @@ -667,6 +667,8 @@ extern List * CreateTriggerStmtObjectAddress(Node *node, bool missingOk, bool isPostprocess); extern void CreateTriggerEventExtendNames(CreateTrigStmt *createTriggerStmt, char *schemaName, uint64 shardId); +extern List * PreprocessAlterTriggerRenameStmt(Node *node, const char *queryString, + ProcessUtilityContext processUtilityContext); extern List * PostprocessAlterTriggerRenameStmt(Node *node, const char *queryString); extern void AlterTriggerRenameEventExtendNames(RenameStmt *renameTriggerStmt, char *schemaName, uint64 shardId); diff --git a/src/test/regress/expected/upgrade_distributed_triggers_before.out b/src/test/regress/expected/upgrade_distributed_triggers_before.out index 5e369c84c..d049c2b0a 100644 --- a/src/test/regress/expected/upgrade_distributed_triggers_before.out +++ b/src/test/regress/expected/upgrade_distributed_triggers_before.out @@ -241,3 +241,6 @@ SELECT * FROM sale_triggers ORDER BY 1, 2; truncate_trigger_xxxxxxx | sale_california | O (12 rows) +-- check that we can't rename child triggers on partitions of distributed tables +ALTER TRIGGER another_trigger ON sale_newyork RENAME TO another_renamed_trigger; +ERROR: cannot rename child triggers on distributed partitions diff --git a/src/test/regress/sql/upgrade_distributed_triggers_before.sql b/src/test/regress/sql/upgrade_distributed_triggers_before.sql index a1d4c9db3..9e0e5b7bb 100644 --- a/src/test/regress/sql/upgrade_distributed_triggers_before.sql +++ b/src/test/regress/sql/upgrade_distributed_triggers_before.sql @@ -114,3 +114,6 @@ FOR EACH ROW EXECUTE FUNCTION upgrade_distributed_triggers.record_sale(); ALTER TRIGGER another_trigger ON sale RENAME TO another_renamed_trigger; SELECT * FROM sale_triggers ORDER BY 1, 2; + +-- check that we can't rename child triggers on partitions of distributed tables +ALTER TRIGGER another_trigger ON sale_newyork RENAME TO another_renamed_trigger;