commit 76e514e wrote: > Author: Andrew Dunstan <> > AuthorDate: Thu Mar 19 09:52:25 2026 -0400 > Commit: Andrew Dunstan <[email protected]> > CommitDate: Sun Apr 5 10:54:54 2026 -0400 > > Add pg_get_role_ddl() function
I reviewed this commit. > Author: Mario Gonzalez <[email protected]> > Author: Bryan Green <[email protected]> > Co-authored-by: Andrew Dunstan <[email protected]> > Co-authored-by: Euler Taveira <[email protected]> > Reviewed-by: Japin Li <[email protected]> > Reviewed-by: Quan Zongliang <[email protected]> > Reviewed-by: jian he <[email protected]> > Discussion: > https://postgr.es/m/[email protected] > Discussion: > https://postgr.es/m/[email protected] > + /* > + * We intentionally omit PASSWORD. There's no way to retrieve the > + * original password text from the stored hash, and even if we could, > + * exposing passwords through a SQL function would be a security issue. > + * Users must set passwords separately after recreating roles. > + */ pg_dumpall recreates password hashes without needing the original plaintext. The first thread message said the use case is "dumping role definitions for migration or backup purposes without needing pg_dumpall." Users expect their passwords to be accepted after migration or restore from backup. I also don't see a security distinction arising merely because SQL is the conduit. > + * pg_get_role_ddl_internal > + * Generate DDL statements to recreate a role > + /* Build a fresh ALTER ROLE statement for this setting > */ > + resetStringInfo(&buf); > + appendStringInfo(&buf, "ALTER ROLE %s", > quote_identifier(rolname)); > + > + if (datname != NULL) > + appendStringInfo(&buf, " IN DATABASE %s", > + > quote_identifier(datname)); This doesn't deal with dependencies. To migrate, you need to dump roles first, then databases (potentially owned by roles), then IN DATABASE ... SET statements (which depend on both). By putting IN DATABASE in the same payload as CREATE ROLE, it's not conducive to restoring from an empty cluster. The caller would need to break apart the payload and do its own dependency analysis, which substantially defeats the point of having $SUBJECT. The word "depend" appears nowhere on the threads or in this commit. For a feature aiming for an outcome like pg_dump, I think dependency handling needs to be foundational in the design. > + /* > + * Scan pg_auth_members for role memberships. We look for rows where > + * member = roleid, meaning this role has been granted membership in > other > + * roles. The corresponding step in pg_dumpall is much more complicated; see this comment in dumpRoleMembership(): /* * We can't dump these GRANT commands in arbitrary order, because a role * that is named as a grantor must already have ADMIN OPTION on the role * for which it is granting permissions, except for the bootstrap * superuser, who can always be named as the grantor. * * We handle this by considering these grants role by role. For each role, * we initially consider the only allowable grantor to be the bootstrap * superuser. Every time we grant ADMIN OPTION on the role to some user, * that user also becomes an allowable grantor. We make repeated passes * over the grants for the role, each time dumping those whose grantors * are allowable and which we haven't done yet. Eventually this should let * us dump all the grants. */ If the backend version achieves the right outcomes without that complexity, it should have a comment about how it achieves that. > src/backend/utils/adt/ddlutils.c | 361 > +++++++++++++++++++++++++++++++++ Separate from the above correctness problems, I object to having two independent implementations in the tree for translating catalog state into SQL that recreates that state. pg_dump support is already a key friction source for implementing most new catalog-stored features. I don't want such features to face updating both a src/bin/pg_dump implementation and an independent backend implementation, each with its own bugs. Wrapping pg_dump and pg_dumpall is already a reasonable implementation of these use cases, so I think the bar for introducing another implementation is high. A shared implementation used by both could potentially clear that bar; this commit doesn't start in that direction. PostgreSQL should not carry two independent implementations of this logic. I also ran an Opus 4.8 review. It found some minor issues that aren't worth addressing before the above. I'm attaching it for completeness.
commit 9ba82a3 (HEAD -> ddlutils-defect-tests) Author: Noah Misch <[email protected]> AuthorDate: Sat Jul 11 16:30:54 2026 +0000 Commit: Noah Misch <[email protected]> CommitDate: Sat Jul 11 18:22:19 2026 +0000 Add TAP test documenting known pg_get_*_ddl defects The new test test_misc/t/015_ddlutils_defects.pl demonstrates two confirmed, still-unfixed defects in the DDL generation functions. Each defect is asserted as its correct expected behavior inside a TODO block, so the assertions currently fail as TAP expected failures and the suite stays green; once a defect is fixed, the corresponding assertion passes and the TODO wrapper should be removed. 1. pg_get_database_ddl() omits the TABLESPACE clause for a database in a user tablespace named "PG_DEFAULT". The clause is suppressed with pg_strcasecmp(spcname, "pg_default"), but tablespace names are case-sensitive and the reserved-name check only rejects lowercase "pg_" prefixes, so such a tablespace is legal and distinct from the built-in pg_default. Expected: the emitted CREATE DATABASE carries TABLESPACE = "PG_DEFAULT" (pg_dump compares with case-sensitive strcmp() and emits it). Actual: no TABLESPACE clause; replaying the output succeeds but silently places the database in the built-in pg_default tablespace -- a lossy round trip. 2. Misleading permission-denied errors. pg_get_role_ddl() requires SELECT on pg_authid and pg_get_tablespace_ddl() requires SELECT on pg_tablespace, but on failure they report "permission denied for role <target>" (even for the caller's own role, which any unprivileged user hits on a stock install) and "permission denied for tablespace <target>". Expected: an error naming the catalog whose SELECT privilege is missing, as a direct SELECT from that catalog reports. Actual: only the irrelevant target object is named, with nothing pointing at the real remedy. The test is registered in the test_misc meson.build test list; the Makefile picks it up automatically via TAP_TESTS. The hunt confirmed three further user-visible defects that are not encoded in the test because they are documentation gaps (the function behavior is intentional, but doc/src/sgml/func/func-info.sgml does not describe it): the reference omits the functions' privilege requirements, omits that they refuse reserved/system targets (pg_default/pg_global, pg_-prefixed roles, template0/template1), and omits ALLOW_CONNECTIONS from pg_get_database_ddl()'s list of emitted ALTER DATABASE statements. These are written up, with reproductions, in the companion 015_ddlutils_defects_uncovered.md. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01Bhit3mWtVg5qYPW8F78h5J --- src/test/modules/test_misc/meson.build | 1 + .../modules/test_misc/t/015_ddlutils_defects.pl | 186 +++++++++++++++++++++ .../test_misc/t/015_ddlutils_defects_uncovered.md | 171 +++++++++++++++++++ 3 files changed, 358 insertions(+) diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build index ee29069..3348350 100644 --- a/src/test/modules/test_misc/meson.build +++ b/src/test/modules/test_misc/meson.build @@ -23,6 +23,7 @@ tests += { 't/012_ddlutils.pl', 't/013_temp_obj_multisession.pl', 't/014_log_statement_max_length.pl', + 't/015_ddlutils_defects.pl', ], # The injection points are cluster-wide, so disable installcheck 'runningcheck': false, diff --git a/src/test/modules/test_misc/t/015_ddlutils_defects.pl b/src/test/modules/test_misc/t/015_ddlutils_defects.pl new file mode 100644 index 0000000..f96a79d --- /dev/null +++ b/src/test/modules/test_misc/t/015_ddlutils_defects.pl @@ -0,0 +1,186 @@ + +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Tests demonstrating known, still-unfixed defects in the DDL generation +# functions pg_get_database_ddl(), pg_get_role_ddl(), and +# pg_get_tablespace_ddl(). Companion to 012_ddlutils.pl, which covers the +# intended behavior of these functions. +# +# Each defect is encoded by asserting the CORRECT (expected) behavior and +# wrapping that assertion in a TODO block, so that: +# - while the defect exists, the assertion fails but is reported as an +# expected failure ("not ok ... # TODO"), keeping the suite green; +# - once the defect is fixed, the assertion passes, at which point the +# TODO wrapper should be removed. +# Assertions outside the TODO blocks hold both before and after a fix. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Package variable consulted by Test::More inside the TODO blocks below. +our $TODO; + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +# Force UTC so that any timestamptz values render the same way regardless +# of the host's local timezone. +$node->append_conf('postgresql.conf', "timezone = 'UTC'\n"); +$node->start; + + +######################################################################## +# Defect 1: pg_get_database_ddl() omits the TABLESPACE clause for a +# database whose tablespace is a user tablespace named "PG_DEFAULT" +# (a silently lossy round trip). +# +# pg_get_database_ddl_internal() decides whether to emit the TABLESPACE +# clause with pg_strcasecmp(spcname, "pg_default"). But tablespace names +# are case-sensitive identifiers, and the reserved-name check only rejects +# lowercase "pg_"-prefixed names, so CREATE TABLESPACE "PG_DEFAULT" is +# perfectly legal and is a distinct tablespace from the built-in +# pg_default. pg_dump makes the same "is it the default?" decision with a +# case-sensitive strcmp() and therefore emits TABLESPACE "PG_DEFAULT" +# correctly. +# +# Expected: the emitted CREATE DATABASE carries TABLESPACE = "PG_DEFAULT", +# and replaying the DDL recreates the database in that tablespace. +# Actual (currently broken): no TABLESPACE clause is emitted; the replay +# succeeds without error but silently places the database in the built-in +# pg_default tablespace instead. +######################################################################## + +# The reserved-name check is case-sensitive, so this name is allowed. +# (It intentionally lacks the regress_ prefix: the defect requires the +# exact spelling "PG_DEFAULT".) +$node->safe_psql( + 'postgres', q{ + SET allow_in_place_tablespaces = true; + CREATE TABLESPACE "PG_DEFAULT" LOCATION ''}); +$node->safe_psql( + 'postgres', q{ + CREATE DATABASE regress_getddl_pgdef + TABLESPACE "PG_DEFAULT" TEMPLATE template0}); + +# owner => false keeps the output free of the (installation-dependent) +# bootstrap superuser name, so the DDL can be replayed verbatim below. +my $ddl = $node->safe_psql( + 'postgres', + q{SELECT * FROM pg_get_database_ddl('regress_getddl_pgdef', + owner => false)}); +like( + $ddl, + qr/CREATE DATABASE regress_getddl_pgdef/, + 'database DDL for tablespace "PG_DEFAULT" includes CREATE DATABASE'); + +{ + local $TODO = 'pg_get_database_ddl: TABLESPACE clause suppressed by ' + . 'case-insensitive comparison against pg_default'; + + like( + $ddl, + qr/TABLESPACE = "PG_DEFAULT"/, + 'database DDL includes TABLESPACE = "PG_DEFAULT"'); +} + +# Round trip: drop the database and replay the captured DDL. The replay +# itself succeeds both before and after a fix; only the resulting +# database's tablespace differs. +$node->safe_psql('postgres', 'DROP DATABASE regress_getddl_pgdef'); +$node->safe_psql('postgres', $ddl); +my $spc = $node->safe_psql( + 'postgres', q{ + SELECT t.spcname + FROM pg_database d JOIN pg_tablespace t ON t.oid = d.dattablespace + WHERE d.datname = 'regress_getddl_pgdef'}); + +{ + local $TODO = 'pg_get_database_ddl: replayed DDL silently lands the ' + . 'database in pg_default instead of "PG_DEFAULT"'; + + is($spc, 'PG_DEFAULT', + 'replayed database DDL preserves tablespace "PG_DEFAULT"'); +} + +$node->safe_psql('postgres', 'DROP DATABASE regress_getddl_pgdef'); +$node->safe_psql('postgres', 'DROP TABLESPACE "PG_DEFAULT"'); + + +######################################################################## +# Defect 2: misleading "permission denied" errors when the caller lacks +# SELECT on the system catalog that the function actually reads. +# +# pg_get_role_ddl() requires SELECT on pg_authid, and +# pg_get_tablespace_ddl() requires SELECT on pg_tablespace. When that +# check fails, however, the error names the *target* object ("permission +# denied for role ...", "permission denied for tablespace ..."), which by +# PostgreSQL convention means the caller lacks a privilege on that named +# object. Nothing points at the actual remedy (GRANT SELECT ON pg_authid +# / pg_tablespace); the role message even blames the caller's own role +# when it introspects itself. (pg_get_database_ddl() is not affected: it +# genuinely checks CONNECT on the named database.) +# +# Expected: the error identifies the catalog whose SELECT privilege is +# missing -- e.g. "permission denied for table pg_authid", exactly as a +# direct SELECT FROM pg_authid reports. +# Actual (currently broken): "permission denied for role <target>" / +# "permission denied for tablespace <target>", naming neither catalog. +######################################################################## + +$node->safe_psql( + 'postgres', q{ + CREATE ROLE regress_getddl_unpriv LOGIN; + SET allow_in_place_tablespaces = true; + CREATE TABLESPACE regress_getddl_ts LOCATION ''; + REVOKE SELECT ON pg_tablespace FROM PUBLIC}); + +# pg_authid is never PUBLIC-readable, so on a stock install any +# unprivileged role hits this even when introspecting its own role. +my ($ret, $stdout, $stderr) = $node->psql( + 'postgres', q{ + SET ROLE regress_getddl_unpriv; + SELECT * FROM pg_get_role_ddl(current_user::regrole)}); +isnt($ret, 0, 'role DDL denied without SELECT on pg_authid'); +like( + $stderr, + qr/permission denied/, + 'role DDL permission failure reports "permission denied"'); + +{ + local $TODO = 'pg_get_role_ddl: error blames the target role instead ' + . 'of naming the missing SELECT privilege on pg_authid'; + + like( + $stderr, + qr/pg_authid/, + 'role DDL permission error identifies pg_authid'); +} + +($ret, $stdout, $stderr) = $node->psql( + 'postgres', q{ + SET ROLE regress_getddl_unpriv; + SELECT * FROM pg_get_tablespace_ddl('regress_getddl_ts')}); +isnt($ret, 0, 'tablespace DDL denied without SELECT on pg_tablespace'); +like( + $stderr, + qr/permission denied/, + 'tablespace DDL permission failure reports "permission denied"'); + +{ + local $TODO = 'pg_get_tablespace_ddl: error blames the target ' + . 'tablespace instead of naming the missing SELECT privilege on ' + . 'pg_tablespace'; + + like( + $stderr, + qr/pg_tablespace/, + 'tablespace DDL permission error identifies pg_tablespace'); +} + +$node->safe_psql('postgres', 'GRANT SELECT ON pg_tablespace TO PUBLIC'); + +$node->stop; + +done_testing(); diff --git a/src/test/modules/test_misc/t/015_ddlutils_defects_uncovered.md b/src/test/modules/test_misc/t/015_ddlutils_defects_uncovered.md new file mode 100644 index 0000000..7ff06c6 --- /dev/null +++ b/src/test/modules/test_misc/t/015_ddlutils_defects_uncovered.md @@ -0,0 +1,171 @@ +# pg_get_*_ddl defects not covered by 015_ddlutils_defects.pl + +A defect hunt over the `pg_get_role_ddl()`, `pg_get_tablespace_ddl()`, and +`pg_get_database_ddl()` feature (commits `76e514ebb4`, `b99fd9fd7f`, +`a4f774cf1c`) on `master` (`5f14f82`) confirmed five still-present, +user-visible defects. The companion TAP test +[`015_ddlutils_defects.pl`](015_ddlutils_defects.pl) encodes the two that +are defects in *function behavior*: + +1. `pg_get_database_ddl()` emits a lossy round trip for a database in a + user tablespace named `"PG_DEFAULT"` (a `TABLESPACE` clause is dropped). +2. The role/tablespace permission errors name the target object instead of + the catalog whose `SELECT` privilege is actually missing. + +The remaining **three** confirmed defects are **documentation** gaps: the +function behavior is intentional, but the SGML reference +(`doc/src/sgml/func/func-info.sgml`, the *Get Object DDL Functions* table) +does not describe it, so a user relying on the documented contract is +surprised. They are not encoded in the TAP test because a meaningful check +would assert on the rendered documentation, not on the functions' behavior; +they are recorded here instead. All three were reproduced from scratch on a +fresh `master` cluster. + +Unless noted, the reproductions assume a superuser `psql` session and, for +the in-place tablespace, `SET allow_in_place_tablespaces = on;`. + +--- + +## D1. Docs omit the privilege requirements of all three functions + +**Category:** doc_mismatch · **Severity:** low (documentation only) + +Each function is privilege-gated in code, but the documentation states no +privilege requirement at all: + +| Function | Required privilege | Enforced at | +| --- | --- | --- | +| `pg_get_role_ddl` | `SELECT` on `pg_authid` | `ddlutils.c:172` | +| `pg_get_tablespace_ddl` | `SELECT` on `pg_tablespace` | `ddlutils.c:502` | +| `pg_get_database_ddl` | `CONNECT` on the target database | `ddlutils.c:680` | + +The `pg_get_role_ddl` case is the sharpest: `pg_authid` is never +PUBLIC-readable, so on a stock install **any** non-superuser hits an +undocumented `permission denied` error — even when introspecting its own +role — although everything the function emits is otherwise visible through +`pg_roles` / `pg_db_role_setting` / `pg_auth_members` (the password is +omitted by design). It is also undocumented that `GRANT pg_read_all_data` +is enough to satisfy the role and tablespace functions. + +**Expected:** the reference lists each function's privilege requirement, as +PostgreSQL customarily documents for privilege-gated functions. + +**Actual:** the *Get Object DDL Functions* table contains no mention of +`privilege`, `permission`, `pg_authid`, or `superuser`. + +**Reproduction:** + +```sql +CREATE ROLE regress_unpriv LOGIN; +SET allow_in_place_tablespaces = on; +CREATE TABLESPACE regress_ts LOCATION ''; +CREATE DATABASE regress_ddb; +REVOKE SELECT ON pg_tablespace FROM PUBLIC; +REVOKE CONNECT ON DATABASE regress_ddb FROM PUBLIC; + +SET ROLE regress_unpriv; +SELECT * FROM pg_get_role_ddl(current_user::regrole); -- ERROR: permission denied for role regress_unpriv +SELECT * FROM pg_get_tablespace_ddl('regress_ts'); -- ERROR: permission denied for tablespace regress_ts +SELECT * FROM pg_get_database_ddl('regress_ddb'); -- ERROR: permission denied for database regress_ddb +RESET ROLE; + +GRANT pg_read_all_data TO regress_unpriv; +SET ROLE regress_unpriv; +SELECT count(*) FROM pg_get_role_ddl(current_user::regrole); -- now succeeds (undocumented) +``` + +**Suggested fix:** add a sentence per function to the reference stating the +required privilege (mirroring the code comments "User must have SELECT +privilege on pg_authid/pg_tablespace"). + +--- + +## D2. Docs omit that the functions refuse reserved / system targets + +**Category:** doc_mismatch · **Severity:** low (documentation only) + +Each function refuses a class of objects that exists in every cluster, with +`ERROR` code `42939` (reserved_name): + +| Call | Error | Raised at | +| --- | --- | --- | +| `pg_get_tablespace_ddl('pg_default')` / `('pg_global')` | `tablespace name "..." is reserved` | `ddlutils.c:512` | +| `pg_get_role_ddl('pg_monitor')` (any `pg_*` role) | `role name "pg_monitor" is reserved` | `ddlutils.c:184` | +| `pg_get_database_ddl('template0')` / `('template1')` | `database "..." is a system database` | `ddlutils.c:702` | + +The refusals themselves are intentional and consistent. But the reference +says each function "Reconstructs the CREATE … statement for the specified +…" with no stated exclusions and no documented error conditions, so a user +iterating over `pg_tablespace`, `pg_authid`, or `pg_database` (a natural way +to dump every object) hits unexpected errors. + +**Expected:** the reference notes that `pg_`-prefixed tablespaces/roles and +`template0`/`template1` are rejected (or, at least, that error conditions +exist). + +**Actual:** the table mentions none of `reserved`, `system`, `pg_default`, +`pg_global`, or `template0` (`template` appears only in "template status"). + +**Reproduction:** + +```sql +SELECT * FROM pg_get_tablespace_ddl('pg_default'); -- ERROR: tablespace name "pg_default" is reserved +SELECT * FROM pg_get_tablespace_ddl('pg_global'); -- ERROR: tablespace name "pg_global" is reserved +SELECT * FROM pg_get_role_ddl('pg_monitor'); -- ERROR: role name "pg_monitor" is reserved +SELECT * FROM pg_get_database_ddl('template1'); -- ERROR: database "template1" is a system database +SELECT * FROM pg_get_database_ddl('template0'); -- ERROR: database "template0" is a system database +``` + +**Suggested fix:** document the refused targets for each function. + +--- + +## D3. Docs omit `ALLOW_CONNECTIONS` from `pg_get_database_ddl`'s output list + +**Category:** doc_mismatch · **Severity:** low (documentation only) + +The reference enumerates the `ALTER DATABASE` statements the function emits +as being for "connection limit, template status, and configuration +settings" (`func-info.sgml:3896`). But the function also emits +`ALTER DATABASE … ALLOW_CONNECTIONS = false` when `datallowconn` is false +(`ddlutils.c:833`), which the enumeration does not cover. A reader would +wrongly conclude the allow-connections state is not captured. + +**Expected:** the enumeration includes the allow-connections state, e.g. +"… connection limit, template status, allow-connections status, and +configuration settings." + +**Actual:** `ALLOW_CONNECTIONS` is emitted but absent from the documented +list. + +**Reproduction:** + +```sql +CREATE DATABASE regress_db1 ALLOW_CONNECTIONS false; +SELECT * FROM pg_get_database_ddl('regress_db1'); +-- CREATE DATABASE regress_db1 WITH TEMPLATE = template0 ENCODING = ... ; +-- ALTER DATABASE regress_db1 OWNER TO ...; +-- ALTER DATABASE regress_db1 ALLOW_CONNECTIONS = false; <-- not in the docs' list +``` + +**Related nit (same paragraph):** the emitted `ALTER DATABASE … OWNER TO` +statement is likewise absent from the enumeration, though the `owner` +parameter description partially covers it. + +**Suggested fix:** extend the enumeration to mention allow-connections (and, +optionally, owner). + +--- + +## Verification notes + +- All reproductions above were run on a fresh `master` (`5f14f82`) assert + build and matched exactly (error text, SQLSTATE `42939`/`42501`, and the + `ddlutils.c` line numbers cited). +- None of these correspond to the already-committed follow-up fixes + (`d6ed87d`, `1f108fc`, `cda0c4c`, `5642a03`, `6c7bce2`). +- The feature is otherwise robust on the surfaces exercised during the hunt: + attribute/GUC/membership round trips, ICU/builtin/libc locale handling, + `GRANTED BY` membership replay, identifier and literal quoting (including + injection attempts), and the set-returning call paths all round-tripped or + behaved correctly.
