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

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

Hello.

At Thu, 11 Jun 2026 13:18:07 +0530, Akshay Joshi  wrote in 
> Fixed the issue above. The v5 patch is ready for review/testing.

I have not looked at the patch in detail, but I noticed that some
comments in the patch seem to contain non-ASCII characters.

> * * re-emitting it as ALTER TABLE … ADD CONSTRAINT ― the column-emit

I don't think that is recommended in PostgreSQL source comments, so
these should probably be replaced with plain ASCII equivalents.

https://www.postgresql.org/message-id/E1pnhhu-003D6z-Ki%40gemulon.postgresql.org

For reference, I have attached the result of a quick search below.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


Quick search result:

=========
20 matches in 18 lines for "[^[:ascii:]]" in buffer: v5-0001-Add-pg_get_table_ddl-to-reconstruct-CREATE-TABLE.patch
    565:+ * re-emitting it as ALTER TABLE … ADD CONSTRAINT ― the column-emit
    877:+				 * defaults ― mirroring pg_get_database_ddl's pattern of
    934:+						 * SEQUENCE NAME ― omit when it matches the implicit
   1042:+	 * Table-level CHECK constraints ― emitted inline in the CREATE TABLE
   1055:+ *		applied per-column overrides ― DEFAULT, NOT NULL, and any locally
   1155:+ * pg_get_ruledef, pg_get_statisticsobjdef_string) ― when
   1383:+ *		ALTER TABLE qualname ALTER COLUMN col SET DEFAULT expr ― one per
   1422:+ *		ALTER TABLE qualname ALTER COLUMN col SET (...) ― one per column
   1468:+ *		out-of-line by emit_local_constraints (the ALTER TABLE … ADD
   1500:+ *		ALTER TABLE … ADD CONSTRAINT for each locally-defined constraint
   1617:+ *		ALTER TABLE qualname REPLICA IDENTITY … ― emitted only when the
   1749:+ * (#if 0) ― they will become a single helper call once the standalone
   1830:+	 * Pre-compute "." too ― the always-qualified helpers
   1884:+	 * Triggers and row-level security policies ― disabled until the
   2680:+-- quoting (its prefix starts with ").  Both forms of the prefix ―
   2681:+-- bare-lowercase and quoted ― must be stripped from outer
   3197:+-- quoting (its prefix starts with ").  Both forms of the prefix ―
   3198:+-- bare-lowercase and quoted ― must be stripped from outer
=========

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

At Mon, 15 Jun 2026 14:25:06 +0530, Akshay Joshi  wrote in 
> Hi Kyotaro and Zsolt,
> 
> I have incorporated the feedback provided by both of you.
> The v6 patch is updated and ready for your review.
> 
> On Fri, Jun 12, 2026 at 6:40 AM Kyotaro Horiguchi 
> wrote:
> >
> > I have not looked at the patch in detail, but I noticed that some
> > comments in the patch seem to contain non-ASCII characters.

Thanks for the update.

That issue appears to be resolved in this version.

Regards,
-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

At Mon, 22 Jun 2026 18:10:53 +0530, Akshay Joshi  wrote in 
> The v9 patch is ready for review.

I have not looked closely at the DDL generation logic itself, but I
have a few comments on how pg_get_table_ddl handles its options.

Since pg_get_table_ddl_internal() appears to copy these values into
TableDdlContext almost immediately, I wonder whether TableDdlContext
could be initialized by the caller instead.

Using positional boolean arguments is probably fine when there are
only a handful of options, but with around fifteen of them the current
approach seems somewhat error-prone.

It might also be clearer to initialize the default values first, and
then override only the fields corresponding to explicitly specified
options, rather than folding the default handling and option lookup
into the same expression.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

Hello,

Looking at this, one thing that concerns me is the large amount of
overlap with dumpTableSchema() in pg_dump.

I wonder if it would make sense to separate the SQL generation logic
into frontend/backend-shared code so that it could also be used by
pg_dump. The catalog lookup would naturally remain separate, but
sharing the DDL generation itself would significantly reduce the
duplication.

By the way, a couple of comments use a Unicode RIGHTWARDS ARROW
(U+2192). Please use an ASCII equivalent instead.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

> On Jun 22, 2026, at 22:11, Akshay Joshi  wrote:
> 
> 
> 
> On Mon, Jun 22, 2026 at 7:30 PM Chao Li  wrote:
> 
> 
> > On Jun 22, 2026, at 20:40, Akshay Joshi  wrote:
> > 
> > You're right, and thanks for spotting this. The existing pattern in pg_proc.dat for variadic-text functions (e.g., jsonb_delete, json_extract_path) uses _text at the variadic position in both proargtypes and proallargtypes, with provariadic => 'text'. That is the convention documented by the sanity check in src/test/regress/sql/opr_sanity.sql.
> > 
> > The same issue applies to pg_get_role_ddl, pg_get_tablespace_ddl (both variants), and pg_get_database_ddl, but that will require a separate patch.
> > 
> 
> Thanks for confirming. Then I will file a patch tomorrow to fix those.
> 
>    I started working on it, but if you want to take the lead, just let me know and I won't send my version over 
> 

The changes to pg_proc.dat have been in my local tree for some time. Today I also fixed the sanity check in opr_sanity so that it can now report this mismatch. Could you please take a look at my patch [1]?

[1] https://www.postgresql.org/message-id/D41A334E-ED9E-42EE-830D-28D4D36E9317%40gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

> On Jun 22, 2026, at 14:26, Akshay Joshi  wrote:
> 
> Thanks for the review; you're right, `includes_foreign_keys=false` on its own is a half-measure. Re-running with the default to add FKs back collides with the existing CREATE TABLE, UNIQUE indexes, etc.
> 
> I've added an only_foreign_keys option (boolean, default false) as the natural complement of includes_foreign_keys=false. When set to true, the function emits only the ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY statements and suppresses everything else (CREATE TABLE, owner, indexes, non-FK constraints, rules, statistics, replica identity, RLS toggles). Partition-child recursion still runs so child FKs are reached too. Combining `only_foreign_keys=true` with `includes_foreign_keys=false` is rejected upfront since it would produce no output.
> 
> The documentation paragraph for `includes_foreign_keys` now directs users to `only_foreign_keys` as the intended second pass. Regression coverage adds three cases: the FK-only emission for your cons example, the zero-row result for a table without FKs, and the error path. 
> 
> The v8 patch is ready for review.
> 
> On Sat, Jun 20, 2026 at 1:15 AM Zsolt Parragi  wrote:
> The previous features all look good to me, I only have one question
> for the new flag.
> 
> > Calling
> > pg_get_table_ddl(t, 'includes_foreign_keys', 'false') now emits everything
> > except FOREIGN KEY constraints. This covers the multi-tenant clone
> > workflow: create tables first without cross-table references, then re-run
> > with the default to add the constraints once all targets exist.
> 
> I think this feature needs a bit more documentation, an
> "only_foreign_keys" flag, or both.
> 
> CREATE TABLE refd (id int PRIMARY KEY);
> CREATE TABLE cons (a int CHECK(a>0), b int UNIQUE, c int REFERENCES refd(id));
> 
> -- pass 1: running without foreign keys
> SELECT * FROM pg_get_table_ddl('cons','includes_foreign_keys','false');
> -- execute everything
> 
> -- loading data
> 
> -- pass 2: running with everything
> SELECT * FROM pg_get_table_ddl('cons','includes_foreign_keys','true');
> -- ERROR: relation "cons" already exists (and the unique constraint
> also collides)
> 
> I could do a "grep FOREIGN KEY" before executing (unless it's a tricky
> schema where that phrase appears elsewhere), or since psql continues
> on error, it will simply work if I accept a significant error noise,
> but then the documentation should be clear about this limitation.
> Following the documented approach and getting a bunch of unexpected
> errors could be confusing for users.
> 
> 
> 

I have a comment, or maybe a question:
```
+{ oid => '8215', descr => 'get DDL to recreate a table',
+  proname => 'pg_get_table_ddl', prorows => '50', provariadic => 'text',
+  proisstrict => 'f', proretset => 't', provolatile => 's', proparallel => 'r',
+  pronargdefaults => '1', prorettype => 'text',
+  proargtypes => 'regclass text', proallargtypes => '{regclass,text}',
+  proargmodes => '{i,v}', proargdefaults => '{NULL}',
+  prosrc => 'pg_get_table_ddl' },
```

Since provariadic is text, I wonder if proallargtypes should be {regclass,_text}, with _text meaning an array of text.

I’m asking because I have had this suspicion for some time. I saw a few other procs using the same pattern, for example:
```
{ oid => '6501', descr => 'get DDL to recreate a role',
  proname => 'pg_get_role_ddl', prorows => '10', provariadic => 'text',
  proisstrict => 'f', proretset => 't', provolatile => 's',
  pronargdefaults => '1', prorettype => 'text', proargtypes => 'regrole text',
  proallargtypes => '{regrole,text}', proargmodes => '{i,v}',
  proargdefaults => '{NULL}', prosrc => 'pg_get_role_ddl' },
```

But for jsonb_delete etc procs, _text is used:
```
{ oid => '3343',
  proname => 'jsonb_delete', provariadic => 'text', prorettype => 'jsonb',
  proargtypes => 'jsonb _text', proallargtypes => '{jsonb,_text}',
  proargmodes => '{i,v}', proargnames => '{from_json,path_elems}',
  prosrc => 'jsonb_delete_array' },
```

So I wonder whether “text” rather than “_text" is intentionally used in proallargtypes, or if this was just never noticed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

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

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

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

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

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

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

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

Hi Akshay,

Re-tested v16 on current master -- builds clean and the recent fixes hold up
(self-ref FK after PK, no duplicate partition-child index, cross-schema
partition child qualified, schema_qualified => true consistent). A few
issues, in severity order; several are places where pg_dump already does the
right thing.

1. Clause order: TABLESPACE is emitted before ON COMMIT (ddlutils.c:2042 vs
2054), but the grammar is "... OptWith OnCommitOption OptTableSpace" -- ON
COMMIT must come first, so a temp table with both clauses is non-replayable:

    CREATE TABLESPACE ts1 LOCATION '/path/to/dir';
    CREATE TEMP TABLE tt (a int) ON COMMIT DROP TABLESPACE ts1;
    SELECT d FROM pg_get_table_ddl('tt'::regclass, owner => false) d;
    --  CREATE TEMPORARY TABLE tt (a integer) TABLESPACE ts1 ON COMMIT DROP;

    CREATE TEMPORARY TABLE tt (a integer) TABLESPACE ts1 ON COMMIT DROP;
    --  ERROR:  syntax error at or near "ON"

Swapping the two blocks so ON COMMIT precedes TABLESPACE fixes it.

2. Child-default override recurses (missing ONLY). emit_child_default_overrides
emits the inherited-column default without ONLY (ddlutils.c:2118), so SET
DEFAULT recurses into the table's children and reconstructing one table
silently rewrites another:

    CREATE TABLE dpar (x int);
    CREATE TABLE dch  () INHERITS (dpar);
    CREATE TABLE dgc  () INHERITS (dch);
    ALTER TABLE ONLY dch ALTER COLUMN x SET DEFAULT 5;
    ALTER TABLE ONLY dgc ALTER COLUMN x SET DEFAULT 10;

    SELECT d FROM pg_get_table_ddl('dch'::regclass, owner => false) d;
    --  ALTER TABLE dch ALTER COLUMN x SET DEFAULT 5;      -- no ONLY

    ALTER TABLE dch ALTER COLUMN x SET DEFAULT 5;          -- replay this line
    --  => dgc's default is now 5, not 10

pg_dump uses ALTER TABLE ONLY here for exactly this reason.

Partitioned tables hit this especially easily -- any partitioned table with a
column default emits a redundant, ONLY-less SET DEFAULT for every partition
that merely inherits it:

    CREATE TABLE p (id int, amt int DEFAULT 5) PARTITION BY LIST (id);
    CREATE TABLE p_a PARTITION OF p FOR VALUES IN (1);

    SELECT d FROM pg_get_table_ddl('p'::regclass, owner => false) d;
    --  CREATE TABLE public.p (id integer, amt integer DEFAULT 5)
PARTITION BY LIST (id);
    --  CREATE TABLE public.p_a PARTITION OF public.p FOR VALUES IN (1);
    --  ALTER TABLE public.p_a ALTER COLUMN amt SET DEFAULT 5;   --
redundant, no ONLY

p_a already inherits amt's default from the PARTITION OF, so the third line is
redundant; pg_dump instead keeps the default inline on the child and attaches
with ALTER TABLE ONLY ... ATTACH PARTITION. A full-hierarchy replay
self-corrects (each child's own SET DEFAULT runs last), but a partial replay or
a lone emitted statement does not. The commit message lists "child-local
DEFAULT overrides on inheritance/partition children" as supported, so this is
in scope. (More generally the patch never emits ONLY anywhere; ADD CONSTRAINT,
where CHECK / NOT NULL also recurse to children, is worth the same audit.)

3. Inherited-only NOT NULL emitted as local. When a child redeclares an
inherited column (attislocal) without restating NOT NULL, the constraint is
inherited-only (conislocal = false); collect_local_not_null skips it, but
append_column_defs keys off att->attnotnull and emits a bare NOT NULL anyway:

    CREATE TABLE par (a int NOT NULL);
    CREATE TABLE chld (a int) INHERITS (par);   -- 'a' redeclared, no NOT NULL

    SELECT d FROM pg_get_table_ddl('chld'::regclass, owner => false) d;
    --  CREATE TABLE public.chld (a integer NOT NULL) INHERITS (public.par);

On replay the child now owns the constraint (conislocal flips false -> true,
name regenerates par_a_not_null -> chld_a_not_null), so a later
"ALTER TABLE par ALTER a DROP NOT NULL" cascades to the original child but not
the reconstructed one. pg_dump emits "a integer" with no NOT NULL here,
suppressing it via notnull_islocal (pg_dump.c ~9916). The docs describe this as
the intended behavior -- "Inherited columns and constraints ... are not
duplicated on inheritance children or partitions" -- so it reads as a
documented contract the code doesn't quite meet. The inline CHECK path in this
patch already filters on conislocal; the NOT NULL path could do the same.

4. Typed-table STORAGE / COMPRESSION not emitted -- intended? append_column_defs
emits per-column STORAGE for ordinary tables, but the typed-table path
(append_typed_column_overrides) only handles DEFAULT / NOT NULL / CHECK, so a
storage override on a typed table is not reproduced:

    CREATE TYPE mytype AS (a int, b text);
    CREATE TABLE typed_t OF mytype;
    ALTER TABLE typed_t ALTER COLUMN b SET STORAGE external;

    SELECT d FROM pg_get_table_ddl('typed_t'::regclass, owner => false) d;
    --  CREATE TABLE public.typed_t OF public.mytype;       -- STORAGE
not emitted

The docs scope the typed-table form to overrides for "defaults, NOT NULL, and
CHECK", so this may well be deliberate. But STORAGE is listed in the general
per-column coverage, and pg_dump does emit it (ALTER TABLE ONLY ... ALTER COLUMN
b SET STORAGE EXTERNAL) -- so it seems worth confirming the omission is
intentional rather than an oversight.

5. Minor: is_auto (ddlutils.c:1381) and the identity SEQUENCE NAME check (1681)
rebuild the expected auto-name with snprintf, but the backend uses
makeObjectName(), which truncates name1/name2 to fit NAMEDATALEN and never the
label -- so for long names the "_not_null" / "_seq" suffix is dropped and the
checks misfire, emitting a name a short-named table would omit:

    CREATE TABLE aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        (bbbbbbbbbbbbbbbbbbbb int GENERATED ALWAYS AS IDENTITY);

    SELECT d FROM pg_get_table_ddl(
        'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'::regclass,
        owner => false) d;
    --  CREATE TABLE public.aaaa...(50) (bbbb...(20) integer
    --      GENERATED ALWAYS AS IDENTITY (SEQUENCE NAME public.aaaa..._seq)
    --      CONSTRAINT aaaa..._not_null NOT NULL);

The default-omission convention in the commit message lists "the auto-generated
identity sequence name" among the clauses meant to be dropped, which is exactly
what misfires here for long names. It still replays (the names are real), so
this one is cosmetic; comparing against makeObjectName(relname, colname,
"not_null" / "seq") makes both sides agree.

Everything else looks good.

Thanks,
Rui

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

Hi Akshay,

Re-tested v17 on current master (73dfe79fd6) -- all five issues are fixed,
and make check passes here.

I then ran a round-trip check against pg_dump: for each table, replay the
pg_get_table_ddl() output into a clone of the database and diff
"pg_dump -t" of both sides (both sides being pg_dump output, any surviving
diff is semantic, not formatting). Corpus: the create_sql scenarios from
002_pg_dump.pl, then the whole regression database. 483 of 560 tables
round-trip identically; setting aside the documented no-output kinds
(trigger/policy), the rest reduce to:

1) serial columns produce non-replayable DDL -- the output references the
sequence but never creates it:

    CREATE TABLE ser (id serial, v text);

    SELECT d FROM pg_get_table_ddl('ser'::regclass, owner => false) d;
    --  CREATE TABLE public.ser (id integer DEFAULT
nextval('public.ser_id_seq'::regclass) NOT NULL, v text);

    CREATE TABLE public.ser (id integer DEFAULT
nextval('public.ser_id_seq'::regclass) NOT NULL, v text);
    --  ERROR:  relation "public.ser_id_seq" does not exist    (on an
empty database)

pg_dump emits CREATE SEQUENCE + OWNED BY + SET DEFAULT. This accounts for
14 regression failures, including whole partition trees.

2) Partition/inheritance children that diverged from their parent don't
survive the PARTITION OF / INHERITS rebuild. Three variants:

  - a dropped default silently comes back:

        CREATE TABLE dp (a int DEFAULT 99) PARTITION BY LIST (a);
        CREATE TABLE dp1 PARTITION OF dp FOR VALUES IN (1);
        ALTER TABLE ONLY dp1 ALTER COLUMN a DROP DEFAULT;

        SELECT d FROM pg_get_table_ddl('dp'::regclass, owner => false) d;
        --  CREATE TABLE public.dp (a integer DEFAULT 99) PARTITION BY LIST (a);
        --  CREATE TABLE public.dp1 PARTITION OF public.dp FOR VALUES IN (1);

        SELECT pg_get_expr(adbin, adrelid) FROM pg_attrdef
          WHERE adrelid = 'dp1'::regclass;
        --  source:   (0 rows)
        --  replayed: 99    => INSERTs into dp1 now get 99, not NULL

  - a child's own NOT NULL constraint name is lost:

        CREATE TABLE np (a int NOT NULL) PARTITION BY LIST (a);
        CREATE TABLE np1 (a int CONSTRAINT np1_nn NOT NULL);
        ALTER TABLE np ATTACH PARTITION np1 FOR VALUES IN (1);

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

        SELECT conname FROM pg_constraint
          WHERE conrelid = 'np1'::regclass AND contype = 'n';
        --  source:   np1_nn
        --  replayed: np_a_not_null

    and when the column itself is inherited, the emitted constraint
    errors instead of merging:

        CREATE TABLE p5 (a int);
        CREATE TABLE c5 () INHERITS (p5);
        ALTER TABLE c5 ADD CONSTRAINT c5_nn NOT NULL a;
        ALTER TABLE p5 ADD CONSTRAINT p5_nn NOT NULL a;

        SELECT d FROM pg_get_table_ddl('c5'::regclass, owner => false) d;
        --  CREATE TABLE public.c5 () INHERITS (public.p5);
        --  ALTER TABLE public.c5 ADD CONSTRAINT c5_nn NOT NULL a;

        ALTER TABLE public.c5 ADD CONSTRAINT c5_nn NOT NULL a;    --
replay, p5 recreated first
        --  ERROR:  cannot create not-null constraint "c5_nn" on
column "a" of table "c5"
        --  DETAIL:  A not-null constraint named "p5_nn" already
exists for this column.

  - a partition attached from a table with different column order is
    rebuilt in the parent's order:

        CREATE TABLE p (a int, b int, c int) PARTITION BY LIST (a);
        CREATE TABLE c1 (c int, b int, a int);
        ALTER TABLE p ATTACH PARTITION c1 FOR VALUES IN (1);

        SELECT d FROM pg_get_table_ddl('p'::regclass, owner => false) d;
        --  CREATE TABLE public.p (a integer, b integer, c integer)
PARTITION BY LIST (a);
        --  CREATE TABLE public.c1 PARTITION OF public.p FOR VALUES IN (1);
        --  => replayed c1 columns are a, b, c (source: c, b, a), so
        --     SELECT * / COPY / positional INSERT all shift

pg_dump handles all three: it emits the named constraint inline in the
child's CREATE body (the merge with the inherited constraint at CREATE
time keeps the local name, and attislocal / conislocal / coninhcount all
survive), and falls back to standalone CREATE + ATTACH PARTITION for
shapes a PARTITION OF / INHERITS clause can't express. The first two
variants have lightweight fixes that keep the PARTITION OF / INHERITS
shape --

    CREATE TABLE c5 (CONSTRAINT c5_nn NOT NULL a) INHERITS (p5);
    CREATE TABLE np1 PARTITION OF np (CONSTRAINT np1_nn NOT NULL a)
FOR VALUES IN (1);

plus a counter-statement for the divergent-default case (ALTER TABLE
ONLY dp1 ALTER COLUMN a DROP DEFAULT). Only the reordered-column case
really needs the standalone shape. The commit message lists child-local
DEFAULT overrides and named NOT NULL constraints as supported, so I'm
treating these as bugs rather than scope cuts.

3) emit_typed_column_storage() (ddlutils.c:2331, 2342) emits ALTER TABLE
without ONLY; SET STORAGE / SET COMPRESSION recurse:

    CREATE TYPE mytype AS (a int, b text);
    CREATE TABLE typed_t OF mytype;
    ALTER TABLE typed_t ALTER COLUMN b SET STORAGE external;
    CREATE TABLE tchild () INHERITS (typed_t);
    ALTER TABLE ONLY tchild ALTER COLUMN b SET STORAGE main;

    ALTER TABLE public.typed_t ALTER COLUMN b SET STORAGE EXTERNAL;
-- replay v17's output
    --  => tchild's attstorage flips m -> e

pg_dump uses ALTER TABLE ONLY here. (The other emitted ALTER TABLEs are
fine: OWNER / REPLICA IDENTITY / RLS / SET (options) don't recurse, and
ADD CONSTRAINT must stay ONLY-less since the dump relies on its recursion
to rebuild the children's suppressed inherited copies.)

4) Some per-table state pg_dump preserves is missing. Index statistics
targets:

    CREATE TABLE ist (c1 int);
    CREATE INDEX ist_idx ON ist ((c1 + 1));
    ALTER INDEX ist_idx ALTER COLUMN 1 SET STATISTICS 400;

    SELECT d FROM pg_get_table_ddl('ist'::regclass, owner => false) d;
    --  CREATE TABLE public.ist (c1 integer);
    --  CREATE INDEX ist_idx ON public.ist USING btree (((c1 + 1)));
 -- no SET STATISTICS

Same story for ALTER TABLE ... CLUSTER ON (the index comes back without
the indisclustered marker) and for ALTER TABLE ... DISABLE RULE (the rule
is emitted but comes back enabled, which changes behavior -- and rule is
a covered kind).

5) emit_indexes doesn't check indisvalid, so an invalid index is emitted
as a normal one:

    CREATE TABLE inv (x int);
    INSERT INTO inv VALUES (1), (1);
    CREATE UNIQUE INDEX CONCURRENTLY inv_uidx ON inv (x);
    --  ERROR:  could not create unique index "inv_uidx"    (leaves
indisvalid = false)

    SELECT d FROM pg_get_table_ddl('inv'::regclass, owner => false) d;
    --  CREATE TABLE public.inv (x integer);
    --  CREATE UNIQUE INDEX inv_uidx ON public.inv USING btree (x);

