Обсуждение: [PATCH] Add pg_get_trigger_ddl() to retrieve the CREATE TRIGGER statement

On Tue, Oct 14, 2025 at 9:59 AM Philip Alger <paalger0@gmail.com> wrote:
>
> Hello,
>
> I am submitting patch as a part of a larger Retail DDL functions project described by Andrew Dunstan here:
https://www.postgresql.org/message-id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net
>
> This patch creates a function pg_get_trigger_ddl, designed to retrieve the full DDL statement for a trigger. Users
canobtain the DDL by providing the table and trigger names, like the following: 
>
>     SELECT pg_get_trigger_ddl('my_table_name', 'my_trigger_name');
>
> While pg_get_triggerdef currently provides a similar SQL statement, it requires the trigger's OID, making it less
convenient.This function simplifies this by allowing direct input of the table and trigger names, eliminating the need
tofind the OID beforehand. I opted not to include the "pretty" formatting capability that pg_get_triggerdef offers. 
>
> This patch includes documentation, comments, and regression tests, all of which have run successfully.
>

I just did a quick test.

src1=# SELECT pg_get_trigger_ddl(2, 'foo_trigger');
ERROR:  trigger "foo_trigger" for table "(null)" does not exist
src1=# SELECT pg_get_trigger_ddl(0, 'foo_trigger');
ERROR:  trigger "foo_trigger" for table "(null)" does not exist

this error message is use facing, is the above error message what we expected?

On Tue, Oct 14, 2025 at 12:03 PM Philip Alger <paalger0@gmail.com> wrote:
>
>
>
> On Mon, Oct 13, 2025 at 9:28 PM jian he <jian.universality@gmail.com> wrote:
>>
>>
>> I just did a quick test.
>>
>> src1=# SELECT pg_get_trigger_ddl(2, 'foo_trigger');
>> ERROR:  trigger "foo_trigger" for table "(null)" does not exist
>> src1=# SELECT pg_get_trigger_ddl(0, 'foo_trigger');
>> ERROR:  trigger "foo_trigger" for table "(null)" does not exist
>>
>> this error message is use facing, is the above error message what we expected?
>
>
> Thank you for checking that. Short answer: no.
>
> Please see v2. The latest version should take care of the (null) relation issue now, since it is checking if the OID
existsfor the table. I've included a test for that as well. It should return a clearer error if the relation does not
exist.
>

select  pg_get_trigger_ddl(-1, 'h');
ERROR:  relation with OID 4294967295 does not exist

this error obviously is not good.
we can follow the approach used by pg_get_viewdef(oid)

On 2025-Oct-14, jian he wrote:

> select  pg_get_trigger_ddl(-1, 'h');
> ERROR:  relation with OID 4294967295 does not exist
> 
> this error obviously is not good.
> we can follow the approach used by pg_get_viewdef(oid)

Hmm.  For pg_get_viewdef() we have two functions:

{ oid => '1640', descr => 'select statement of a view',
  proname => 'pg_get_viewdef', provolatile => 's', proparallel => 'r',
  prorettype => 'text', proargtypes => 'text',
  prosrc => 'pg_get_viewdef_name' },

{ oid => '1641', descr => 'select statement of a view',
  proname => 'pg_get_viewdef', provolatile => 's', proparallel => 'r',
  prorettype => 'text', proargtypes => 'oid', prosrc => 'pg_get_viewdef' },

one of which takes a 'name' reference the table, and the other takes
OID.  I suspect this arrangement predates the 'regclass' business ... 

   git show 52200befd0^:src/backend/utils/adt/ruleutils.c

yep, it does.  I think we wouldn't do it this way nowadays.  I think the
choice to implement pg_get_trigger_ddl(regclass) is a good one.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801

+{ oid => '9569', descr => 'get CREATE statement for a trigger',
+  proname => 'pg_get_trigger_ddl', proisstrict => 't', prorettype => 'text',
+  proargtypes => 'regclass name', prosrc => 'pg_get_trigger_ddl' },

your documentation and the function's comment specifically say that the
function take a trigger name and a table name, so it should not use
regclass type, which allows OID as input as well. 

There is already a family of pg_get_[xxx]def functions available in
PostgreSQL. pg_get_triggerdef() being one of them and it already can take
OID as input and output the same text, so regclass type is not necessary. 

