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 <pe...@eisentraut.org>
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 @@ <title><structname>pg_trigger</structname>
Columns</title>
</para></entry>
</row>
- <row>
- <entry role="catalog_table_entry"><para role="column_definition">
- <structfield>tgnargs</structfield> <type>int2</type>
- </para>
- <para>
- Number of argument strings passed to trigger function
- </para></entry>
- </row>
-
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>tgattr</structfield> <type>int2vector</type>
@@ -8512,10 +8503,11 @@ <title><structname>pg_trigger</structname>
Columns</title>
<row>
<entry role="catalog_table_entry"><para role="column_definition">
- <structfield>tgargs</structfield> <type>bytea</type>
+ <structfield>tgargs</structfield> <type>pg_node_tree</type>
</para>
<para>
- Argument strings to pass to trigger, each NULL-terminated
+ List of argument strings to pass to trigger (in
+ <function>nodeToString()</function> representation)
</para></entry>
</row>
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), &isnull);
- 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), &isnull);
+ if (isnull)
+ elog(ERROR, "tgargs is null for trigger \"%s\" in
partition \"%s\"",
+ NameStr(trigForm->tgname),
RelationGetRelationName(partition));
- for (int i = 0; i < trigForm->tgnargs; i++)
- {
- trigargs = lappend(trigargs,
makeString(pstrdup(p)));
- p += strlen(p) + 1;
- }
- }
+ trigargs = stringToNode(TextDatumGetCString(value));
trigStmt = makeNode(CreateTrigStmt);
trigStmt->replace = false;
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index c344ff09442..f7f6650d881 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -881,50 +881,7 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char
*queryString,
values[Anum_pg_trigger_tgconstraint - 1] =
ObjectIdGetDatum(constraintOid);
values[Anum_pg_trigger_tgdeferrable - 1] =
BoolGetDatum(stmt->deferrable);
values[Anum_pg_trigger_tginitdeferred - 1] =
BoolGetDatum(stmt->initdeferred);
-
- if (stmt->args)
- {
- ListCell *le;
- char *args;
- int16 nargs = list_length(stmt->args);
- int len = 0;
-
- foreach(le, stmt->args)
- {
- char *ar = strVal(lfirst(le));
-
- len += strlen(ar) + 4;
- for (; *ar; ar++)
- {
- if (*ar == '\\')
- len++;
- }
- }
- args = (char *) palloc(len + 1);
- args[0] = '\0';
- foreach(le, stmt->args)
- {
- char *s = strVal(lfirst(le));
- char *d = args + strlen(args);
-
- while (*s)
- {
- if (*s == '\\')
- *d++ = '\\';
- *d++ = *s++;
- }
- strcpy(d, "\\000");
- }
- values[Anum_pg_trigger_tgnargs - 1] = Int16GetDatum(nargs);
- values[Anum_pg_trigger_tgargs - 1] =
DirectFunctionCall1(byteain,
-
CStringGetDatum(args));
- }
- else
- {
- values[Anum_pg_trigger_tgnargs - 1] = Int16GetDatum(0);
- values[Anum_pg_trigger_tgargs - 1] =
DirectFunctionCall1(byteain,
-
CStringGetDatum(""));
- }
+ values[Anum_pg_trigger_tgargs - 1] =
CStringGetTextDatum(nodeToString(stmt->args));
/* build column number array if it's a column-specific trigger */
ncolumns = list_length(stmt->columns);
@@ -1902,6 +1859,7 @@ RelationBuildTriggers(Relation relation)
Trigger *build;
Datum datum;
bool isnull;
+ List *argslist;
if (numtrigs >= maxtrigs)
{
@@ -1923,7 +1881,6 @@ RelationBuildTriggers(Relation relation)
build->tgconstraint = pg_trigger->tgconstraint;
build->tgdeferrable = pg_trigger->tgdeferrable;
build->tginitdeferred = pg_trigger->tginitdeferred;
- build->tgnargs = pg_trigger->tgnargs;
/* tgattr is first var-width field, so OK to access directly */
build->tgnattr = pg_trigger->tgattr.dim1;
if (build->tgnattr > 0)
@@ -1934,24 +1891,23 @@ RelationBuildTriggers(Relation relation)
}
else
build->tgattr = NULL;
+
+ datum = fastgetattr(htup, Anum_pg_trigger_tgargs,
+ tgrel->rd_att, &isnull);
+ if (isnull)
+ elog(ERROR, "tgargs is null in trigger for relation
\"%s\"",
+ RelationGetRelationName(relation));
+
+ argslist = castNode(List,
stringToNode(TextDatumGetCString(datum)));
+ build->tgnargs = list_length(argslist);
if (build->tgnargs > 0)
{
- bytea *val;
- char *p;
-
- val = DatumGetByteaPP(fastgetattr(htup,
-
Anum_pg_trigger_tgargs,
-
tgrel->rd_att, &isnull));
- if (isnull)
- elog(ERROR, "tgargs is null in trigger for
relation \"%s\"",
- RelationGetRelationName(relation));
- p = (char *) VARDATA_ANY(val);
+ ListCell *lc;
+
build->tgargs = (char **) palloc(build->tgnargs *
sizeof(char *));
- for (i = 0; i < build->tgnargs; i++)
- {
- build->tgargs[i] = pstrdup(p);
- p += strlen(p) + 1;
- }
+ i = 0;
+ foreach(lc, argslist)
+ build->tgargs[i++] =
pstrdup(strVal(lfirst(lc)));
}
else
build->tgargs = NULL;
diff --git a/src/backend/utils/adt/ruleutils.c
b/src/backend/utils/adt/ruleutils.c
index b625f471a84..b97604e98ad 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1100,26 +1100,25 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
NIL, NULL,
false, NULL, EXPR_KIND_NONE));
- if (trigrec->tgnargs > 0)
{
- char *p;
- int i;
+ deparse_context context;
value = fastgetattr(ht_trig, Anum_pg_trigger_tgargs,
tgrel->rd_att, &isnull);
if (isnull)
elog(ERROR, "tgargs is null for trigger %u", trigid);
- p = (char *) VARDATA_ANY(DatumGetByteaPP(value));
- for (i = 0; i < trigrec->tgnargs; i++)
- {
- if (i > 0)
- appendStringInfoString(&buf, ", ");
- simple_quote_literal(&buf, p);
- /* advance p to next string embedded in tgargs */
- while (*p)
- p++;
- p++;
- }
+
+ context.buf = &buf;
+ context.namespaces = NIL;
+ context.windowClause = NIL;
+ context.windowTList = NIL;
+ context.varprefix = true;
+ context.prettyFlags = pretty ? (PRETTYFLAG_PAREN |
PRETTYFLAG_INDENT | PRETTYFLAG_SCHEMA) : PRETTYFLAG_INDENT;
+ context.wrapColumn = WRAP_COLUMN_DEFAULT;
+ context.indentLevel = PRETTYINDENT_STD;
+ context.special_exprkind = EXPR_KIND_NONE;
+
+ get_rule_expr(stringToNode(TextDatumGetCString(value)),
&context, false);
}
/* We deliberately do not put semi-colon at end */
@@ -9811,6 +9810,10 @@ get_rule_expr(Node *node, deparse_context *context,
}
break;
+ case T_String:
+ simple_quote_literal(buf, strVal(node));
+ break;
+
case T_TableFunc:
get_tablefunc((TableFunc *) node, context,
showimplicit);
break;
diff --git a/src/include/catalog/pg_trigger.h b/src/include/catalog/pg_trigger.h
index 7fdff161184..5bbf7294770 100644
--- a/src/include/catalog/pg_trigger.h
+++ b/src/include/catalog/pg_trigger.h
@@ -55,7 +55,6 @@ CATALOG(pg_trigger,2620,TriggerRelationId)
* if any */
bool tgdeferrable; /* constraint trigger is deferrable */
bool tginitdeferred; /* constraint trigger is deferred
initially */
- int16 tgnargs; /* # of extra arguments in
tgargs */
/*
* Variable-length fields start here, but we allow direct access to
@@ -65,7 +64,7 @@ CATALOG(pg_trigger,2620,TriggerRelationId)
* on columns */
#ifdef CATALOG_VARLEN
- bytea tgargs BKI_FORCE_NOT_NULL; /*
first\000second\000tgnargs\000 */
+ pg_node_tree tgargs BKI_FORCE_NOT_NULL; /* list of arguments */
pg_node_tree tgqual; /* WHEN expression, or NULL if none */
NameData tgoldtable; /* old transition table, or
NULL if none */
NameData tgnewtable; /* new transition table, or
NULL if none */
base-commit: 774bcffe4a9853a24e61d758637c0aad2871f1fb
--
2.43.0
