From: Andrew Dunstan Date: Fri, 26 Jun 2026 12:00:39 +0000 (-0400) Subject: Use named boolean parameters for pg_get_*_ddl option arguments X-Git-Tag: REL_19_BETA2~106 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d6ed87d19890b1cfa93d1f6e8957fa525834c0e2;p=thirdparty%2Fpostgresql.git Use named boolean parameters for pg_get_*_ddl option arguments Replace the VARIADIC text[] alternating key/value option interface with typed named boolean parameters for pg_get_role_ddl(), pg_get_tablespace_ddl(), and pg_get_database_ddl(), as added by commit 4881981f920 and friends. The new signatures are: pg_get_role_ddl(role regrole, pretty boolean DEFAULT false, memberships boolean DEFAULT true) pg_get_tablespace_ddl(tablespace oid/name, pretty boolean DEFAULT false, owner boolean DEFAULT true) pg_get_database_ddl(db regdatabase, pretty boolean DEFAULT false, owner boolean DEFAULT true, tablespace boolean DEFAULT true) This provides type safety at the SQL level, allows named-argument calling syntax (pretty => true), removes the runtime string-parsing machinery (DdlOption, parse_ddl_options) in favour of direct PG_GETARG_BOOL() calls, and allows the functions to be marked STRICT. While we're here, I added an extra TAP test for pg_get_database(owner => false, ...) Catalog version bumped. Author: Jelte Fennema-Nio Discussion: https://postgr.es/m/DHM6C7SLS4BN.1WW9Z4PRPN0VJ@jeltef.nl (and on Discord) --- diff --git a/doc/src/sgml/func/func-info.sgml b/doc/src/sgml/func/func-info.sgml index 211bc8b238b..bc80bcbd0b3 100644 --- a/doc/src/sgml/func/func-info.sgml +++ b/doc/src/sgml/func/func-info.sgml @@ -3856,9 +3856,7 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres} reconstruct DDL statements for various global database objects. Each function returns a set of text rows, one SQL statement per row. (This is a decompiled reconstruction, not the original text of the - command.) Functions that accept VARIADIC options - take alternating name/value text pairs; values are parsed as boolean, - integer or text. + command.) @@ -3883,8 +3881,10 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres} pg_get_role_ddl ( roleregrole - , VARIADIC options - text[] ) + , pretty boolean + DEFAULT false + , memberships boolean + DEFAULT true ) setof text @@ -3892,10 +3892,10 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres} ALTER ROLE ... SET statements for the given role. Each statement is returned as a separate row. Password information is never included in the output. - The following options are supported: pretty (boolean) - for pretty-printed output and memberships (boolean, - default true) to include GRANT statements for - role memberships and their options. + When pretty is true, the output is + pretty-printed. When memberships is false, + GRANT statements for role memberships are + omitted. @@ -3905,15 +3905,19 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres} pg_get_tablespace_ddl ( tablespace oid - , VARIADIC options - text[] ) + , pretty boolean + DEFAULT false + , owner boolean + DEFAULT true ) setof text pg_get_tablespace_ddl ( tablespace name - , VARIADIC options - text[] ) + , pretty boolean + DEFAULT false + , owner boolean + DEFAULT true ) setof text @@ -3921,9 +3925,9 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres} the specified tablespace (by OID or name). If the tablespace has options set, an ALTER TABLESPACE ... SET statement is also returned. Each statement is returned as a separate row. - The following options are supported: pretty (boolean) - for formatted output and owner (boolean) to include - OWNER. + When pretty is true, the output is + pretty-printed. When owner is false, the + OWNER clause is omitted. @@ -3933,8 +3937,12 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres} pg_get_database_ddl ( database regdatabase - , VARIADIC options - text[] ) + , pretty boolean + DEFAULT false + , owner boolean + DEFAULT true + , tablespace boolean + DEFAULT true ) setof text @@ -3942,11 +3950,11 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres} specified database, followed by ALTER DATABASE statements for connection limit, template status, and configuration settings. Each statement is returned as a separate row. - The following options are supported: - pretty (boolean) for formatted output, - owner (boolean) to include OWNER, - and tablespace (boolean) to include - TABLESPACE. + When pretty is true, the output is + pretty-printed. When owner is false, the + OWNER clause is omitted. When + tablespace is false, the + TABLESPACE clause is omitted. diff --git a/src/backend/utils/adt/ddlutils.c b/src/backend/utils/adt/ddlutils.c index f32fcd453ef..a70f1c28655 100644 --- a/src/backend/utils/adt/ddlutils.c +++ b/src/backend/utils/adt/ddlutils.c @@ -33,7 +33,6 @@ #include "mb/pg_wchar.h" #include "miscadmin.h" #include "utils/acl.h" -#include "utils/array.h" #include "utils/builtins.h" #include "utils/datetime.h" #include "utils/fmgroids.h" @@ -46,35 +45,6 @@ #include "utils/timestamp.h" #include "utils/varlena.h" -/* Option value types for DDL option parsing */ -typedef enum -{ - DDL_OPT_BOOL, - DDL_OPT_TEXT, - DDL_OPT_INT, -} DdlOptType; - -/* - * A single DDL option descriptor: caller fills in name and type, - * parse_ddl_options fills in isset + the appropriate value field. - */ -typedef struct DdlOption -{ - const char *name; /* option name (case-insensitive match) */ - DdlOptType type; /* expected value type */ - bool isset; /* true if caller supplied this option */ - /* fields for specific option types */ - union - { - bool boolval; /* filled in for DDL_OPT_BOOL */ - char *textval; /* filled in for DDL_OPT_TEXT (palloc'd) */ - int intval; /* filled in for DDL_OPT_INT */ - }; -} DdlOption; - - -static void parse_ddl_options(FunctionCallInfo fcinfo, int variadic_start, - DdlOption *opts, int nopts); static void append_ddl_option(StringInfo buf, bool pretty, int indent, const char *fmt, ...) pg_attribute_printf(4, 5); @@ -83,150 +53,11 @@ static void append_guc_value(StringInfo buf, const char *name, static List *pg_get_role_ddl_internal(Oid roleid, bool pretty, bool memberships); static List *pg_get_tablespace_ddl_internal(Oid tsid, bool pretty, bool no_owner); -static Datum pg_get_tablespace_ddl_srf(FunctionCallInfo fcinfo, Oid tsid, bool isnull); +static Datum pg_get_tablespace_ddl_srf(FunctionCallInfo fcinfo, Oid tsid); static List *pg_get_database_ddl_internal(Oid dbid, bool pretty, bool no_owner, bool no_tablespace); -/* - * parse_ddl_options - * Parse variadic name/value option pairs - * - * Options are passed as alternating key/value text pairs. The caller - * provides an array of DdlOption descriptors specifying the accepted - * option names and their types; this function matches each supplied - * pair against the array, validates the value, and fills in the - * result fields. - */ -static void -parse_ddl_options(FunctionCallInfo fcinfo, int variadic_start, - DdlOption *opts, int nopts) -{ - Datum *args; - bool *nulls; - Oid *types; - int nargs; - - /* Clear all output fields */ - for (int i = 0; i < nopts; i++) - { - opts[i].isset = false; - switch (opts[i].type) - { - case DDL_OPT_BOOL: - opts[i].boolval = false; - break; - case DDL_OPT_TEXT: - opts[i].textval = NULL; - break; - case DDL_OPT_INT: - opts[i].intval = 0; - break; - } - } - - nargs = extract_variadic_args(fcinfo, variadic_start, true, - &args, &types, &nulls); - - if (nargs <= 0) - return; - - /* Handle DEFAULT NULL case */ - if (nargs == 1 && nulls[0]) - return; - - if (nargs % 2 != 0) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("variadic arguments must be name/value pairs"), - errhint("Provide an even number of variadic arguments that can be divided into pairs."))); - - /* - * For each option name/value pair, find corresponding positional option - * for the option name, and assign the option value. - */ - for (int i = 0; i < nargs; i += 2) - { - char *name; - char *valstr; - DdlOption *opt = NULL; - - if (nulls[i]) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("option name at variadic position %d is null", i + 1))); - - name = TextDatumGetCString(args[i]); - - if (nulls[i + 1]) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("value for option \"%s\" must not be null", name))); - - /* Find matching option descriptor */ - for (int j = 0; j < nopts; j++) - { - if (pg_strcasecmp(name, opts[j].name) == 0) - { - opt = &opts[j]; - break; - } - } - - if (opt == NULL) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("unrecognized option: \"%s\"", name))); - - if (opt->isset) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("option \"%s\" is specified more than once", - name))); - - valstr = TextDatumGetCString(args[i + 1]); - - switch (opt->type) - { - case DDL_OPT_BOOL: - if (!parse_bool(valstr, &opt->boolval)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("invalid value for boolean option \"%s\": %s", - name, valstr))); - break; - - case DDL_OPT_TEXT: - opt->textval = valstr; - valstr = NULL; /* don't pfree below */ - break; - - case DDL_OPT_INT: - { - char *endp; - long val; - - errno = 0; - val = strtol(valstr, &endp, 10); - if (*endp != '\0' || errno == ERANGE || - val < PG_INT32_MIN || val > PG_INT32_MAX) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("invalid value for integer option \"%s\": %s", - name, valstr))); - opt->intval = (int) val; - } - break; - } - - opt->isset = true; - - if (valstr) - pfree(valstr); - pfree(name); - } -} - /* * Helper to append a formatted string with optional pretty-printing. */ @@ -590,7 +421,6 @@ pg_get_role_ddl_internal(Oid roleid, bool pretty, bool memberships) * Each row is a complete SQL statement. The first row is always the * CREATE ROLE statement; subsequent rows are ALTER ROLE SET statements * and optionally GRANT statements for role memberships. - * Returns no rows if the role argument is NULL. */ Datum pg_get_role_ddl(PG_FUNCTION_ARGS) @@ -602,26 +432,17 @@ pg_get_role_ddl(PG_FUNCTION_ARGS) { MemoryContext oldcontext; Oid roleid; - DdlOption opts[] = { - {"pretty", DDL_OPT_BOOL}, - {"memberships", DDL_OPT_BOOL}, - }; + bool pretty; + bool memberships; funcctx = SRF_FIRSTCALL_INIT(); oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); - if (PG_ARGISNULL(0)) - { - MemoryContextSwitchTo(oldcontext); - SRF_RETURN_DONE(funcctx); - } - roleid = PG_GETARG_OID(0); - parse_ddl_options(fcinfo, 1, opts, lengthof(opts)); + pretty = PG_GETARG_BOOL(1); + memberships = PG_GETARG_BOOL(2); - statements = pg_get_role_ddl_internal(roleid, - opts[0].isset && opts[0].boolval, - !opts[1].isset || opts[1].boolval); + statements = pg_get_role_ddl_internal(roleid, pretty, memberships); funcctx->user_fctx = statements; funcctx->max_calls = list_length(statements); @@ -755,7 +576,7 @@ pg_get_tablespace_ddl_internal(Oid tsid, bool pretty, bool no_owner) * pg_get_tablespace_ddl_srf - common SRF logic for tablespace DDL */ static Datum -pg_get_tablespace_ddl_srf(FunctionCallInfo fcinfo, Oid tsid, bool isnull) +pg_get_tablespace_ddl_srf(FunctionCallInfo fcinfo, Oid tsid) { FuncCallContext *funcctx; List *statements; @@ -763,25 +584,16 @@ pg_get_tablespace_ddl_srf(FunctionCallInfo fcinfo, Oid tsid, bool isnull) if (SRF_IS_FIRSTCALL()) { MemoryContext oldcontext; - DdlOption opts[] = { - {"pretty", DDL_OPT_BOOL}, - {"owner", DDL_OPT_BOOL}, - }; + bool pretty; + bool no_owner; funcctx = SRF_FIRSTCALL_INIT(); oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); - if (isnull) - { - MemoryContextSwitchTo(oldcontext); - SRF_RETURN_DONE(funcctx); - } - - parse_ddl_options(fcinfo, 1, opts, lengthof(opts)); + pretty = PG_GETARG_BOOL(1); + no_owner = !PG_GETARG_BOOL(2); - statements = pg_get_tablespace_ddl_internal(tsid, - opts[0].isset && opts[0].boolval, - opts[1].isset && !opts[1].boolval); + statements = pg_get_tablespace_ddl_internal(tsid, pretty, no_owner); funcctx->user_fctx = statements; funcctx->max_calls = list_length(statements); @@ -813,14 +625,9 @@ pg_get_tablespace_ddl_srf(FunctionCallInfo fcinfo, Oid tsid, bool isnull) Datum pg_get_tablespace_ddl_oid(PG_FUNCTION_ARGS) { - Oid tsid = InvalidOid; - bool isnull; - - isnull = PG_ARGISNULL(0); - if (!isnull) - tsid = PG_GETARG_OID(0); + Oid tsid = PG_GETARG_OID(0); - return pg_get_tablespace_ddl_srf(fcinfo, tsid, isnull); + return pg_get_tablespace_ddl_srf(fcinfo, tsid); } /* @@ -830,19 +637,10 @@ pg_get_tablespace_ddl_oid(PG_FUNCTION_ARGS) Datum pg_get_tablespace_ddl_name(PG_FUNCTION_ARGS) { - Oid tsid = InvalidOid; - Name tspname; - bool isnull; + Name tspname = PG_GETARG_NAME(0); + Oid tsid = get_tablespace_oid(NameStr(*tspname), false); - isnull = PG_ARGISNULL(0); - - if (!isnull) - { - tspname = PG_GETARG_NAME(0); - tsid = get_tablespace_oid(NameStr(*tspname), false); - } - - return pg_get_tablespace_ddl_srf(fcinfo, tsid, isnull); + return pg_get_tablespace_ddl_srf(fcinfo, tsid); } /* @@ -1140,28 +938,20 @@ pg_get_database_ddl(PG_FUNCTION_ARGS) { MemoryContext oldcontext; Oid dbid; - DdlOption opts[] = { - {"pretty", DDL_OPT_BOOL}, - {"owner", DDL_OPT_BOOL}, - {"tablespace", DDL_OPT_BOOL}, - }; + bool pretty; + bool no_owner; + bool no_tablespace; funcctx = SRF_FIRSTCALL_INIT(); oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); - if (PG_ARGISNULL(0)) - { - MemoryContextSwitchTo(oldcontext); - SRF_RETURN_DONE(funcctx); - } - dbid = PG_GETARG_OID(0); - parse_ddl_options(fcinfo, 1, opts, lengthof(opts)); + pretty = PG_GETARG_BOOL(1); + no_owner = !PG_GETARG_BOOL(2); + no_tablespace = !PG_GETARG_BOOL(3); - statements = pg_get_database_ddl_internal(dbid, - opts[0].isset && opts[0].boolval, - opts[1].isset && !opts[1].boolval, - opts[2].isset && !opts[2].boolval); + statements = pg_get_database_ddl_internal(dbid, pretty, no_owner, + no_tablespace); funcctx->user_fctx = statements; funcctx->max_calls = list_length(statements); diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 2fe3be9ada5..635c0d9cb13 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -57,6 +57,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202606251 +#define CATALOG_VERSION_NO 202606281 #endif diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 384ba908d35..402d869710b 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -8591,30 +8591,29 @@ proname => 'pg_get_constraintdef', provolatile => 's', prorettype => 'text', proargtypes => 'oid bool', prosrc => 'pg_get_constraintdef_ext' }, { oid => '6501', descr => 'get DDL to recreate a role', - proname => 'pg_get_role_ddl', prorows => '10', provariadic => 'text', - proisstrict => 'f', proretset => 't', provolatile => 's', - pronargdefaults => '1', prorettype => 'text', proargtypes => 'regrole _text', - proallargtypes => '{regrole,_text}', proargmodes => '{i,v}', - proargdefaults => '{NULL}', prosrc => 'pg_get_role_ddl' }, + proname => 'pg_get_role_ddl', prorows => '10', proisstrict => 't', + proretset => 't', provolatile => 's', pronargdefaults => '2', + prorettype => 'text', proargtypes => 'regrole bool bool', + proargnames => '{role,pretty,memberships}', + proargdefaults => '{false,true}', prosrc => 'pg_get_role_ddl' }, { oid => '6499', descr => 'get DDL to recreate a tablespace', - proname => 'pg_get_tablespace_ddl', prorows => '10', provariadic => 'text', - proisstrict => 'f', proretset => 't', provolatile => 's', - pronargdefaults => '1', prorettype => 'text', proargtypes => 'oid _text', - proallargtypes => '{oid,_text}', proargmodes => '{i,v}', - proargdefaults => '{NULL}', prosrc => 'pg_get_tablespace_ddl_oid' }, + proname => 'pg_get_tablespace_ddl', prorows => '10', proisstrict => 't', + proretset => 't', provolatile => 's', pronargdefaults => '2', + prorettype => 'text', proargtypes => 'oid bool bool', + proargnames => '{tablespace,pretty,owner}', + proargdefaults => '{false,true}', prosrc => 'pg_get_tablespace_ddl_oid' }, { oid => '6500', descr => 'get DDL to recreate a tablespace', - proname => 'pg_get_tablespace_ddl', prorows => '10', provariadic => 'text', - proisstrict => 'f', proretset => 't', provolatile => 's', - pronargdefaults => '1', prorettype => 'text', proargtypes => 'name _text', - proallargtypes => '{name,_text}', proargmodes => '{i,v}', - proargdefaults => '{NULL}', prosrc => 'pg_get_tablespace_ddl_name' }, + proname => 'pg_get_tablespace_ddl', prorows => '10', proisstrict => 't', + proretset => 't', provolatile => 's', pronargdefaults => '2', + prorettype => 'text', proargtypes => 'name bool bool', + proargnames => '{tablespace,pretty,owner}', + proargdefaults => '{false,true}', prosrc => 'pg_get_tablespace_ddl_name' }, { oid => '6502', descr => 'get DDL to recreate a database', - proname => 'pg_get_database_ddl', prorows => '10', provariadic => 'text', - proisstrict => 'f', proretset => 't', provolatile => 's', - pronargdefaults => '1', prorettype => 'text', - proargtypes => 'regdatabase _text', proallargtypes => '{regdatabase,_text}', - proargmodes => '{i,v}', proargdefaults => '{NULL}', - prosrc => 'pg_get_database_ddl' }, + proname => 'pg_get_database_ddl', prorows => '10', proisstrict => 't', + proretset => 't', provolatile => 's', pronargdefaults => '3', + prorettype => 'text', proargtypes => 'regdatabase bool bool bool', + proargnames => '{database,pretty,owner,tablespace}', + proargdefaults => '{false,true,true}', prosrc => 'pg_get_database_ddl' }, { oid => '2509', descr => 'deparse an encoded expression with pretty-print option', proname => 'pg_get_expr', provolatile => 's', prorettype => 'text', diff --git a/src/test/modules/test_misc/t/012_ddlutils.pl b/src/test/modules/test_misc/t/012_ddlutils.pl index bcdb831a676..e541dfa38d1 100644 --- a/src/test/modules/test_misc/t/012_ddlutils.pl +++ b/src/test/modules/test_misc/t/012_ddlutils.pl @@ -93,7 +93,7 @@ like($result, qr/"regress_role-with-dash"/, 'role name requiring quoting'); # Pretty-printed output $result = $node->safe_psql('postgres', - q{SELECT * FROM pg_get_role_ddl('regress_role_ddl_test2', 'pretty', 'true')} + q{SELECT * FROM pg_get_role_ddl('regress_role_ddl_test2', pretty => true)} ); like($result, qr/\n\s+SUPERUSER/, 'role pretty-print indents attributes'); @@ -123,7 +123,7 @@ like($result, qr/ADMIN TRUE/, 'membership includes ADMIN TRUE'); # Memberships suppressed $result = $node->safe_psql('postgres', - q{SELECT * FROM pg_get_role_ddl('regress_role_ddl_member', 'memberships', 'false')} + q{SELECT * FROM pg_get_role_ddl('regress_role_ddl_member', memberships => false)} ); unlike($result, qr/GRANT/, 'memberships suppressed'); @@ -176,18 +176,18 @@ $result = $node->safe_psql('postgres', q{SELECT count(*) FROM pg_get_database_ddl(NULL)}); is($result, '0', 'NULL database returns no rows'); -# Invalid option +# Invalid option (bad boolean cast) ($ret, $stdout, $stderr) = $node->psql('postgres', - q{SELECT * FROM pg_get_database_ddl('regression_ddlutils_test', 'owner', 'invalid')} + q{SELECT * FROM pg_get_database_ddl('regression_ddlutils_test', owner => 'invalid')} ); isnt($ret, 0, 'invalid boolean option errors'); -like($stderr, qr/invalid value/, 'invalid option error message'); +like($stderr, qr/invalid input syntax for type boolean/, 'invalid option error message'); -# Duplicate option +# Duplicate named argument ($ret, $stdout, $stderr) = $node->psql( 'postgres', q{SELECT * FROM pg_get_database_ddl('regression_ddlutils_test', - 'owner', 'false', 'owner', 'true')}); + owner => false, owner => true)}); isnt($ret, 0, 'duplicate option errors'); # Basic output (without locale details) @@ -218,9 +218,17 @@ $result = ddl_filter( 'postgres', q{SELECT pg_get_database_ddl FROM pg_get_database_ddl('regression_ddlutils_test', - 'pretty', 'true', 'tablespace', 'false')})); + pretty => true, tablespace => false)})); like($result, qr/\n\s+WITH TEMPLATE/, 'database DDL pretty-prints WITH'); +# Owner suppressed +$result = ddl_filter( + $node->safe_psql( + 'postgres', + q{SELECT pg_get_database_ddl + FROM pg_get_database_ddl('regression_ddlutils_test', owner => false)})); +unlike($result, qr/OWNER/, 'database DDL owner suppressed'); + # Permission check $node->safe_psql( 'postgres', q{ @@ -292,14 +300,14 @@ like($result, qr/seq_page_cost='1.5'/, 'tablespace DDL includes options'); $result = $node->safe_psql( 'postgres', q{SELECT * FROM pg_get_tablespace_ddl('regress_allopt_tblsp', - 'pretty', 'true')}); + pretty => true)}); like($result, qr/\n\s+OWNER/, 'tablespace DDL pretty-prints OWNER'); # Owner suppressed $result = $node->safe_psql( 'postgres', q{SELECT * FROM pg_get_tablespace_ddl('regress_allopt_tblsp', - 'owner', 'false')}); + owner => false)}); unlike($result, qr/OWNER/, 'tablespace DDL owner suppressed'); # Lookup by OID