Consistency is important in PostgreSQL, so instead of creating a new
function pg_get_trigger_ddl(), I think it is better to just overload the
original pg_get_triggerdef() function by adding another version of it that
takes trigger and table name instead and you should keep the pretty
boolean argument as well for consistency.

Personally, I think the existing pg_triggerdef() functions shall be sufficient,
as it is relatively easy to look up a trigger OID, but perhaps in some
scenarios using trigger name + table name may be more convenient. 

Again for consistency, in addition to triggers, there are other functions like
pg_get_viewdef, and pg_get_indexdef that take OID and return the creating
commands for a view and index, these can also have the same variant of
taking view/index name and table name just like triggers.

+/* ----------
+ * pg_get_trigger_ddl - Get the DDL statement for a trigger
+ *
+ * This function retrieves the DDL statement for a specified trigger given a
+ * table name and trigger name. It uses the pg_get_triggerdef_worker function
+ * to perform the actual retrieval of the DDL statement. This function allows
+ * users to obtain the DDL definition of a trigger in a convenient manner using
+ * the trigger's name and the table it belongs to, rather than having to
+ * look up the trigger OID first to obtain the definition.
+ * ----------
+ */

The term "DDL statement" may be a little misleading here, it does not return
the actual DDL statements executed to create the trigger. The documentation for
"pg_get_triggerdef" calls this statement as follows :

"the creating command for a trigger. (This is a decompiled reconstruction,
not the original text of the command.)"


Cary

Apologies, I forgot to add a new version of the patch with the documentation change.

This is my first time doing this.

your documentation and the function's comment specifically say that the
function take a trigger name and a table name, so it should not use
regclass type, which allows OID as input as well.

Thanks for pointing that out in the documentation.

I think the regclass type actually makes it easier to use because you can input a name or OID, but you're right in that it is essentially 'table_name'::regclass::oid. However, isn't the point of the regclass type to make it easier to do table lookups? In that case, using a name seems easier.

I think you should change the documentation. It seems better to use the regfoo types where available to save a lot a code duplication.

There is already a family of pg_get_[xxx]def functions available in
PostgreSQL. pg_get_triggerdef() being one of them and it already can take
OID as input and output the same text, so regclass type is not necessary.

True, but you have to look for the trigger OID. If you have more than one table using the same trigger name, then you have to figure that out as well. Using names over OIDs seems more intuitive and less error prone than having to look up all the OIDs in my opinion. Also, having the added option of using an OID as well shouldn't be frowned upon since that's what it's using under the hood with regclass, but I understand what you're saying about pg_get_triggerdef(OID) doing the same thing with the OID only.

Yes, what this function buys us anything is that you don't need to get the trigger OID.

 

The term "DDL statement" may be a little misleading here, it does not return
the actual DDL statements executed to create the trigger. The documentation for
"pg_get_triggerdef" calls this statement as follows :

"the creating command for a trigger. (This is a decompiled reconstruction,
not the original text of the command.)"

True, Cary. Appreciate calling that out. I can fix that in the documentation as well. 

by "DDL statement" we mean a statement that would create the object as it exists now if it were not already present, not the original creation statement. I don't think we need to state that all over the place.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, Oct 14, 2025 at 9:14 PM Philip Alger <paalger0@gmail.com> wrote:
>
>> select  pg_get_trigger_ddl(-1, 'h');
>> ERROR:  relation with OID 4294967295 does not exist
>>
>> this error obviously is not good.
>> we can follow the approach used by pg_get_viewdef(oid)
>
> But isn't that an edge case? Would a user really pass in an arbitrary number like -1? That seems counterintuitive.
>

hi.

I complained the same issue for pg_basetype at
https://www.postgresql.org/message-id/3759807.1711658868%40sss.pgh.pa.us

maybe we can return NULL for
select pg_get_trigger_ddl(-1, 'h');

Hi Phil,

Thanks for the patch.

On 10/15/25 23:25, Philip Alger wrote:
> I've updated v4, attached here.

The function fails to look up triggers with quoted names

db=# CREATE TABLE t (c int);
CREATE TABLE

db=# CREATE FUNCTION trgf()
RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
  RETURN NULL;
END; $$;
CREATE FUNCTION

