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. 3 ``` + initStringInfo(&buf); + + appendStringInfo(&buf, "%s;", res); ``` I think it’s better to pfree(res). 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/
