On Mon, Jun 28, 2021 at 11:45 AM Arne Roland <a.rol...@index.de> wrote:
> Hi, > > *From:* Zhihong Yu <z...@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.
