> +callbackForRenameTrigger(char *relname, Oid relid)
>
> Since this method is only called by RangeVarCallbackForRenameTrigger(), it seems the method can be folded into RangeVarCallbackForRenameTrigger.
that seems obvious. I have no idea why I didn't do that.
> + * renametrig - changes the name of a trigger on a possibly partitioned relation recurisvely
> ...
> +renametrigHelper(RenameStmt *stmt, Oid tgoid, Oid relid)
>
> Comment doesn't match the name of method. I think it is better to use _ instead of camel case.
The other functions in this file seem to be in mixed case (camel case with lower first character). I don't have a strong point either way, but the consistency seems to be worth it to me.
> -renametrig(RenameStmt *stmt)
> +renameonlytrig(RenameStmt *stmt, Relation tgrel, Oid relid, Oid tgparent)
>
> Would renametrig_unlocked be better name for the method ?
I think the name renametrig_unlocked might be confusing. I am not sure my name is good. But upon calling this function everything is locked already and stays locked. So unlocked seems a confusing name to me. renameOnlyOneTriggerWithoutAccountingForChildrenWhereAllLocksAreAquiredAlready sadly isn't very concise anymore. Do you have another idea?
> strcmp(stmt->subname, NameStr(trigform->tgname)) can be saved in a variable since it is used after the if statement.
It's always needed later. I did miss it due to the short circuiting expression. Thanks!
> + tgrel = table_open(TriggerRelationId, RowExclusiveLock);
> ...
> + ReleaseSysCache(tuple); /* We are still holding the
> + * AccessExclusiveLock. */
>
> The lock mode in comment doesn't seem to match the lock taken.
I made the comment slightly more verbose. I hope it's clear now.
Thank you very much for the feedback! I attached a new version of the patch based on your suggestions.
Regards
Arne
