Re: DDL deparse

Hi Sawada-san,

>
> 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.
>
>
Based on all the pros and cons of each of the earlier approaches discussed,
even to me, DDL deparse that preserves user intent seems to be a good way
to move forward towards DDL support in Logical Replication (DDLR).

> 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.
Is this consistent with the shared capture infrastructure you mentioned
earlier, and would you see it as a follow-up patch to the deparser?

Regards,
Nikhil
---
Nikhil Sontakke
PlanetScale Postgres Core Team