Re: [PATCH] Add pretty formatting to pg_get_triggerdef

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