Re: [PATCH] Add pg_get_table_ddl() to reconstruct CREATE TABLE statements

On Sun, Jun 28, 2026 at 10:14 PM Rui Zhao <zhaorui126(at)gmail(dot)com> wrote:

> Hi Akshay,
>
> Nice patch -- server-side CREATE TABLE reconstruction is something we want
> in
> production. I tested v11 on current master and it round-trips correctly
> across the documented coverage, including the recent grammar (VIRTUAL
> columns, NOT ENFORCED, temporal WITHOUT OVERLAPS / PERIOD) and state set
> later via ALTER. A few comments:
>
> 1. Real bug in schema_qualified => false. strip_schema_prefix() strips the
> base prefix anywhere in code position with no token-boundary check, so a
> cross-schema name whose schema ends with the base schema's name gets
> mangled:
>
> postgres=# CREATE SCHEMA xs;
> CREATE SCHEMA
> postgres=# CREATE TABLE xs.reftbl(id int PRIMARY KEY);
> CREATE TABLE
> postgres=# CREATE SCHEMA s;
> CREATE SCHEMA
> postgres=# CREATE TABLE s.orders(id int PRIMARY KEY, ref int
> REFERENCES xs.reftbl(id));
> CREATE TABLE
> postgres=# SELECT d FROM
> pg_get_table_ddl('s.orders'::regclass,'schema_qualified','false') d;
> d
>
> ---------------------------------------------------------------------------------------------
> CREATE TABLE orders (id integer NOT NULL, ref integer);
> ALTER TABLE orders OWNER TO postgres;
> ALTER TABLE orders ADD CONSTRAINT orders_pkey PRIMARY KEY (id);
> ALTER TABLE orders ADD CONSTRAINT orders_ref_fkey FOREIGN KEY (ref)
> REFERENCES xreftbl(id);
> (4 rows)
>
> The last FK line says REFERENCES xreftbl(id); it should be xs.reftbl(id),
> and the result doesn't replay. Gating the match on a token boundary fixes
> it -- though see (2), which would remove this code path (and the bug)
> entirely.
>

Fixed.

>
> 2. Bigger picture: is schema_qualified needed at all? None of the existing
> pg_get_*def / pg_get_*ddl functions have such a knob, so this would be the
> lone exception. The established convention is to let search_path decide
> (generate_relation_name): pg_get_viewdef and pg_get_constraintdef already
> work that way, and pg_get_indexdef supports it too via
> pg_get_indexdef(idx, 0, true) (the PRETTYFLAG_SCHEMA code path). It is
> genuinely what we want in production -- the caller
> controls qualification through search_path. Set it to the table's schema
> for
> unqualified output, or to pg_catalog (or '') for fully-qualified
> schema.table; pg_dump itself dumps under an empty search_path
> (ALWAYS_SECURE_SEARCH_PATH_SQL) for exactly this reason. Following the
> convention would also drop the option and the strip_schema_prefix code (and
> this bug).
>
> That last point matters: there's no robust way to strip a schema prefix out
> of already-generated SQL by text processing. Doing it safely means
> re-tokenizing arbitrary SQL (string literals, quoted identifiers, dollar
> quotes, comments, casts, operators, ...), and strip_schema_prefix is a
> hand-rolled partial scanner of exactly that. It shows -- it has already
> needed several over-stripping fixes during review (the base name appearing
> inside a string literal, and inside a quoted identifier), and the
> token-boundary case in (1) is yet another. Deciding qualification at
> generation time (generate_relation_name) avoids the whole class: the
> backend's real deparser already gets this right, rather than a post-hoc
> string pass trying to re-derive it.
>

Done. strip_schema_prefix and append_stripped_stmt are gone. Instead of
post-processing, I added four thin wrappers in ruleutils.c. Let the
deparser decide qualification at generation time:
- pg_get_indexdef_ddl passes PRETTYFLAG_SCHEMA to the worker, enabling
generate_relation_name instead of generate_qualified_relation_name.
- pg_get_ruledef_ddl — same flag.
- pg_get_constraintdef_body — returns body only (FK targets already use
generate_relation_name); emit_local_constraints now builds the ALTER TABLE
ctx->qualname ADD CONSTRAINT prefix itself.
- pg_get_statisticsobjdef_ddl — uses StatisticsObjIsVisible() to qualify
the statistics object name
*The schema_qualified=false path still narrows search_path to the base
schema, which is what makes all four helpers emit unqualified names for
same-schema objects. The option is kept for per-call convenience.*

> 3. typedefs.list is missing TableDdlContext and LocalNotNullEntry, so
> pgindent leaves the "Type * var" pointer spacing in ~30 places -- for
> example the forward declarations at ddlutils.c:226-237:
>
> static void append_stmt(TableDdlContext * ctx);
>
> There is also a stale comment at ddlutils.c:1830 in append_column_defs():
> inherited columns are described as "emitted by the INHERITS clause (once
> implemented)", but INHERITS is implemented now.
>
> 4. Related to (2): a temporary table's default output qualifies it with the
> session-private pg_temp_NN schema (e.g. CREATE TEMPORARY TABLE pg_temp_3.t
> ...), which won't replay anywhere else. A reconstructed temp table should
> just be CREATE TEMPORARY TABLE t (...) -- the TEMPORARY keyword already
> puts
> it in pg_temp, so the schema name should never be emitted. This also falls
> out for free under the search_path convention in (2): pg_temp is always in
> the effective search_path, so the table is visible and wouldn't be
> qualified
> in the first place.
>
> 5. The commit message is out of sync with the code and func-info.sgml on
> the
> option interface: it still says "include and exclude" and lists plural kind
> names (indexes, foreign_keys, triggers, policies, partitions), whereas the
> code and docs use only/except and singular names (index, foreign_key,
> trigger, policy, partition). Looks like a leftover from the
> include/exclude -> only/except rename. (FWIW on the naming itself,
> include/exclude is the more common convention for this kind of list
> parameter -- pg_dump has --exclude-table/-schema/-extension, etc. -- while
> only/except reads more like the SQL keywords.)
>

Fixed 3, 4 and 5.

The v12 patch is ready for review/test.

>
> Otherwise it looks good.
>
> Thanks,
> Rui
>