Re: [PATCH] pg_get_domain_ddl: DDL reconstruction function for CREATE DOMAIN statement

Hi,

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> They are adding a large amount of new code that will have to be
> kept in sync with behavior elsewhere, and there is basically zero
> forcing function to ensure that that happens.

Agree. For the sake of completeness I did a thorough pass over the
rest of v7 and found a few more issues. Documenting them here so
they're on the record regardless of where the broader discussion
about the approach lands.

1) get_formatted_string silently drops large formatted strings

+ va_start(args, fmt);
+ appendStringInfoVA(buf, fmt, args);
+ va_end(args);

appendStringInfoVA returns non-zero when the buffer is too small,
requiring enlargeStringInfo + retry (see appendStringInfo in
stringinfo.c). The return value is ignored here, so large
formatted text is silently lost.

Reproduction -- a domain with a ~2647-char CHECK expression:

DO $$
DECLARE long_check text;
BEGIN
long_check := 'CHECK (';
FOR i IN 1..50 LOOP
IF i > 1 THEN long_check := long_check || ' OR '; END IF;
long_check := long_check || format(
'VALUE ~ ''^pattern_%s_[a-zA-Z0-9]{10,20}$''', i);
END LOOP;
long_check := long_check || ')';
EXECUTE format(
'CREATE DOMAIN huge_domain AS text CONSTRAINT big_check %s',
long_check);
END $$;

select pg_get_domain_ddl('huge_domain');
CREATE DOMAIN public.huge_domain AS pg_catalog.text CONSTRAINT big_check ;
(1 row)

The entire CHECK clause (~2647 chars) is silently dropped.

This function was adopted from the pg_get_policy_ddl patch [1].
I checked v8 there and confirmed the same bug exists.

2) Function is VOLATILE PARALLEL UNSAFE

pg_proc.dat is missing provolatile => 's', and system_functions.sql
does not specify STABLE PARALLEL SAFE. Every other pg_get_*def
function is STABLE PARALLEL SAFE:

select proname, provolatile, proparallel from pg_proc
where proname in ('pg_get_domain_ddl','pg_get_constraintdef',
'pg_get_functiondef','pg_get_triggerdef');

pg_get_constraintdef | s | s
pg_get_domain_ddl | v | u <--
pg_get_functiondef | s | s
pg_get_triggerdef | s | s

Same issue in pg_get_policy_ddl v8 [1].

3) Internal type names exposed (related to typmod bug)

generate_qualified_type_name also uses the raw pg_type.typname,
so beyond losing modifiers:

int[] -> pg_catalog._int4 (should be integer[])
char(5) -> pg_catalog.bpchar (should be character(5))
timestamp(6) -> pg_catalog."timestamp" (should be timestamp(6) without time zone)

The format_type_extended fix from my earlier message resolves
this too. Several test expectations in object_ddl.out will need
updating once fixed.

Regards,
Haritabh

[1] https://www.postgresql.org/message-id/flat/CANxoLDdJsRJqnjMXV3yjsk07Z5iRWxG-c2hZJC7bAKqf8ZXj_A%40mail.gmail.com