pg_dump skips those (getIndexes: "i.indisvalid OR t2.relkind = 'p'").

6) Minor: COMMENT ON and GRANT/REVOKE are not emitted. If that's
intentional -- like trigger/policy -- worth saying so in the doc.

Thanks,
Rui

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

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

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

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

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

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

The previous features all look good to me, I only have one question
for the new flag.

> Calling
> pg_get_table_ddl(t, 'includes_foreign_keys', 'false') now emits everything
> except FOREIGN KEY constraints. This covers the multi-tenant clone
> workflow: create tables first without cross-table references, then re-run
> with the default to add the constraints once all targets exist.

I think this feature needs a bit more documentation, an
"only_foreign_keys" flag, or both.

CREATE TABLE refd (id int PRIMARY KEY);
CREATE TABLE cons (a int CHECK(a>0), b int UNIQUE, c int REFERENCES refd(id));

-- pass 1: running without foreign keys
SELECT * FROM pg_get_table_ddl('cons','includes_foreign_keys','false');
-- execute everything

-- loading data

-- pass 2: running with everything
SELECT * FROM pg_get_table_ddl('cons','includes_foreign_keys','true');
-- ERROR: relation "cons" already exists (and the unique constraint
also collides)

I could do a "grep FOREIGN KEY" before executing (unless it's a tricky
schema where that phrase appears elsewhere), or since psql continues
on error, it will simply work if I accept a significant error noise,
but then the documentation should be clear about this limitation.
Following the documented approach and getting a bunch of unexpected
errors could be confusing for users.

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

