Re: Rename of triggers for partitioned tables - Mailing list pgsql-hackers

From Zhihong Yu
Subject Re: Rename of triggers for partitioned tables
Date
Msg-id CALNJ-vRSVC6+zPS-d_ZozoizcRYb_DENX1T_PP09e2m33BdVmg@mail.gmail.com
Whole thread Raw
In response to Re: Rename of triggers for partitioned tables  (Arne Roland <A.Roland@index.de>)
List pgsql-hackers


On Mon, Jun 28, 2021 at 11:45 AM Arne Roland <A.Roland@index.de> wrote:
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

Adding mailing list. 

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: [PATCH] Make jsonapi usable from libpq
Next
From: Andrew Dunstan
Date:
Subject: pg_indent instructions