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

Hi Akshay,

Thanks for v19. I re-ran the pg_dump round-trip check (the 002_pg_dump.pl
scenarios plus the whole regression database) on current master (2e6578292a).
The five fixes hold up: SET STORAGE/COMPRESSION now use ONLY, invalid indexes
are skipped, SET STATISTICS / CLUSTER ON / DISABLE RULE are emitted, and the
child-default and named-NOT NULL cases I sent all round-trip now. 494 of 562
tables match pg_dump exactly (was 483/560 on v17). Three things came out of
the run below; the third is a side effect of that invalid-index skip.

1) The named-NOT NULL handling for inheritance/partition children still isn't
complete. It works when the child constraint has a user-defined name, but the
corpus turns up two ways it still goes wrong.

1a) For an auto-named inherited NOT NULL it emits an out-of-line ADD CONSTRAINT
that collides with the one INHERITS/PARTITION OF already propagates, giving
non-replayable DDL:

CREATE TABLE nt (a int PRIMARY KEY);
CREATE TABLE ntc (PRIMARY KEY (a) DEFERRABLE) INHERITS (nt);

SELECT d FROM pg_get_table_ddl('ntc'::regclass, owner => false) d;
-- CREATE TABLE public.ntc () INHERITS (public.nt);
-- ALTER TABLE public.ntc ADD CONSTRAINT ntc_a_not_null NOT NULL a;
-- ALTER TABLE public.ntc ADD CONSTRAINT ntc_pkey PRIMARY KEY (a)
DEFERRABLE;

-- replaying that, with nt already restored first so INHERITS propagates
-- its nt_a_not_null down to ntc:
ERROR: cannot create not-null constraint "ntc_a_not_null" on
column "a" of table "ntc"
DETAIL: A not-null constraint named "nt_a_not_null" already
exists for this column.

(notnull_tbl4_cld2 in the regression suite fails the same way.)

1b) Even where it does replay, a partition attached from a standalone table
keeps its name but comes back with the wrong constraint locality, because the
constraint is folded into PARTITION OF instead of a standalone CREATE + ATTACH:

CREATE TABLE mp (a int NOT NULL) PARTITION BY LIST (a);
CREATE TABLE mp1 (a int CONSTRAINT mp1_nn NOT NULL);
ALTER TABLE mp ATTACH PARTITION mp1 FOR VALUES IN (1);
-- source mp1.mp1_nn: conislocal = false (the ATTACH merged it
into the parent's)

SELECT d FROM pg_get_table_ddl('mp'::regclass, owner => false) d;
-- CREATE TABLE public.mp (a integer NOT NULL) PARTITION BY LIST (a);
-- CREATE TABLE public.mp1 PARTITION OF public.mp (CONSTRAINT
mp1_nn NOT NULL a) FOR VALUES IN (1);

Declaring the constraint inline makes it local, so mp1.mp1_nn comes back with
conislocal = true, and the behavior diverges once the parent's NOT NULL is
dropped:

ALTER TABLE mp ALTER a DROP NOT NULL;
-- source: mp1.a is no longer NOT NULL; reconstructed: mp1.a stays NOT NULL

pg_dump reconstructs both faithfully. For reference, this is where it puts
each constraint:

- In the CREATE TABLE body: NOT NULL, and validated CHECK.
- As a separate ALTER TABLE ... ADD CONSTRAINT: PRIMARY KEY, UNIQUE, EXCLUDE,
FOREIGN KEY, and NOT VALID CHECK.

The split is the same on plain tables, inheritance children and partitions;
only the table shape differs:

- A plain-inheritance child keeps CREATE ... INHERITS and emits only its
locally-owned constraints (purely-inherited NOT NULL/CHECK are left to
INHERITS); the NOT NULL goes in the body unnamed so it auto-names, e.g.
for the 1a case: CREATE TABLE ntc (NOT NULL a) INHERITS (nt); ...

- A partition is never dumped with CREATE ... PARTITION OF; it is a standalone
CREATE TABLE plus ALTER TABLE ONLY parent ATTACH PARTITION, so the child's
column order, constraint names and conislocal come from its own definition
plus the merge ATTACH performs.

That partition path is the same standalone+ATTACH that the reordered-column
case you deferred needs, so 1b likely folds into that follow-up. I'll
leave the exact approach to you.

2) A NOT VALID CHECK constraint loses its NOT VALID flag on round-trip. The
constraint is emitted inline in CREATE TABLE, where it is validated against the
(empty) table, so the reconstructed constraint is marked validated:

CREATE TABLE nv (a int);
ALTER TABLE nv ADD CONSTRAINT chk CHECK (a < 50) NOT VALID;

SELECT d FROM pg_get_table_ddl('nv'::regclass, owner => false) d;
-- CREATE TABLE public.nv (a integer, CONSTRAINT chk CHECK ((a <
50)) NOT VALID);

-- source nv.chk: convalidated = false
-- reconstructed nv.chk: convalidated = true

pg_dump emits NOT VALID constraints as a separate ALTER TABLE ... ADD
CONSTRAINT ... NOT VALID for this reason.

3) The invalid-index skip added for the earlier finding also drops legitimate
partitioned indexes. emit_indexes now has:

/* Skip invalid indexes; they may be left over from a failed
CREATE INDEX CONCURRENTLY. */
if (!idxform->indisvalid)
{
ReleaseSysCache(indTup);
continue;
}

but a partitioned index on the parent is normally indisvalid = false (e.g. one
created with ON ONLY, or before all child indexes are attached), so the whole
index is lost:

CREATE TABLE p (a int) PARTITION BY RANGE (a);
CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (0) TO (100);
CREATE INDEX ON ONLY p (a); -- p_a_idx.indisvalid = false

SELECT d FROM pg_get_table_ddl('p'::regclass, owner => false) d;
-- CREATE TABLE public.p (a integer) PARTITION BY RANGE (a);
-- CREATE TABLE public.p1 PARTITION OF public.p FOR VALUES FROM
(0) TO (100);

No CREATE INDEX at all; pg_dump keeps it as "CREATE INDEX p_a_idx ON ONLY
public.p USING btree (a)". That's the pg_dump condition I quoted earlier --
"i.indisvalid OR t2.relkind = 'p'"; the relkind = 'p' half needs to stay so
partitioned indexes aren't filtered out along with the failed-CIC ones.

Thanks,
Rui