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

Hi Akshay,

v25 applies to master (798bdcae89), builds warning-free, make check
244/244. The three points from my last mail are all in: the doc note now
says the child constraint names are not preserved in either direction, the
schema_qualified => false script replays under search_path = sc with sb.yc
attached to sc.yp and sa.yp untouched, and the cross-schema ATTACH line is
in the expected output.

Round-trip against pg_dump, re-run on v25 and, alongside it, on v22 so
the two are like for like: 508 of the 563 top-level tables in the
regression database come back identical on both, and the same 55 tables
fall outside for the same reasons. 30 are objects the function doesn't
emit by design (triggers, grants, policies, comments, an owned sequence),
14 are the copy's DROP ... CASCADE taking a serial's sequence or a
function with it, and the remaining 11 are the partition child name and
column-order cases already deferred, three name collisions in the copy,
and one I hadn't looked at before:

1. An unlogged table's identity sequence that was SET LOGGED comes back
unlogged:

CREATE UNLOGGED TABLE idu (a int GENERATED ALWAYS AS IDENTITY);
ALTER SEQUENCE idu_a_seq SET LOGGED;
SELECT d FROM pg_get_table_ddl('idu', owner => false) d;
-- CREATE UNLOGGED TABLE public.idu (a integer GENERATED ALWAYS AS
-- IDENTITY NOT NULL);

-- replayed: idu_a_seq has relpersistence = u; the source has p

emit_identity_sequence_alterations() emits ALTER SEQUENCE ... SET UNLOGGED
when the sequence is unlogged (ddlutils.c:2990). The sequence starts out
with the table's persistence, so the thing to test against is the table,
not LOGGED: on an unlogged table the sequence is unlogged by default and
the change worth emitting is the opposite one. This is the mirror of
Zsolt's 11b, and the regression database has it (identity_dump_unlogged).
Attached 0001 compares against the table and emits SET LOGGED or SET
UNLOGGED accordingly, with the mirror test next to 11b. With it the
round-trip is 509 of 563 and make check stays 244/244.

Two notes on the changes that aren't from my patches.

2. Switching the inline CHECKs to pg_get_constraintdef_body() also drops
PRETTYFLAG_INDENT, so a CHECK with a CASE no longer brings the deparser's
newlines into an otherwise single-line CREATE TABLE. Good, and the
inline path now matches the table-level one; just noting it since the
change is visible and nothing in the test exercises it.

3. The ReleaseSysCache() calls added in front of ereport(ERROR) and
aclcheck_error() aren't needed. The pin is registered with the resource
owner when the tuple is fetched, and the abort path releases it
(ResOwnerReleaseCatCache), which is what every other ereport(ERROR) with
a syscache tuple in hand relies on. There are seven of these in
ddlutils.c now; harmless, just dead code.

From my side this is the last round. With 0001 in, every table in the
regression database is accounted for, and v22 through v25 haven't moved
on that corpus in either direction. I have no open items; I'll re-run
the round-trip on later versions only to check for regressions.

Regards,
Rui