Re: Rename of triggers for partitioned tables

Hi,

From: Zhihong Yu <zyu@yugabyte.com>

Sent: Monday, June 28, 2021 15:28
Subject: Re: Rename of triggers for partitioned tables

 

> -                                void *arg)

> +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