Thread
-
[PATCH] Add pretty formatting to pg_get_triggerdef
Philip Alger <paalger0@gmail.com> — 2025-11-04T03:36:58Z
Hello Hackers, Currently, `pg_get_triggerdef` includes a "pretty" flag, but it does not actually format the `CREATE TRIGGER` statement in a "pretty" way. Unlike `pg_get_viewdef`, `pg_get_ruledef`, and `pg_get_indexdef`, the purpose of pretty formatting has been to remove the schema name so it can be used by the `\d` psql comment to display triggers associated with a view or table, as shown below: postgres=# \d main_table Table "public.main_table" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- a | integer | | | b | integer | | | Indexes: "main_table_a_key" UNIQUE CONSTRAINT, btree (a) Triggers: foofoo AFTER INSERT ON main_table FOR EACH STATEMENT EXECUTE FUNCTION trigger_func('foo_bar') foo_bar BEFORE INSERT ON main_table FOR EACH STATEMENT EXECUTE FUNCTION trigger_func('foo_bar') bar_fooAFTER DELETE ON main_table FOR EACH ROW WHEN ((old.a = 123)) EXECUTE FUNCTION trigger_func('foo_bar') This patch introduces true pretty formatting to `pg_get_triggerdef`. Additionally, it creates a new function specifically for the `\d` psql command, as that command requires schema removal and a single line statement. With this patch, when the `pretty` parameter is set to `true`, `pg_get_triggerdef` now displays a formatted output, consistent with `pg_get_viewdef`, `pg_get_ruledef`, and `pg_get_indexdef`: postgres=# select pg_get_triggerdef(12345, true); pg_get_triggerdef -------------------------------------------------- CREATE TRIGGER some_trig_foobar AFTER UPDATE + ON some_t + FOR EACH ROW + WHEN (NOT new.some_col) + EXECUTE FUNCTION dummy_update_func('foobar') (1 row) When the `pretty` flag is `false`, the function's behavior remains unchanged from the original implementation: postgres=# select pg_get_triggerdef(47901, false); pg_get_triggerdef --------------------------------------------------------------------------------------------------------------------------------------------------- CREATE TRIGGER some_trig_foobar AFTER UPDATE ON public.some_t FOR EACH ROW WHEN ((NOT new.some_col)) EXECUTE FUNCTION dummy_update_func('foobar') (1 row) -- Best, Phil Alger -
Re: [PATCH] Add pretty formatting to pg_get_triggerdef
Steven Niu <niushiji@gmail.com> — 2025-11-04T07:19:57Z
Hi, Philip, I verified your patch on my Ubuntu 24.04 and it works as expected. The "make check" passed. Also, the code change looks good to me. Regards, Steven ________________________________ From: Philip Alger <paalger0@gmail.com> Sent: Tuesday, November 04, 2025 11:36 To: pgsql-hackers <pgsql-hackers@postgresql.org> Subject: [PATCH] Add pretty formatting to pg_get_triggerdef Hello Hackers, Currently, `pg_get_triggerdef` includes a "pretty" flag, but it does not actually format the `CREATE TRIGGER` statement in a "pretty" way. Unlike `pg_get_viewdef`, `pg_get_ruledef`, and `pg_get_indexdef`, the purpose of pretty formatting has been to remove the schema name so it can be used by the `\d` psql comment to display triggers associated with a view or table, as shown below: postgres=# \d main_table Table "public.main_table" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- a | integer | | | b | integer | | | Indexes: "main_table_a_key" UNIQUE CONSTRAINT, btree (a) Triggers: foofoo AFTER INSERT ON main_table FOR EACH STATEMENT EXECUTE FUNCTION trigger_func('foo_bar') foo_bar BEFORE INSERT ON main_table FOR EACH STATEMENT EXECUTE FUNCTION trigger_func('foo_bar') bar_fooAFTER DELETE ON main_table FOR EACH ROW WHEN ((old.a = 123)) EXECUTE FUNCTION trigger_func('foo_bar') This patch introduces true pretty formatting to `pg_get_triggerdef`. Additionally, it creates a new function specifically for the `\d` psql command, as that command requires schema removal and a single line statement. With this patch, when the `pretty` parameter is set to `true`, `pg_get_triggerdef` now displays a formatted output, consistent with `pg_get_viewdef`, `pg_get_ruledef`, and `pg_get_indexdef`: postgres=# select pg_get_triggerdef(12345, true); pg_get_triggerdef -------------------------------------------------- CREATE TRIGGER some_trig_foobar AFTER UPDATE + ON some_t + FOR EACH ROW + WHEN (NOT new.some_col) + EXECUTE FUNCTION dummy_update_func('foobar') (1 row) When the `pretty` flag is `false`, the function's behavior remains unchanged from the original implementation: postgres=# select pg_get_triggerdef(47901, false); pg_get_triggerdef --------------------------------------------------------------------------------------------------------------------------------------------------- CREATE TRIGGER some_trig_foobar AFTER UPDATE ON public.some_t FOR EACH ROW WHEN ((NOT new.some_col)) EXECUTE FUNCTION dummy_update_func('foobar') (1 row) -- Best, Phil Alger -
Re: [PATCH] Add pretty formatting to pg_get_triggerdef
Chao Li <li.evan.chao@gmail.com> — 2025-11-04T09:40:19Z
> On Nov 4, 2025, at 11:36, Philip Alger <paalger0@gmail.com> wrote: > > -- > Best, > Phil Alger > <v1-0001-Add-pretty-formatting-to-pg_get_triggerdef.patch> 1 ``` +/* + * pg_get_triggerdef_compact + * Returns trigger definition in a compact, single-line format without + * schema qualification designed for the psql \d command. + */ +Datum +pg_get_triggerdef_compact(PG_FUNCTION_ARGS) +{ + Oid trigid = PG_GETARG_OID(0); + char *res; + + res = pg_get_triggerdef_worker(trigid, PRETTYFLAG_SCHEMA); ``` I think this is a mis-use of PRETTYFLAG_SCHEMA that is for printing schema-qualified names but omitting schema. 2 ``` + if (prettyFlags & PRETTYFLAG_INDENT) + appendStringInfoString(&buf, "\n "); ``` We should not hardcode 4 white-spaces, instead, we should use the const PRETTYINDENT_STD. Did you ever consider using appendContextKeyword()? Checking for an existing usage: ``` static void get_rule_windowclause(Query *query, deparse_context *context) { StringInfo buf = context->buf; const char *sep; ListCell *l; sep = NULL; foreach(l, query->windowClause) { WindowClause *wc = (WindowClause *) lfirst(l); if (wc->name == NULL) continue; /* ignore anonymous windows */ if (sep == NULL) appendContextKeyword(context, " WINDOW ", -PRETTYINDENT_STD, PRETTYINDENT_STD, 1); else appendStringInfoString(buf, sep); appendStringInfo(buf, "%s AS ", quote_identifier(wc->name)); get_rule_windowspec(wc, query->targetList, context); sep = ", "; } } ``` 3 Looks like you forgot to update pg_get_triggerdef(), when it calls pg_get_triggerdef_worker(tirgid, false), the second parameter “false” should be updated to an int flag. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: [PATCH] Add pretty formatting to pg_get_triggerdef
Philip Alger <paalger0@gmail.com> — 2025-11-04T20:56:41Z
Hi Chao, +/* > + * pg_get_triggerdef_compact > + * Returns trigger definition in a compact, single-line > format without > + * schema qualification designed for the psql \d command. > + */ > +Datum > +pg_get_triggerdef_compact(PG_FUNCTION_ARGS) > +{ > + Oid trigid = PG_GETARG_OID(0); > + char *res; > + > + res = pg_get_triggerdef_worker(trigid, PRETTYFLAG_SCHEMA); > ``` > > I think this is a mis-use of PRETTYFLAG_SCHEMA that is for printing > schema-qualified names but omitting schema. > Yes, this is to omit the schema because the functions is used to print out the triggers when using \d in psql, The current practice isn't to print out a schema for the table/view/etc. 2 > ``` > + if (prettyFlags & PRETTYFLAG_INDENT) > + appendStringInfoString(&buf, "\n "); > ``` > > We should not hardcode 4 white-spaces, instead, we should use the const > PRETTYINDENT_STD. Did you ever consider using appendContextKeyword()? > Checking for an existing usage: > ``` > > Thanks for pointing this out. I refactored the code using appendContextKeyword() and added a few tests as well in v2 (attached). > 3 Looks like you forgot to update pg_get_triggerdef(), when it calls > pg_get_triggerdef_worker(tirgid, false), the second parameter “false” > should be updated to an int flag. > I fixed this to set to 0. However, false is defined as 0 in stdbool.h #define false 0 -- Best, Phil Alger EDB: https://www.enterprisedb.com -
Re: [PATCH] Add pretty formatting to pg_get_triggerdef
Chao Li <li.evan.chao@gmail.com> — 2025-11-05T09:28:37Z
> On Nov 5, 2025, at 04:56, Philip Alger <paalger0@gmail.com> wrote: > > Hi Chao, > > +/* > + * pg_get_triggerdef_compact > + * Returns trigger definition in a compact, single-line format without > + * schema qualification designed for the psql \d command. > + */ > +Datum > +pg_get_triggerdef_compact(PG_FUNCTION_ARGS) > +{ > + Oid trigid = PG_GETARG_OID(0); > + char *res; > + > + res = pg_get_triggerdef_worker(trigid, PRETTYFLAG_SCHEMA); > ``` > > I think this is a mis-use of PRETTYFLAG_SCHEMA that is for printing schema-qualified names but omitting schema. > > Yes, this is to omit the schema because the functions is used to print out the triggers when using \d in psql, The current practice isn't to print out a schema for the table/view/etc. > I guess I didn’t express myself clearly. My comment was that, this function wants to omit schema, so PRETTYFLAG_SCHEMA should not be used here, because it is for adding schema. > > > 3 Looks like you forgot to update pg_get_triggerdef(), when it calls pg_get_triggerdef_worker(tirgid, false), the second parameter “false” should be updated to an int flag. > > I fixed this to set to 0. However, false is defined as 0 in stdbool.h > #define false 0 > I know false is actually 0, but passing false to an int argument is misleading. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: [PATCH] Add pretty formatting to pg_get_triggerdef
Philip Alger <paalger0@gmail.com> — 2025-11-05T16:21:48Z
Hi Chao, > > I think this is a mis-use of PRETTYFLAG_SCHEMA that is for printing > schema-qualified names but omitting schema. > > > > Yes, this is to omit the schema because the functions is used to print > out the triggers when using \d in psql, The current practice isn't to print > out a schema for the table/view/etc. > > > > I guess I didn’t express myself clearly. My comment was that, this > function wants to omit schema, so PRETTYFLAG_SCHEMA should not be used > here, because it is for adding schema. > > I am not sure that's right because I am using it in a similar way as what's written in pg_get_index_worker: appendStringInfo(&buf, "CREATE %sINDEX %s ON %s%s USING %s (", idxrec->indisunique ? "UNIQUE " : "", quote_identifier(NameStr(idxrelrec->relname)), idxrelrec->relkind == RELKIND_PARTITIONED_INDEX && !inherits ? "ONLY " : "", (prettyFlags & PRETTYFLAG_SCHEMA) ? generate_relation_name(indrelid, NIL) : generate_qualified_relation_name(indrelid), quote_identifier(NameStr(amrec->amname))); For that function, with `true` set you don't get the schema: postgres=# select pg_get_indexdef(16395, 0, true); pg_get_indexdef -------------------------------------------------------------------- CREATE UNIQUE INDEX main_table_a_key ON main_table USING btree (a) (1 row) Similarly, in this patch, you get: postgres=# \d child3 Table "public.child3" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- b | integer | | | a | text | | | Partition of: parent FOR VALUES IN ('CCC') Triggers: child3_delete_trig AFTER DELETE ON child3 REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION dump_delete() Or this without the schema as well: postgres=# SELECT pg_get_triggerdef(oid, true) FROM pg_trigger WHERE tgrelid = 'child3'::regclass AND tgname = 'child3_insert_trig'; pg_get_triggerdef ------------------------------------------------ CREATE TRIGGER child3_insert_trig AFTER INSERT+ ON child3 + REFERENCING NEW TABLE AS new_table + FOR EACH STATEMENT + EXECUTE FUNCTION dump_insert() (1 row) Setting prettyFlags = PRETTYFLAG_SCHEMA removes the schema. -- Best, Phil Alger EDB: https://www.enterprisedb.com -
Re: [PATCH] Add pretty formatting to pg_get_triggerdef
Philip Alger <paalger0@gmail.com> — 2025-11-05T17:03:10Z
I am attaching v3. I changed the name of the function that prints out the trigger using the \d command (below) from `pg_get_triggerdef_compact` to `pg_get_triggerdef_string`, which matches the index naming convention for a similar function. postgres=# \d child3 > Table "public.child3" > Column | Type | Collation | Nullable | Default > --------+---------+-----------+----------+--------- > b | integer | | | > a | text | | | > Partition of: parent FOR VALUES IN ('CCC') > Triggers: > child3_delete_trig AFTER DELETE ON child3 REFERENCING OLD TABLE AS > old_table FOR EACH STATEMENT EXECUTE FUNCTION dump_delete() > > I've cleaned up the code by removing deparse_context variables related to appendContextKeyword that were set as NULL or NIL. All tests passed. -- Best, Phil Alger EDB: https://www.enterprisedb.com -
Re: [PATCH] Add pretty formatting to pg_get_triggerdef
Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-05T19:29:29Z
Philip Alger <paalger0@gmail.com> writes: > I am attaching v3. This patch will cause psql's \d to fail hard against any pre-v19 server. That's not acceptable. Note the comment at the top of describe.c: * Support for the various \d ("describe") commands. Note that the current * expectation is that all functions in this file will succeed when working * with servers of versions 9.2 and up. It's okay to omit irrelevant * information for an old server, but not to fail outright. (But failing * against a pre-9.2 server is allowed.) You could get around that with a conditional on the server version, as is done elsewhere in describe.c. But I kind of wonder why we need this new function at all. Isn't "pg_get_triggerdef(oid, false)" close enough? An alternative suggestion is to keep "pg_get_triggerdef(oid, true)" doing exactly what it does today and have the newly-introduced function be the one that has new behavior. This might be a smarter plan anyway to avoid side-swiping other clients that are expecting the current behavior. regards, tom lane -
Re: [PATCH] Add pretty formatting to pg_get_triggerdef
Philip Alger <paalger0@gmail.com> — 2025-11-06T20:19:29Z
Hi Tom, > This patch will cause psql's \d to fail hard against any pre-v19 > server. That's not acceptable. Note the comment at the top of > describe.c: > > * Support for the various \d ("describe") commands. Note that the current > * expectation is that all functions in this file will succeed when working > * with servers of versions 9.2 and up. It's okay to omit irrelevant > * information for an old server, but not to fail outright. (But failing > * against a pre-9.2 server is allowed.) > > I didn't see that at the top of the file, and didn't think the new function would break it all. > You could get around that with a conditional on the server version, > as is done elsewhere in describe.c. But I kind of wonder why we > need this new function at all. Isn't "pg_get_triggerdef(oid, false)" > close enough? No. That works to print out the CREATE TRIGGER statement with the schema.relation name. I am trying to make `pretty` print similar to what is in pg_get_viewdef and pg_get_functiondef which formats the output. > An alternative suggestion is to keep "pg_get_triggerdef(oid, true)" > doing exactly what it does today and have the newly-introduced > function be the one that has new behavior. This might be a smarter > plan anyway to avoid side-swiping other clients that are expecting > the current behavior. > Thinking aloud, I would need to create a new function like pg_get_triggerdef_worker and let it handle the formatting. -- Best, Phil Alger EDB: https://www.enterprisedb.com