ne 2. 11. 2025 v 22:12 odesílatel Chao Li <[email protected]> napsal: > > Hi Philip, > > Thanks for the patch. I just reviewed it and got a few comments: > > > On Oct 23, 2025, at 06:27, Philip Alger <[email protected]> wrote: > > > > <v8-0001-Add-pg_get_trigger_ddl-function.patch> > > 1 > ``` > + /* Parse the trigger name to handle quoted identifiers */ > + nameList = textToQualifiedNameList(trgName); > + if (list_length(nameList) != 1) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("trigger name cannot be schema > qualified"))); > + > + DeconstructQualifiedName(nameList, &schemaName, &objName); > ``` > > As we have checked list length must be 1, so that schemaName must be NULL and > objName must be trgName, thus it doesn’t need to call > DeconstructQualifiedName(), and the local variable schemaName is not needed, > we can just do: > > ``` > objName = text_to_cstring(trgName); > ``` > > 2 > ``` > + appendStringInfo(&buf, "%s;", res); > ``` > > As we are only appending a single char “;”, appendStringInfoChar() is > cheaper.
+1 > > 3 > ``` > + initStringInfo(&buf); > + > + appendStringInfo(&buf, "%s;", res); > ``` > > I think it’s better to pfree(res). Would you mind to share why pfree is needed? I tried to trace this with Valgrind, but even pfree(res) was present or not, there was no leak detected and both compiles without additional warnings. Wouldn't be res "trashed" at the end of the function (after next line) anyway? > 4. I am just thinking that if this function need to check permissions like > has_table_privilege(relid, 'USAGE’), otherwise any user can query the DDL of > triggers on other users’ tables. > > 5. I wonder why this function is needed as there is pg_get_triggerdef() > already, only difference is that this function adds a tailing “;”. > > 6. I wonder if this function needs to add a third argument for pretty flag. > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > > > > >
