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

Fixed some compiler warnings. The *v13* patch is ready for review/test.

On Mon, Jun 29, 2026 at 4:10 PM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

>
>
> 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
>>
>