Re: Rename of triggers for partitioned tables

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.