ne 2. 11. 2025 v 22:12 odesílatel Chao Li <li.evan.chao@gmail.com> 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 <paalger0@gmail.com> 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’tneed 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’),
otherwiseany 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
functionadds 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/
>
>
>
>
>
>
>
>
