MIME-Version: 1.0
References: 
 <CANxoLDfjQnhM=E6JSyYo9s9OdjqoN8s_3wE5yL=kaDu_X8j-dA@mail.gmail.com>
 <CANxoLDcxmXNHNyPt5v+LaBJVJmyt=j95D7TDE9SepoM7y15t-w@mail.gmail.com>
 <CAN4CZFNqf41TCg1ENccq6DAYQpArCbnKUSidHnRKaVMWGj5pBA@mail.gmail.com>
 <CANxoLDfL1AxL=k9SsRNReKQ-sJUSy6RvCqeog5HWckFxff=0Pg@mail.gmail.com>
 <CAN4CZFP5-fAGwxMCyKGePgp3N8NCpoFXkruO=5xjTC=gPwqYxw@mail.gmail.com>
 <CANxoLDc6UFMsyzbXKF8SpMoXdoKE=-yRPqtvyBMKKJ2zOUKX4A@mail.gmail.com>
 <CAN4CZFMhnZyQsdLC5iZNego77JHJyP6J_4CpWu=99vKHCd-vzg@mail.gmail.com>
 <CANxoLDco7z7Fj7_pj_3YQLvjq8ZH2Ai4c3GcOamed01fZhJ77Q@mail.gmail.com>
 <CAB-JLwY=RYPJhdFid0YDqk1L1OQPuHFz9J2tOeeVCFLjov+Ktg@mail.gmail.com>
 <CANxoLDdRts-F+QxL+wtM8529Z3+TUJku8PKy3WoxcWBs7zQJ-w@mail.gmail.com>
 <CAB-JLwbyzF+JRPCoLCf-H6dQ_1+-O_wBZcESRdC6_H2z+-d-gg@mail.gmail.com>
 <CANxoLDcWwoWOxvmV_uBBUsbGf+_AZzBV1GkcjoX+omNpcrCNMQ@mail.gmail.com>
 <CAN4CZFM99cGopPJXD_t05+Y9CnocOOTipfXCzoAfBUFSC-i4+A@mail.gmail.com>
 <CANxoLDejn2JFXF7kVMtcznW6XyessrJ7WToSaQau2GxBdAGSUg@mail.gmail.com>
 <983C7B03-D9C3-4F15-8913-7616EC5E6414@gmail.com>
 <CANxoLDe_GOXyyZs7GN3VUJoT+o1DwqZGhid-TWWvS0D8d5ggYw@mail.gmail.com>
 <20260623.162104.775158751399277188.horikyota.ntt@gmail.com>
 <CANxoLDdq06oXRYuzYfn2gkn3yfoxp=nA14xu5p+qSEOzpHXEMQ@mail.gmail.com>
 <CANxoLDdEBYzKfj3NGrP5PjoTa-eCViWPkFR-sXLq4bUnbokyPA@mail.gmail.com>
 <CAN4CZFPKAr2VfxGMTA4CkMPHVSnpQrRKJcwUH1ZLh4eHG09LSw@mail.gmail.com>
 <CANxoLDe308ogjUZD4DLjaMysJzHZ4z3SOXbUTyC7SAEKvSgqcw@mail.gmail.com>
In-Reply-To: 
 <CANxoLDe308ogjUZD4DLjaMysJzHZ4z3SOXbUTyC7SAEKvSgqcw@mail.gmail.com>
From: Rui Zhao <zhaorui126@gmail.com>
Date: Mon, 29 Jun 2026 00:44:31 +0800
Message-ID: 
 <CAHWVJhHW4jcfS=w20Ungecb3A4uWiHe9OM9OLHHBtokhKaCs=w@mail.gmail.com>
Subject: Re: [PATCH] Add pg_get_table_ddl() to reconstruct CREATE TABLE
 statements
To: Akshay Joshi <akshay.joshi@enterprisedb.com>
Cc: pgsql-hackers@lists.postgresql.org, marcos@f10.com.br,
	horikyota.ntt@gmail.com, li.evan.chao@gmail.com,
	Zsolt Parragi <zsolt.parragi@percona.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHWVJhHW4jcfS%3Dw20Ungecb3A4uWiHe9OM9OLHHBtokhKaCs%3Dw%40mail.gmail.com>
Precedence: bulk

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.

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.

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

Otherwise it looks good.

Thanks,
Rui