Thanks for the update! The new version looks mostly good, I only found
one corner case that doesn't work, double quoted literals can still
get over-stripped:

CREATE SCHEMA s;
CREATE TABLE s.p (id int, "s.weird" int) PARTITION BY RANGE (id);
CREATE TABLE s.pc PARTITION OF s.p
    (CONSTRAINT chk CHECK ("s.weird" > 0)) FOR VALUES FROM (0) TO (100);
SELECT * FROM pg_get_table_ddl('s.pc', 'owner', 'false',
                               'schema_qualified', 'false');

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

I can confirm that most fixes work correctly, but there are two
remaining issues / regressions caused by the fixes:

1.

CREATE TABLE pidx (a int, b int) PARTITION BY RANGE (a);
CREATE TABLE pidx_1 PARTITION OF pidx FOR VALUES FROM (0) TO (10);
CREATE TABLE pidx_2 PARTITION OF pidx FOR VALUES FROM (10) TO (20);
CREATE INDEX ON pidx (b);

is emitted as:

CREATE TABLE public.pidx (a integer, b integer) PARTITION BY RANGE (a);
CREATE INDEX pidx_b_idx ON ONLY public.pidx USING btree (b);
CREATE TABLE public.pidx_1 PARTITION OF public.pidx FOR VALUES FROM (0) TO (10);
CREATE INDEX pidx_1_b_idx ON public.pidx_1 USING btree (b);   <-- fails here
--  ERROR:  relation "pidx_1_b_idx" already exists
ALTER INDEX public.pidx_b_idx ATTACH PARTITION pidx_1_b_idx;
-- ...

2.

CREATE TABLE idt (a int GENERATED BY DEFAULT AS IDENTITY);
ALTER SEQUENCE idt_a_seq AS smallint;

is emitted as:

-- ERROR:  conflicting or redundant options
-- LINE 1: ....idt (a integer GENERATED BY DEFAULT AS IDENTITY (AS smallin...
CREATE TABLE public.idt (a integer GENERATED BY DEFAULT AS IDENTITY
(AS smallint MAXVALUE 32767) NOT NULL);


> > Interestingly pg_dump also skips this, that looks like a pg_dump bug?
>
> The behavior was indeed inconsistent with what emit_indexes() already
> did for non-constraint indexes. emit_local_constraints() now emits ALTER
> INDEX ... ALTER COLUMN n SET STATISTICS for PK/UNIQUE/EXCLUSION constraint
> backing indexes.

I submitted a matching patch for pg_dump:
https://www.postgresql.org/message-id/CAN4CZFMd9riOuV5LkM%2BuV%3DuF-HYp49C8Fh22xWyMxptGLBCZ3A%40mail.gmail.com

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

I did some more testing, I noticed one more issue with self
referencing foreign keys:

CREATE TABLE t (id int PRIMARY KEY, parent_id int REFERENCES t(id));
SELECT pg_get_table_ddl('t'::regclass);
-- CREATE TABLE public.t (id integer NOT NULL, parent_id integer);
-- ALTER TABLE public.t OWNER TO postgres;
-- ALTER TABLE public.t ADD CONSTRAINT t_parent_id_fkey FOREIGN KEY
(parent_id) REFERENCES public.t(id);
-- ALTER TABLE public.t ADD CONSTRAINT t_pkey PRIMARY KEY (id);

It tries to add the foreign key before the primary, and fails with
`ERROR:  there is no unique constraint matching given keys for
referenced table "t"`

There's also another issue in schema_qualified false, with partitions
in different schemas:

CREATE SCHEMA s;
CREATE SCHEMA other;
CREATE TABLE s.pt (id int, val int) PARTITION BY RANGE (id);
CREATE TABLE other.pt_c PARTITION OF s.pt FOR VALUES FROM (0) TO (100);
SELECT pg_get_table_ddl('s.pt'::regclass, schema_qualified => false);
-- CREATE TABLE pt (id integer, val integer) PARTITION BY RANGE (id);
-- ALTER TABLE pt OWNER TO postgres;
-- CREATE TABLE pt_c PARTITION OF s.pt FOR VALUES FROM (0) TO (100);
-- ALTER TABLE pt_c OWNER TO postgres;

The second create table statement references pt as s.pt, which seems incorrect.
It is also missing its own schema qualification, which I'm unsure if
it is wrong or not. If I interpret the documentation strictly, it
isn't the target table, so it should appear with its schema
qualification?

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

Hello!

I did some basic testing with the new functions, and found a few bugs:

1. Seems like check constraints on partitions are ignored:

CREATE TABLE p (id int, val int) PARTITION BY RANGE (id);
CREATE TABLE p_child PARTITION OF p (CONSTRAINT chk_inline CHECK (val > 0))
    FOR VALUES FROM (0) TO (100);
SELECT * FROM pg_get_table_ddl('p_child', 'owner','false');

2. inherited stored generated columns can't be replayed:

CREATE TABLE par_s (
    id int,
    g int GENERATED ALWAYS AS (id * 2) STORED
);
CREATE TABLE ch_s () INHERITS (par_s);
SELECT * FROM pg_get_table_ddl('ch_s', 'owner','false');
-- CREATE TABLE public.ch_s () INHERITS (public.par_s);
-- ALTER TABLE public.ch_s ALTER COLUMN g SET DEFAULT (id * 2);

Dropping ch_s, executing the returned statements:

ERROR:  column "g" of relation "ch_s" is a generated column
HINT:  Use ALTER TABLE ... ALTER COLUMN ... SET EXPRESSION instead.

3. named not null constraints can't be replayed:

CREATE TABLE t (a int CONSTRAINT my_nn NOT NULL);
SELECT * FROM pg_get_table_ddl('t'::regclass,'owner','false');
-- CREATE TABLE public.t ( a integer NOT NULL);
-- ALTER TABLE public.t ADD CONSTRAINT my_nn NOT NULL a;

Dropping t, executing the statements:

ERROR:  cannot create not-null constraint "my_nn" on column "a" of table "t"
DETAIL:  A not-null constraint named "t_a_not_null" already exists for
this column.

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

Thanks, I can confirm that only_foreign_keys works properly.

> Combining `only_foreign_keys=true` with `includes_foreign_keys=false` is
> rejected upfront since it would produce no output.

Shouldn't only_foreign_keys=true with include_constraints=false also error out?

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

Thanks, I can confirm that the previous bugs were fixed, however the
bugfixes also introduce a new issue, where inherited not null
constraints are missing:

CREATE TABLE par (a int);
CREATE TABLE ch (b int) INHERITS (par);
ALTER TABLE ch ADD CONSTRAINT my_nn NOT NULL a;
SELECT * FROM pg_get_table_ddl('ch','owner','false');
--  CREATE TABLE public.ch ( b integer) INHERITS (public.par);

The schema_qualified => false part also doesn't work as described:

+        CREATE STATISTICS statement.  References to
+        objects in the same schema as the target table (inheritance
+        parents, partition parents, identity sequences, and any
+        same-schema object the deparse helpers happen to mention) are
+        also emitted unqualified, so the script can be replayed under a
+        different search_path to recreate the table
+        in another schema.

but:

CREATE SCHEMA s1;
CREATE SEQUENCE s1.myseq;
CREATE FUNCTION s1.f(int) RETURNS int LANGUAGE sql IMMUTABLE AS 'SELECT $1';
CREATE TABLE s1.t (
    id  int DEFAULT nextval('s1.myseq'),
    val int,
    CONSTRAINT chk CHECK (s1.f(val) > 0)
);
SELECT * FROM pg_get_table_ddl('s1.t', 'owner','false',
'schema_qualified','false');
--  CREATE TABLE t ( id integer DEFAULT nextval('s1.myseq'::regclass),
val integer, CONSTRAINT chk CHECK ((s1.f(val) > 0)));

s1 appears twice in the output.

It also has an issue with strings containing the schema:

CREATE SCHEMA myschema;
CREATE TABLE myschema.p (id int, note text) PARTITION BY RANGE (id);
CREATE TABLE myschema.pc PARTITION OF myschema.p
    (CONSTRAINT chk CHECK (note <> 'myschema.secret')) FOR VALUES FROM
(0) TO (100);
SELECT * FROM pg_get_table_ddl('myschema.pc', 'owner','false',
'schema_qualified','false');
-- CREATE TABLE pc PARTITION OF p FOR VALUES FROM (0) TO (100);
-- ALTER TABLE pc ADD CONSTRAINT chk CHECK ((note <> 'secret'::text));

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

Hello!

I can confirm the previous issues fixed, however I also found one more
with unique indexes on partitioned tables:

CREATE SCHEMA s;
CREATE TABLE s.p (id int, region text) PARTITION BY LIST (region);
CREATE TABLE s.p_a PARTITION OF s.p FOR VALUES IN ('a');
CREATE UNIQUE INDEX p_uidx ON s.p (id, region);

SELECT pg_get_table_ddl('s.p', owner => false);
-- CREATE TABLE s.p (id integer, region text) PARTITION BY LIST (region);
-- CREATE UNIQUE INDEX p_uidx ON s.p USING btree (id, region);
-- CREATE TABLE s.p_a PARTITION OF s.p FOR VALUES IN ('a');
-- CREATE UNIQUE INDEX p_a_id_region_idx ON s.p_a USING btree (id,
region); -- fails because index already exists

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

The new version seem to work correctly to me, I didn't find any
further issues. Now the main blockers seem to be the remaining TODOs
related to includes_triggers/includes_policies.

I only have some minor comments about code structuring:

+ * get_inheritance_parents
+ *		Return a List of parent OIDs for relid, ordered by inhseqno.
+ *
+ * find_inheritance_children() walks the opposite direction (parent->children),

Shouldn't this follow the same naming and parameter pattern and live
at the same place in pg_inherits?

+static char *
+lookup_qualified_relname(Oid relid)
+...
+static char *
+lookup_relname_for_emit(Oid relid, bool schema_qualified, Oid base_namespace)

Is lookup_qualified_relname needed? It is only called within
lookup_relname_for_emit, and it results in a double syscache lookup,
which could be avoided if these were a single function.

+		/* COMPRESSION clause, only if explicitly set on the column. */
+		if (CompressionMethodIsValid(att->attcompression))
+		{
+			const char *cm = NULL;
+
+			switch (att->attcompression)
+			{
+				case TOAST_PGLZ_COMPRESSION:
+					cm = "pglz";
+					break;
+				case TOAST_LZ4_COMPRESSION:
+					cm = "lz4";
+					break;
+			}
+			if (cm)
+				appendStringInfo(buf, " COMPRESSION %s", cm);
+		}

Isn't this basically GetCompressionMethodName(att->attcompression)?

+		/* STORAGE clause, only if it differs from the type's default. */
+		if (att->attstorage != get_typstorage(att->atttypid))
+		{
+			const char *storage = NULL;
+
+			switch (att->attstorage)
+			{
+				case TYPSTORAGE_PLAIN:
+...

And this seems like storage_name(att->attstorage) from tablecmds.c,
the only issue is that that's currently static

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

The new design is definitely an improvement as a more generic
interface. I'm unsure about the "include" wording, to me that suggest
"also includes", while the actual behavior is "only includes". To me
it would be a bit surprising pg_get_table_ddl including something not
including the basic create table statement.

I also found a few issues with include in v10.

Include check doesn't seem to work:

CREATE TABLE t (
    id int PRIMARY KEY,
    qty int CHECK (qty > 0),
    CONSTRAINT t_id_pos CHECK (id > 0)
);

SELECT * FROM pg_get_table_ddl('t', 'include', 'check'); -- empty?

The include partitions clause also doesn't work:

CREATE TABLE p (id int, val text) PARTITION BY RANGE (id);
CREATE TABLE p_a PARTITION OF p FOR VALUES FROM (0) TO (100);
CREATE TABLE p_b PARTITION OF p FOR VALUES FROM (100) TO (200);

SELECT * FROM pg_get_table_ddl('p','include','partitions'); -- empty

There's also an issue with replica identity non primary key unique indexes:

CREATE TABLE t2 (a int NOT NULL UNIQUE, b int);
ALTER TABLE t2 REPLICA IDENTITY USING INDEX t2_a_key;
SELECT * FROM pg_get_table_ddl('t2','exclude','unique');
-- ALTER TABLE public.t2 REPLICA IDENTITY USING INDEX t2_a_key;
-- but there's no such index

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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