commit 9ba82a3 (HEAD -> ddlutils-defect-tests)
Author:     Noah Misch <nm@cqla.c.codeql-202606a.internal>
AuthorDate: Sat Jul 11 16:30:54 2026 +0000
Commit:     Noah Misch <nm@cqla.c.codeql-202606a.internal>
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 <noreply@anthropic.com>
    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.
