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

Hi Akshay,

Thanks -- v23 builds clean here and installcheck is green (246/246). All three
from my last mail check out: CLUSTER ON no longer dangles on the case I posted,
the self-referential FK errors with the same shape as the REPLICA IDENTITY
guard, and check_stack_depth() is in the right place in emit_partition_children.

Five things after going over the new code.

1) The FK guard misses index-backed keys. fk_backing_constraint_is_blocked()
scans pg_constraint for a PK/UNIQUE on confrelid, but a foreign key only needs a
unique *index* over the referenced columns -- there needn't be a constraint at
all:

CREATE TABLE u (id int, parent_id int);
CREATE UNIQUE INDEX u_id_uidx ON u (id);
ALTER TABLE u ADD CONSTRAINT u_fk FOREIGN KEY (parent_id) REFERENCES u (id);

SELECT d FROM pg_get_table_ddl('u', owner => false,
except_kinds => ARRAY['index']) d;
-- CREATE TABLE public.u (id integer, parent_id integer);
-- ALTER TABLE public.u ADD CONSTRAINT u_fk FOREIGN KEY (parent_id)
-- REFERENCES public.u(id);
-- replay: ERROR: there is no unique constraint matching given keys for
-- referenced table "u"

The scan finds no candidate, concludes "not blocked", and we are back to the
dangling case. Worth noting REPLICA IDENTITY does catch this same shape -- a
table whose replica-identity index is a plain unique index gives

ERROR: REPLICA IDENTITY for table "ri" requires kind "index" to be emitted

pg_constraint.conindid on the FK row points straight at the backing index in
both shapes -- u_id_uidx above, and the PK's own index for the constraint-backed
case from my last mail -- so feeding that to the index_ddl_kind() you just added
for CLUSTER ON covers both, and the matching scan can go:

- if (con->confrelid == ctx->relid)
+ if (con->confrelid == ctx->relid &&
+ OidIsValid(con->conindid))
{
- TableDdlKind ref_kind;
+ TableDdlKind ref_kind = index_ddl_kind(con->conindid);

- if (fk_backing_constraint_is_blocked(conTup, conRel,
- ctx, &ref_kind))
+ if (!is_kind_included(ctx, ref_kind))
{
@@
- errdetail("The key references columns constrained by a \"%s\"
constraint, which is not in the active filter.",
+ errdetail("The key references an index produced by the \"%s\" kind,
which is not in the active filter.",

with fk_backing_constraint_is_blocked() and its forward declaration deleted --
net -122 lines. The errdetail moves because "constrained by an \"index\"
constraint" doesn't read right for the plain-index case; that wording now
matches the REPLICA IDENTITY guard, and the same edit applies to three lines in
pg_get_table_ddl.out. On top of v23 that gives:

self-FK on a plain unique index, except index -> errors (silent before)
self-FK on a PK / UNIQUE, that kind filtered -> still errors
cross-table FK, backing kind filtered -> still no error
nothing filtered -> unchanged
installcheck -> 246/246

2) CLUSTER ON is dropped even when its backing kind is emitted.
emit_cluster_on() still opens with

if (!is_kind_included(ctx, TABLE_DDL_KIND_INDEX))
return;

which predates the new per-index check below it, and for a constraint-backed
clustered index the two now disagree:

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,
only_kinds => ARRAY['table','primary_key']) d;
-- CREATE TABLE public.b (x integer NOT NULL);
-- ALTER TABLE public.b ADD CONSTRAINT b_pk PRIMARY KEY (x);

The PK is emitted, so the index exists on replay and CLUSTER ON would have
applied cleanly -- I appended it by hand to check -- but the early return drops
it because "index" isn't in the filter. except_kinds => ARRAY['index'] behaves
the same. With index_ddl_kind() in place that outer test is redundant anyway,
since it returns TABLE_DDL_KIND_INDEX for a plain index, so dropping it is
enough:

List *indexoids;
ListCell *lc;

- if (!is_kind_included(ctx, TABLE_DDL_KIND_INDEX))
- return;
-
indexoids = RelationGetIndexList(ctx->rel);

With that, a PK- or UNIQUE-backed CLUSTER ON survives only_kinds =>
ARRAY['table','primary_key'] and except_kinds => ARRAY['index'], while the
plain-index case and the filtered-backing-kind cases behave as they do now;
installcheck stays at 246/246, including your new CLUSTER ON tests.

3) On the deferred naming case -- two notes, then it's your call on scope.

It isn't only EXCLUDE. A custom-named PK or UNIQUE on a partition child goes the
same way, since they all sit behind the same conislocal filter:

CREATE TABLE pk_p (a int, b int) PARTITION BY LIST (a);
CREATE TABLE pk_c (a int, b int, CONSTRAINT pk_c_mypk PRIMARY KEY (a,b));
ALTER TABLE pk_p ADD CONSTRAINT pk_p_mypk PRIMARY KEY (a,b);
ALTER TABLE pk_p ATTACH PARTITION pk_c FOR VALUES IN (1);

round-trips as pk_c_mypk -> pk_c_pkey, and the UNIQUE equivalent as
uq_c_myuq -> uq_c_a_b_key.

On the cost: I think it splits in two, and ChooseConstraintName is only needed
for one half.

When the whole tree is emitted -- pg_get_table_ddl on the parent -- the child's
name is still in pg_constraint (ATTACH only clears conislocal and bumps
coninhcount), so nothing has to be synthesised. It is the shape emit_indexes
already produces for a plain partitioned index:

CREATE INDEX ic_myidx ON public.ic USING btree (b);
CREATE INDEX ip_myidx ON ONLY public.ip USING btree (b);
ALTER INDEX public.ip_myidx ATTACH PARTITION ic_myidx;

child first, parent ON ONLY, then ATTACH (that child name is unqualified per
(4), but these are all in one schema). The same works for constraints, and
the ordering already holds because emit_partition_children runs before
emit_local_constraints. I hacked it up here to check: let the conislocal filter
through for a partition child's PK/UNIQUE/EXCLUSION, put ONLY on both sides, and
reuse your ALTER INDEX ... ATTACH PARTITION block. ep1_excl, pk_c_mypk and
uq_c_myuq then come back with conislocal = false and coninhcount = 1, matching
the source, with installcheck still green.

The other half is where your concern lands, and I was wrong to wave it off.
Called on the child alone, the parent must already exist with its constraint, so
CREATE TABLE ... PARTITION OF creates the child's automatically and an explicit
ADD CONSTRAINT collides:

ERROR: multiple primary keys for table "parted_pk_a" are not allowed

which is what your own regression test gave me before I gated it. Renaming after
the fact does need the generated name, so that path is the fragile one, and
telling the two apart needs a flag on the context.

If the tree case is worth having on its own, it's the one round-trip fidelity
depends on; the standalone-child call could stay a documented limitation.

4) Unrelated, but it turned up while comparing against pg_dump:
emit_indexes() qualifies the parent in ALTER INDEX ... ATTACH PARTITION but not
the child, so a partition child in another schema doesn't replay:

