Re: Improve pg_trigger.tgargs representation

[Tom Lane]

Peter Eisentraut  writes:
> I always found the encoding of the pg_trigger.tgargs field quite weird 
> and archaic.  (It encodes the trigger argument list into a bytea 
> column.)

It is that ...

> So I tried replacing this with a list of nodes in a 
> pg_node_tree.  This worked out quite nicely.  It makes the internal code 
> much simpler, and it would maybe also allow clients to decode the column 
> more easily.

> (Trigger arguments are all strings, so another representation might be 
> an array of text, but I think a node representation makes more sense. 
> For example, you could imagine encoding numeric constants more directly, 
> but this is not done here.)

I come down on the other side of that: if we change this we should
change to array-of-text.  I think expecting clients to be able to
decode pg_node_tree is an absolutely horrid idea: it's overly complex
and we change the representation constantly.

Also, while I see that indeed none of our client-side code looks
directly at pg_trigger.tgargs anymore, there is a lot of backend and
contrib code (and therefore likely also 3rd-party extension code) that
depends on the representation used in struct Trigger ... which is
array of C strings.  I do not think we dare change that.

On the whole I'm not sure this is worth messing with.  We run at least
some risk of breaking somebody's code, and it doesn't seem like we
gain much unless we're willing to change struct Trigger, which'd
break a *lot* of code.

regards, tom lane

Improve pg_trigger.tgargs representation

[Peter Eisentraut]

I always found the encoding of the pg_trigger.tgargs field quite weird 
and archaic.  (It encodes the trigger argument list into a bytea 
column.)  So I tried replacing this with a list of nodes in a 
pg_node_tree.  This worked out quite nicely.  It makes the internal code 
much simpler, and it would maybe also allow clients to decode the column 
more easily.


(Trigger arguments are all strings, so another representation might be 
an array of text, but I think a node representation makes more sense. 
For example, you could imagine encoding numeric constants more directly, 
but this is not done here.)


Note that clients shipped with PostgreSQL (such as psql and pg_dump) use
pg_get_triggerdef() and don't access the field directly, so they need no 
changes.  (See also [0].)  I don't know about other clients, such as 
third-party GUIs.


Now, I don't really know if this is worth actually moving forward with, 
unless perhaps someone has additional arguments in favor.  But I figured 
it was worth sharing as an idea.



[0]: 
https://www.postgresql.org/message-id/flat/56c8f5bf-de47-48c1-a592-588fb526e9e6%40eisentraut.orgFrom 134e562354a91630dd8dbc10c5b334cd05e028f5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sun, 4 Feb 2024 19:13:20 +0100
Subject: [PATCH] Improve pg_trigger.tgargs representation

Before, the trigger argument list was encoded into a bytea column.
Replace this with a list of nodes in a pg_node_tree.  This makes the
internal code much simpler and also allows clients to decode the
column more easily.

Clients shipped with PostgreSQL (such as psql and pg_dump) use
pg_get_triggerdef() and don't access the field directly, so they need
to no changes.

TODO: catversion
---
 doc/src/sgml/catalogs.sgml | 14 +---
 src/backend/catalog/information_schema.sql |  3 +-
 src/backend/commands/tablecmds.c   | 27 +++-
 src/backend/commands/trigger.c | 76 +-
 src/backend/utils/adt/ruleutils.c  | 31 +
 src/include/catalog/pg_trigger.h   |  3 +-
 6 files changed, 47 insertions(+), 107 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 880f717b103..9f319039353 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -8490,15 +8490,6 @@ pg_trigger 
Columns
   
  
 
- 
-  
-   tgnargs int2
-  
-  
-   Number of argument strings passed to trigger function
-  
- 
-
  
   
tgattr int2vector
@@ -8512,10 +8503,11 @@ pg_trigger 
Columns
 
  
   
-   tgargs bytea
+   tgargs pg_node_tree
   
   
-   Argument strings to pass to trigger, each NULL-terminated
+   List of argument strings to pass to trigger (in
+   nodeToString() representation)
   
  
 
diff --git a/src/backend/catalog/information_schema.sql 
b/src/backend/catalog/information_schema.sql
index c402ae72741..866015faedc 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -2138,8 +2138,7 @@ CREATE VIEW triggers AS
ELSE null END
  AS character_data) AS action_condition,
CAST(
- substring(pg_get_triggerdef(t.oid) from
-   position('EXECUTE FUNCTION' in 
substring(pg_get_triggerdef(t.oid) from 48)) + 47)
+ 'EXECUTE FUNCTION ' || t.tgfoid::regprocedure || '(' || 
pg_get_expr(t.tgargs, 0) || ')'
  AS character_data) AS action_statement,
CAST(
  -- hard-wired reference to TRIGGER_TYPE_ROW
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9f51696740e..19a84f76008 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -19423,25 +19423,16 @@ CloneRowTriggersToPartition(Relation parent, Relation 
partition)
}
}
 
-   /* Reconstruct trigger arguments list. */
-   if (trigForm->tgnargs > 0)
-   {
-   char   *p;
-
-   value = heap_getattr(tuple, Anum_pg_trigger_tgargs,
-
RelationGetDescr(pg_trigger), );
-   if (isnull)
-   elog(ERROR, "tgargs is null for trigger \"%s\" 
in partition \"%s\"",
-NameStr(trigForm->tgname), 
RelationGetRelationName(partition));
-
-   p = (char *) VARDATA_ANY(DatumGetByteaPP(value));
+   /*
+* Reconstruct trigger arguments list.
+*/
+   value = heap_getattr(tuple, Anum_pg_trigger_tgargs,
+
RelationGetDescr(pg_trigger), );
+   if (isnull)
+   elog(ERROR, "tgargs i