db=# CREATE TRIGGER "Foo"
BEFORE INSERT ON t
FOR EACH STATEMENT EXECUTE PROCEDURE trgf();
CREATE TRIGGER

db=# SELECT pg_get_trigger_ddl('t','"Foo"');
ERROR:  trigger ""Foo"" for table "t" does not exist

The same applies for unicode trigger names:

db=# CREATE TRIGGER "🐘"
BEFORE INSERT ON t
FOR EACH STATEMENT EXECUTE PROCEDURE trgf();
CREATE TRIGGER

db=# SELECT pg_get_trigger_ddl('t','"🐘"');
ERROR:  trigger ""🐘"" for table "t" does not exist

db=# \d t
                 Table "public.t"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 c      | integer |           |          |
Triggers:
    "Foo" BEFORE INSERT ON t FOR EACH STATEMENT EXECUTE FUNCTION trgf()
    "🐘" BEFORE INSERT ON t FOR EACH STATEMENT EXECUTE FUNCTION trgf()


(it does work if we omit the double quotes)

postgres=# SELECT pg_get_trigger_ddl('t','Foo');
                                     pg_get_trigger_ddl

--------------------------------------------------------------------------------------------
 CREATE TRIGGER "Foo" BEFORE INSERT ON public.t FOR EACH STATEMENT
EXECUTE FUNCTION trgf();
(1 row)

postgres=# SELECT pg_get_trigger_ddl('t','🐘');
                                    pg_get_trigger_ddl

-------------------------------------------------------------------------------------------
 CREATE TRIGGER "🐘" BEFORE INSERT ON public.t FOR EACH STATEMENT
EXECUTE FUNCTION trgf();
(1 row)


I don't think it's the expected behaviour. For instance,
pg_get_viewdef() sees it differently (opposite approach):


postgres=# CREATE TEMPORARY VIEW "MyView" AS SELECT 42;
CREATE VIEW

postgres=# SELECT pg_get_viewdef('"MyView"');
      pg_get_viewdef
---------------------------
  SELECT 42 AS "?column?";
(1 row)

postgres=# SELECT pg_get_viewdef('MyView');
ERROR:  relation "myview" does not exist


Best, Jim

On 10/16/25 16:20, Philip Alger wrote:
>     pg_get_viewdef() sees it differently (opposite approach)
> 
> That's true, and it's pretty strict. However, pg_get_trigger_ddl seems
> more intuitive since it can return the statement whether the trigger is
> quoted or unquoted without the user thinking about adding quotes.


I can see how it can be more practical to not care about double quotes
when using pg_get_trigger_ddl(), but IMHO consistency and predictability
are more important in this particular case. If we do this, users would
need to know where to keep or remove the double quotes when using
functions to describe catalog objects. Another argument **for** my case
is the following example:

postgres=# CREATE SCHEMA "S";
CREATE SCHEMA

postgres=# CREATE TABLE "S"."T" (c int);
CREATE TABLE

postgres=# CREATE FUNCTION "S".trgf()
RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
  RETURN NULL;
END; $$;
CREATE FUNCTION

postgres=# CREATE TRIGGER trg BEFORE INSERT ON "S"."T"
FOR EACH STATEMENT EXECUTE PROCEDURE "S".trgf();
CREATE TRIGGER

postgres=# SELECT pg_get_trigger_ddl('"S"."T"','trg');
                                     pg_get_trigger_ddl

---------------------------------------------------------------------------------------------
 CREATE TRIGGER trg BEFORE INSERT ON "S"."T" FOR EACH STATEMENT EXECUTE
FUNCTION "S".trgf();
(1 row)

postgres=# SELECT pg_get_trigger_ddl('S.T','trg');
ERROR:  relation "s.t" does not exist
LINE 1: SELECT pg_get_trigger_ddl('S.T','trg');

The table parameter expects the double quotes, so it would be a hard
sell to make the trigger name parameter **not accept them** either.

In that light, I tend to think that the approach of pg_get_viewdef()
would be the best way to go, but let's see what the other reviewers have
to say about it.


Best, Jim

Hi Phil

On 10/16/25 21:04, Philip Alger wrote:
> I refactored the code in v5 attached and it should now be strict and use
> double quotes for those scenarios. Additionally, it takes care of the -1
> OID issue.