CREATE SCHEMA sa; CREATE SCHEMA sb;
CREATE TABLE sa.yp (a int, b int) PARTITION BY LIST (a);
CREATE TABLE sb.yc (a int, b int);
CREATE INDEX yc_myidx ON sb.yc (b);
CREATE INDEX yp_myidx ON sa.yp (b);
ALTER TABLE sa.yp ATTACH PARTITION sb.yc FOR VALUES IN (1);

SELECT d FROM pg_get_table_ddl('sa.yp', owner => false) d;
-- CREATE TABLE sa.yp (a integer, b integer) PARTITION BY LIST (a);
-- CREATE TABLE sb.yc PARTITION OF sa.yp FOR VALUES IN (1);
-- CREATE INDEX yc_myidx ON sb.yc USING btree (b);
-- CREATE INDEX yp_myidx ON ONLY sa.yp USING btree (b);
-- ALTER INDEX sa.yp_myidx ATTACH PARTITION yc_myidx;
-- replay: ERROR: relation "yc_myidx" does not exist

schema_qualified doesn't help, since the child name is never qualified either
way:

SELECT d FROM pg_get_table_ddl('sa.yp', owner => false,
schema_qualified => true) d;
-- ALTER INDEX sa.yp_myidx ATTACH PARTITION yc_myidx;

Same-schema cases only work because search_path happens to find the child.
pg_dump writes both sides qualified -- ALTER INDEX sa.yp_myidx ATTACH PARTITION
sb.yc_myidx; -- and running the child name through lookup_relname_for_emit(),
as you already do for the parent, is enough:

foreach(clc, childIdxOids)
{
Oid childIdxOid = lfirst_oid(clc);
- char *childIdxName = get_rel_name(childIdxOid);
+ char *childIdxName = lookup_relname_for_emit(childIdxOid,
+ ctx->schema_qualified,
+ ctx->base_namespace);
@@
parentIdxName,
- quote_identifier(childIdxName));
+ childIdxName);

With that the cross-schema case replays, schema_qualified => false still leaves
a same-schema child unqualified, and in installcheck it only moves six existing
ATTACH lines to the qualified form.

5) Two limitations aren't in the documented exclusion list yet. The list
in func-info.sgml has owned sequences, partition children whose column order
differs from the parent, triggers/policies, and COMMENT/GRANT, but not:

- security labels, which you said back on v21 should be noted as a known
limitation -- there's no mention of them in the docs as of v23;
- the partition-child constraint naming above. Its sibling, the column
reordering, is documented, so a reader would expect to find this one there
too once it's deferred.

Regards,
Rui