Re: DDL deparse - Mailing list pgsql-hackers

I've updated the DDL deparse patch for CREATE TABLE and ALTER TABLE.
There are three major changes from the previous DDL deparse code;

1. The new design explicitly mentions that DDL deparse should preserve
user intent (e.g., a clause user didn't write is not emitted) while
allowing to normalize and schema-qualify the original DDL. That makes
things what we should follow in future changes.

2. Support the latest CREATE/ALTER TABLE syntaxes and refactored the
deparse code while fixing a lot of bugs. I've refactored the DDL
deparse code and introduced some helper functions to make JSON blob
construction easy. Also, the previous patch could not deparse some
CREATE/ALTER TABLE syntax properly. For instance,

- "create table t (a int, unique (a) using index tablespace ts
deferrable);" produces "CREATE TABLE public.t (a pg_catalog.int4
STORAGE PLAIN, CONSTRAINT t_a_key UNIQUE (a) DEFERRABLE USING INDEX
TABLESPACE ts)", which is not executable syntax.

- "create table t (a int, unique (a) with (fillfactor = 90));"
produces "CREATE TABLE public.t2 (a pg_catalog.int4 STORAGE PLAIN,
CONSTRAINT t2_a_key UNIQUE (a))", which lacks the fillfactor setting.

Also, I found out that we need to differently treat the USING clause
in ALTER TABLE .. ALTER TYPE command from other claude especially when
DROP COLUMN subcommand is also executed in the same ALTER TABLE
command. The following ALTER TABLE cannot be deparsed properly:

create table test (a int, b int);
alter table test drop column a, alter column b type numeric using (a +
b)::numeric;

old design: ALTER TABLE public.test DROP COLUMN a, ALTER COLUMN b SET
DATA TYPE pg_catalog."numeric" USING (("?dropped?column?" +
b))::numeric

new design: ALTER TABLE public.test DROP COLUMN a, ALTER COLUMN b SET
DATA TYPE pg_catalog."numeric" USING ((a + b))::numeri

I've confirmed these are fixed and verified that DDL depasse can
properly deparse all CREATE/ALTER TABLE commands written in the
regression tests by using the new regression tests mentioned below.

3. Regression tests now require only one regression test run.
Previously it required running regression tests twice in order to
prove that deparsed DDLs result in the same effect. A new
001_deparse_regress.pl test reliably detects the problem if newly
added clause/syntaxes miss DDL deparse support.

In the new tests, we prepare an event trigger function, and in the
process utility hook function we execute the CREATE TABLE command in a
subtransaction. In the event trigger function we perform DDL deparse
and save the generated JSON blob in TopTransactionContext. Then,
rollback the subtransaction and re-execute the deparsed DDL again.

I've added more tests for CREATE/ALTER TABLE deparse to test_ddl_deparse.

FYI test_ddl_deparse can be used for interactive tests during the
development. It can be installed via shared_preload_libraries or LOAD
command, and we can set test_ddl_deparse.print_deparsed_ddl to
'json|text|both' to see how the CREATE TABLE DDL is deparsed:

=# create extension test_ddl_deparse;
CREATE EXTENSION
=# set test_ddl_deparse.print_deparsed_ddl to 'both';
SET
=# create table test_tbl (id serial, name text);
NOTICE:  deparsed JSON: {"tag": "CREATE TABLE", "command": {"fmt":
"CREATE%{persistence}s TABLE%{if_not_exists}s
%{identity}D%{of_type}s%{partition_of}s%{table_elements}s%{inherits}s%{partition_bound}s%{partition_by}s%{access_method}s%{with_clause}s%{on_commit}s%{tablespace}s",
"of_type": null, "identity": {"objname" : "test_tbl", "schemaname":
"public"}, "inherits": null, "on_commit": null, "tablespace": null,
"persistence": null, "with_clause": null, "partition_by": null,
"partition_of": null, "access_method": null, "if_not_exists": null,
"table_elements": {"fmt": " (%{elements:, }s)", "elements": [{"fmt":
"%{name}I %{coltype}T%{storage}s%{compression}s%{collation}s%{not_null}s%{default}s%{identity_column}s%{generated_column}s",
"name": "id", "type": "column", "coltype": {"typmod": "", "typarray":
false, "typename": "serial", "schemaname": ""}, "default": null,
"storage": null, "not_null": null, "collation": null, "compression":
null, "identity_column": null, "generated_column": null}, {"fmt":
"%{name}I %{coltype}T%{storage}s%{compression}s%{collation}s%{not_null}s%{default}s%{identity_column}s%{generated_column}s",
"name": "name", "type": "column", "coltype": {"typmod": "",
"typarray": false, "typename": "text", "schemaname": "pg_catalog"},
"default": null, "storage": null, "not_null": null, "collation": null,
"compression": null, "identity_column": null, "generated_column":
null}]}, "partition_bound": null}}
NOTICE:  deparsed DDL: CREATE TABLE public.test_tbl (id serial, name
pg_catalog.text)
CREATE TABLE

And setting test_ddl_deparse.execute_deparsed_ddl to on does the
round-trip test.

Thanks for the updated patches. Looking ahead to DDLR, would it make sense to separate command collection from event-trigger state, so that both event triggers and the DDLR implementation can consume it?

The existing collector, including the additional capture for ALTER TYPE USING and partition operations, seems like a useful foundation. Collection could be enabled when either consumer needs it, while event-trigger registration, filtering, and invocation remain separate.

The consumers may also need different notification points. Ordinary CREATE/ALTER could expose the collected command before end-event callbacks execute, whereas CTAS would need a schema-only capture point after destination creation and before row insertion for the DDLR case.