Re: DDL deparse

Hi Sawada-san,

>
> >
> >>
> >> 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?
>
> Yes, that's the infrastructure I mentioned earlier. I have a PoC patch
> for it as a follow-up to the DDL deparse patches, and I'll post it
> once these settle.
>
> It does the split the way you describe. With that patch DDLs are
> captured when effective_wal_level is 'logical', so that any logical
> decoding output plugin can see them, and DDL replication can be built
> on top.
>
> Good point about CTAS. The PoC doesn't handle it yet.
> ExecCreateTableAs() creates the destination and then runs the query,
> and collection happens at the end of ProcessUtilitySlow(), so the DDL
> would be emitted after the inserts it has to precede. A schema-only
> capture point between the two is needed, as you say.
>
> I've rebased and updated the patch.
>
>
I reviewed and tested the v2 series. And found some issues that I attempted
to fix via
two follow-up patches, to apply in order on top of your four-patch series.

The first addresses three correctness cases:

* Repeated identity clauses on the same column currently match the first
raw subcommand more than once. The patch matches successive collected
identity subcommands to successive raw occurrences.

* ADD COLUMN b int DEFAULT 1 followed by ALTER COLUMN b SET DEFAULT 2
currently reads the final catalog default for both clauses. The patch
captures each relevant subcommand's cooked default when it is collected,
preserving the distinct effects on existing rows and future inserts.

* Table-level IF EXISTS is preserved for ordinary ALTER TABLE, column and
constraint renames, and SET SCHEMA. Table rename already handles it.

The second patch adds SQL regression coverage for nested DDL. I'm happy to
report that these cases worked with the existing deparsing logic. The
harness changes
use the existing settings without adding another GUC.

I know that full deparse support is still pending :-) (and am happy to
contribute towards
full deparse support as well) but looking ahead to DDLR, do you have a
message
format in mind for conveying the deparsed commands to subscribers?

The deparser already produces a JSON envelope that can be expanded into
SQL. Would you carry that JSON in a transactional logical message, or
expand it on the publisher and send SQL with a separate metadata envelope?

This also seems closely related to your subscriber logical-message hook
work. Do you see that hook as the intended entry point for DDL apply, or
would you prefer a dedicated logical replication protocol message?

Hope you find these fixes and test coverage patches useful.

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