Nice!

v5 now parses the double quotes correctly:

postgres=# CREATE SCHEMA "S";
CREATE SCHEMA

postgres=# CREATE TABLE "S"."T" (c int);
CREATE TABLE

postgres=# CREATE FUNCTION "S"."TriggerFunc"()
RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
  RETURN NULL;
END; $$;
CREATE FUNCTION

postgres=# CREATE TRIGGER "MyTrigger" BEFORE INSERT ON "S"."T"
FOR EACH STATEMENT EXECUTE PROCEDURE "S"."TriggerFunc"();
CREATE TRIGGER

postgres=# SELECT pg_get_trigger_ddl('"S"."T"','"MyTrigger"');
                                              pg_get_trigger_ddl

--------------------------------------------------------------------------------------------------------------
 CREATE TRIGGER "MyTrigger" BEFORE INSERT ON "S"."T" FOR EACH STATEMENT
EXECUTE FUNCTION "S"."TriggerFunc"();
(1 row)

... making non-quoted object names case insensitive:

postgres=# CREATE TABLE t (c int);
CREATE TABLE

postgres=# CREATE FUNCTION trgfunc()
RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
  RETURN NULL;
END; $$;
CREATE FUNCTION

postgres=# CREATE TRIGGER mytrigger BEFORE INSERT ON t
FOR EACH STATEMENT EXECUTE PROCEDURE trgfunc();
CREATE TRIGGER

postgres=# SELECT pg_get_trigger_ddl('T','MYTRIGGER');
                                        pg_get_trigger_ddl

---------------------------------------------------------------------------------------------------
 CREATE TRIGGER mytrigger BEFORE INSERT ON public.t FOR EACH STATEMENT
EXECUTE FUNCTION trgfunc();
(1 row)


-1 and NULL for the table name now return NULL.

The tests were also updated accordingly.

I am now wondering if introducing these new set of parameters to
pg_get_triggerdef() would be a better solution that creating a new
function. Like pg_get_indexdef():

{ oid => '2507', descr => 'index description (full create statement or
single expression) with pretty-print option', proname =>
'pg_get_indexdef', provolatile => 's', prorettype => 'text', proargtypes
=> 'oid int4 bool', prosrc => 'pg_get_indexdef_ext' },

...

{ oid => '1643', descr => 'index description', proname =>
'pg_get_indexdef', provolatile => 's', prorettype => 'text',
proargtypes => 'oid', prosrc => 'pg_get_indexdef' },

Doing so we keep it consistent with the other pg_get*def functions. What
do you think?

Thanks!

Best, Jim

On 10/17/25 21:07, Philip Alger wrote:
> The rationale behind it is here: https://www.postgresql.org/message-
> id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net <https://
> www.postgresql.org/message-id/945db7c5-be75-45bf-b55b-
> cb1e56f2e3e9%40dunslane.net>

Ah, I wasn’t aware of this thread. That settles it then :)

Documentation also LGTM.

One last thing: should we perhaps check for NULL before calling
appendStringInfo here?

/* pg_get_triggerdef_worker retrieves the trigger definition */
res = pg_get_triggerdef_worker(trgOid, false);

appendStringInfo(&buf, "%s;", res);


Best, Jim

On 10/18/25 06:08, Philip Alger wrote:
> 
>  Yes, you're correct. I've added that in v6 attached.

Nice. The code now checks res for NULL, which aligns with other similar
functions, e.g. pg_get_indexdef.

if (res == NULL)
  PG_RETURN_NULL();

One nitpick:

You're probably initialising the buffer a bit too early:

...

/* Validate that the relation exists */
if (!OidIsValid(relid) || get_rel_name(relid) == NULL)
    PG_RETURN_NULL();

initStringInfo(&buf);

...

If the function is going to return NULL, there is no need to allocate
memory for buf. I guess you could place it right before the
appendStringInfo call:

initStringInfo(&buf);
appendStringInfo(&buf, "%s;", res);

Other than that, the patch LGTM. If the other reviewers have no
objections, I'll mark it as ready for committer.

Thanks for the patch.

Best, Jim

Philip Alger <paalger0@gmail.com> writes:
> I attached v7 but it looks like that one of the CI tests failed, but I
> reran though my Github branch and they all passed. The error had nothing to
> do with the patch though. Wondering if I need to resubmit a v8 to kick it
> off again? Thoughts?

