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

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