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

Thanks for the detailed pass.

A. The 31 design-expected, 17 artifact, and 10 column-reordering cases are
all accounted for. The EXCLUDE naming gap is real — after ATTACH
PARTITION, ATExecAttachPartition clears conislocal on the merged child
constraint, so we never see it in emit_local_constraints. Fixing it cleanly
would require replicating ChooseConstraintName's logic to emit a post-ADD
CONSTRAINT RENAME CONSTRAINT, which is fragile. I'm deferring it alongside
the column-reordering limitation.

B. Fixed both. emit_cluster_on now calls get_index_constraint on the
clustered index and maps its type to the appropriate TableDdlKind; CLUSTER
ON is silently skipped when the backing kind is filtered (consistent with
how the plain-index case already drops when INDEX is excluded). For the
self-referential FK, a new fk_backing_constraint_is_blocked helper scans
pg_constraint for the matching PK/UNIQUE on confrelid and raises an ERROR
with the same message/detail/hint structure as the REPLICA IDENTITY guard
when the backing kind is filtered. Cross-table FKs are intentionally not
checked because the referenced table's DDL is independent. Test cases added
for all paths including the only_kinds form and the cross-table no-error
case.

C. Fixed. check_stack_depth() added immediately before
CHECK_FOR_INTERRUPTS() in emit_partition_children.

The *v23* patch is ready for review/test.

On Fri, Jul 24, 2026 at 12:48 PM Rui Zhao <zhaorui126(at)gmail(dot)com> wrote:

> Hi Akshay,
>
> Sorry for the slow turnaround -- the World Cup got the better of my
> evenings.
>
> A) Round-trip against pg_dump. I re-ran it on v22 (master 8767a10cb8): for
> each
> of the 563 top-level tables in the regression database I ran
> pg_get_table_ddl,
> dropped and recreated that table from its output in a copy of the
> database, and
> diffed it against pg_dump. 504 of 563 round-trip clean, and the rate has
> climbed
> with each round -- 483/560 at v17, 494/562 at v19, 501/562 at v21,
> 504/563 now --
> with installcheck passing throughout.
>
> For the record, here is what the remaining 59 are:
>
> - 31 are objects pg_get_table_ddl doesn't emit by design (triggers,
> grants,
> policies, comments, owned sequences). The pg_dump comparison flags
> them, but
> that is expected.
>
> - 17 are artifacts of testing (I replay into a copy with the table
> dropped),
> not defects: 14 rely on an owned sequence or dependent function
> that goes with
> the drop and isn't part of the table's own DDL; 3 pick up a "1" suffix
> on a
> NOT NULL constraint whose name a sibling table still holds. The
> emitted DDL is
> correct.
>
> - 10 are column reordering on partition or inheritance children -- the
> reordered-column limitation you deferred, plus its
> inheritance-child variant.
>
> - 1 is real but cosmetic: a partition child's named EXCLUDE constraint
> isn't
> preserved -- it replays fine, only the child's name is regenerated.
> Just the
> parent constraint is emitted:
>
> CREATE TABLE ep (a int4range, b int4range) PARTITION BY LIST (a);
> CREATE TABLE ep1 (a int4range, b int4range,
> CONSTRAINT ep1_excl EXCLUDE USING gist (a WITH
> =, b WITH &&));
> ALTER TABLE ep ADD CONSTRAINT ep_excl EXCLUDE USING gist (a WITH
> =, b WITH &&);
> ALTER TABLE ep ATTACH PARTITION ep1 FOR VALUES IN ('[0,1)');
>
> SELECT d FROM pg_get_table_ddl('ep', owner => false) d;
> -- CREATE TABLE public.ep (a int4range, b int4range) PARTITION
> BY LIST (a);
> -- CREATE TABLE public.ep1 PARTITION OF public.ep FOR VALUES IN
> ('[0,1)');
> -- ALTER TABLE public.ep ADD CONSTRAINT ep_excl EXCLUDE USING
> gist (a WITH =, b WITH &&);
>
> ep1's own constraint (ep1_excl) is never emitted, so on replay the
> parent's
> ADD CONSTRAINT recreates it auto-named ep1_a_b_excl instead of
> ep1_excl.
>
> So the substantive round-trip gaps come down to the reordered columns (your
> deferred limitation) and that one EXCLUDE naming case; none breaks replay.
> (1b,
> the conislocal locality case from my v19 pass -- a hand-built one, not in
> the
> corpus above -- is the same family and still open, as you'd noted.)
>
> B) only_kinds/except_kinds filtering. The cross-kind dependency check is
> only
> half there. When a statement leans on an object another kind emits -- a
> constraint's backing index -- and you filter that kind out, REPLICA
> IDENTITY
> USING INDEX catches it and errors cleanly ('requires kind "primary_key" to
> be
> emitted'); CLUSTER ON and a self-referential FK don't, and dangle:
>
> CREATE TABLE b (x int);
> ALTER TABLE b ADD CONSTRAINT b_pk PRIMARY KEY (x);
> ALTER TABLE b CLUSTER ON b_pk;
>
> SELECT d FROM pg_get_table_ddl('b', owner => false,
> except_kinds => ARRAY['primary_key']) d;
> -- CREATE TABLE public.b (x integer NOT NULL);
> -- ALTER TABLE public.b CLUSTER ON b_pk;
> -- replay: ERROR: index "b_pk" for table "b" does not exist
>
> CREATE TABLE t (id int PRIMARY KEY, parent_id int REFERENCES t(id));
>
> SELECT d FROM pg_get_table_ddl('t', owner => false,
> except_kinds => ARRAY['primary_key']) d;
> -- CREATE TABLE public.t (id integer NOT NULL, parent_id integer);
> -- ALTER TABLE public.t ADD CONSTRAINT t_parent_id_fkey
> -- FOREIGN KEY (parent_id) REFERENCES public.t(id);
> -- replay: ERROR: there is no unique constraint matching given keys
> for
> -- referenced table "t"
>
> except_kinds => ARRAY['unique'] does the same. Both want the guard REPLICA
> IDENTITY already has -- the regression test even covers it for all three
> index
> kinds; CLUSTER ON and the FK case just aren't wired to it. A plain-index
> CLUSTER ON and a cross-table FK are fine as they are -- the first drops
> with the
> index kind, the second gets its key from the referenced table's own output.
>
> C) One small code note. emit_partition_children() recurses into
> pg_get_table_ddl_internal() once per partition-tree level:
>
> children = find_inheritance_children(ctx->relid, AccessShareLock);
> foreach(lc, children)
> {
> ...
> CHECK_FOR_INTERRUPTS();
> ...
> childstmts = pg_get_table_ddl_internal(&childctx);
>
> For user-triggerable recursion like this the convention is to put a
> check_stack_depth() next to that CHECK_FOR_INTERRUPTS(). It does fail
> cleanly
> today -- I threw a 12000-level partition tree at it and got the usual
> "stack
> depth limit exceeded" -- but only because some callee happens to check; the
> recursion itself never does.
>
> Regards,
> Rui
>