commit 9ba82a3 (HEAD -> ddlutils-defect-tests) Author: Noah Misch AuthorDate: Sat Jul 11 16:30:54 2026 +0000 Commit: Noah Misch 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 " (even for the caller's own role, which any unprivileged user hits on a stock install) and "permission denied for tablespace ". 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 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 " / +# "permission denied for tablespace ", 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.