Just ignore it if you're pretty sure the error is unrelated.  The
cfbot cycles through all open patches and will re-run CI for yours
in a day or three (not sure of the exact cycle time right now).
If the same error persists then you'd better look closer.

            regards, tom lane

On 21/10/2025 18:01, Tom Lane wrote:
> Philip Alger <paalger0@gmail.com> writes:
>> I attached v7 but it looks like that one of the CI tests failed, but I
>> reran though my Github branch and they all passed. The error had nothing to
>> do with the patch though. Wondering if I need to resubmit a v8 to kick it
>> off again? Thoughts?
> Just ignore it if you're pretty sure the error is unrelated.  The
> cfbot cycles through all open patches and will re-run CI for yours
> in a day or three (not sure of the exact cycle time right now).
> If the same error persists then you'd better look closer.

The error is most likely unrelated to your patch. Just wait for the
cfbot to run again, which will happen in a day or two.

Best, Jim

On Sun, Oct 19, 2025 at 5:35 AM Philip Alger <paalger0@gmail.com> wrote:
>
> Attached is v7. Moved the `initStringInfo` as suggested and reran tests.
>
hi.

https://www.postgresql.org/docs/current/sql-createtrigger.html
the parameter section:
>>>>
The name to give the new trigger. This must be distinct from the name of any
other trigger for the same table. The name cannot be schema-qualified — the
trigger inherits the schema of its table
>>>>

doc said trigger name can not be schema-qualified,
we can not do:
CREATE TRIGGER public.modified_a BEFORE UPDATE OF a ON main_table
FOR EACH ROW WHEN (OLD.a <> NEW.a) EXECUTE PROCEDURE trigger_func('modified_a');


+ text    *trgName = PG_GETARG_TEXT_PP(1);
+ Oid     trgOid;
+ List   *nameList;
+ char *schemaName;
+ char *objName;
+
+
+ /* Parse the trigger name to handle quoted identifiers */
+ nameList = textToQualifiedNameList(trgName);
+ DeconstructQualifiedName(nameList, &schemaName, &objName);

So the above ``textToQualifiedNameList(trgName);`` part is wrong?

Hi Phil, Hi Jian

On 23/10/2025 00:27, Philip Alger wrote:
> It might be advantageous to show a `trigger name cannot be schema
> qualified` error to the user. Therefore, I added the check and the tests
> on v8 attached.
> 
> postgres=# SELECT pg_get_trigger_ddl('main_table', 'public.modified_a');
> ERROR:  trigger name cannot be schema qualified

I'm not sure this is the way to go here. Why specifically check for a
schema qualified trigger if it cannot be created in the first place? The
current error message for "trigger not found" would IMHO suffice, e.g.

ERROR:  trigger "s.tr" for table "t" does not exist

Jian, is that what you had in mind?

Best, Jim

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’t
needto 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’),
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 function
addsa 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/

On Sun, Oct 26, 2025 at 2:48 AM Jim Jones <jim.jones@uni-muenster.de> wrote:
>
> On 23/10/2025 00:27, Philip Alger wrote:
> > It might be advantageous to show a `trigger name cannot be schema
> > qualified` error to the user. Therefore, I added the check and the tests
> > on v8 attached.
> >
> > postgres=# SELECT pg_get_trigger_ddl('main_table', 'public.modified_a');
> > ERROR:  trigger name cannot be schema qualified
>
> I'm not sure this is the way to go here. Why specifically check for a
> schema qualified trigger if it cannot be created in the first place? The
> current error message for "trigger not found" would IMHO suffice, e.g.
>
> ERROR:  trigger "s.tr" for table "t" does not exist
>
hi.

https://www.postgresql.org/docs/current/sql-createtrigger.html
says:
<<<
The name cannot be schema-qualified — the trigger inherits the schema of its
table.
<<<

SELECT pg_get_trigger_ddl('main_table', 'public.modified_a');
should error out, I think.
but V7 will not error out, instead it will ignore text "public" and print out
DDL for the trigger name as "modified_a" which is not what I expected, so I
raised the question.

The error message can be different, but it should error out.
I am fine with v8.
ERROR:  trigger name cannot be schema qualified

I’m fine with changing it to the other error message:
ERROR:  trigger name \"%s\" for table \"%s\" does not exist

Hi Jian

On 28/10/2025 07:06, jian he wrote:
> SELECT pg_get_trigger_ddl('main_table', 'public.modified_a');
> should error out, I think.
> but V7 will not error out, instead it will ignore text "public" and print out
> DDL for the trigger name as "modified_a" which is not what I expected, so I
> raised the question.
> 
> The error message can be different, but it should error out.

+1

> I am fine with v8.
> ERROR:  trigger name cannot be schema qualified
> 
> I’m fine with changing it to the other error message:
> ERROR:  trigger name \"%s\" for table \"%s\" does not exist

I also think that raising an ERROR is the right approach here. My point
was rather the *extra check* for a schema qualified trigger name.
Letting it fail with the existing error message "trigger name \"%s\" for
table \"%s\" does not exist" down the road should be enough.

Best, Jim

Hi Phil

On 28/10/2025 20:01, Philip Alger wrote:
>  I am not sure what you mean here. Are you suggesting keep the check in
> v7 with `trigger name \"%s\" for table \"%s\" does not exist"` and
> remove the extra check in v8?

No, having an error message here is correct. I was just thinking that
the error message could be the same one we get when providing a wrong
trigger name. For instance (from your tests):

ERROR:  trigger "no_such_trigger" for table "main_table" does not exist

This raises an error because no_such_trigger does not exist, so the same
error could be returned if the user specifies myschema.no_such_trigger
instead, making the extra check for a schema-qualified trigger name
unnecessary. I do realise this is rather a nitpick, and since the other
reviewers are fine with it, don't worry too much about it for now :)

Best, Jim

ne 2. 11. 2025 v 22:08 odesílatel Philip Alger <paalger0@gmail.com> napsal:
>
>
>>
>>>> doc said trigger name can not be schema-qualified,
>>>> we can not do:
>>>> CREATE TRIGGER public.modified_a BEFORE UPDATE OF a ON main_table
>>>> FOR EACH ROW WHEN (OLD.a <> NEW.a) EXECUTE PROCEDURE trigger_func('modified_a');
>>>
>>>
>>>>
>>>> + nameList = textToQualifiedNameList(trgName);
>>>>
>>
>> I am wondering if adding an error message if someone inserted a schema name would be advantageous?
>
>
> It might be advantageous to show a `trigger name cannot be schema qualified` error to the user. Therefore, I added
thecheck and the tests on v8 attached. 

Would it make sense to rename trigger related variables to "trig"
prefix instead of "trg" as is done in other functions in the same file
(for example in function pg_get_triggerdef)?

> postgres=# SELECT pg_get_trigger_ddl('main_table', 'public.modified_a');
> ERROR:  trigger name cannot be schema qualified
>
> Thanks.
>
> --
> Best,
> Phil Alger

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/
>
>
>
>
>
>
>
>

po 3. 11. 2025 v 2:18 odesílatel Philip Alger <paalger0@gmail.com> napsal:
>
> Hi Josef,
>
>> Would it make sense to rename trigger related variables to "trig"
>> prefix instead of "trg" as is done in other functions in the same file
>> (for example in function pg_get_triggerdef)?
>
>
> Not sure it would be the same as triggerdef uses trig, while triggerdef_worker uses a mix of trig and tg in the same
function.It would be another patch, I believe, to and correct a prefix, then you'd have to normalize all the other
functionswithin the same file. 

If you mind updating the current patch to not add a third version (or
spread the problem), I'm happy to provide a later separate patch to
unify terminology in that file for triggers.

> --
> Best,
> Phil Alger

po 3. 11. 2025 v 2:22 odesílatel Philip Alger <paalger0@gmail.com> napsal:
>
> Hello,
>
>
>>
>> > 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?
>
>
> The wrapper function string_to_text, which is a wrapper for cstring_to_text, includes pfree.
>
> see ruleutils.c
>
> static text *
> string_to_text(char *str)
> {
>      text   *result;
>
>      result = cstring_to_text(str);
>      pfree(str);
>      return result;
> }

Indeed, IMHO pfree is not needed and the current patch is good as is.

>
> --
> Best,
> Phil Alger