Re: Support logical replication of DDLs

On Mon, Jun 12, 2023 at 7:17 AM Wei Wang (Fujitsu)
<wangw.fnst@fujitsu.com> wrote:
>
> On Thur, Jun 8, 2023 20:02 PM shveta malik <shveta.malik@gmail.com> wrote:
> > Thank You Vignesh for handling (a), Ajin for handling (b), Shi-san and
> > Hou-san for contributing in (c).
> >
> > The new changes are in patch 0001, 0002, 0005 and 0008.
>
> Thanks for updating the patch set.
>
> Here are some comments:
> ===
> For 0002 patch.
> 1. The typo atop the function EventTriggerTableInitWrite.
> ```
> +/*
> + * Fire table_init_rewrite triggers.
> + */
> +void
> +EventTriggerTableInitWrite(Node *real_create, ObjectAddress address)
> ```
> s/table_init_rewrite/table_init_write
>
> ~~~
>
> 2. The new process for "SCT_CreateTableAs" in the function pg_event_trigger_ddl_commands.
> With the event trigger created in
> test_ddl_deparse_regress/sql/test_ddl_deparse.sql, when creating the table that
> already exists with `CreateTableAs` command, an error is raised like below:
> ```
> postgres=# CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
> postgres=# CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
> NOTICE:  relation "as_select1" already exists, skipping
> ERROR:  unrecognized object class: 0
> CONTEXT:  PL/pgSQL function test_ddl_deparse() line 6 at FOR over SELECT rows
> ```
> It seems that we could check cmd->d.ctas.real_create in the function
> pg_event_trigger_ddl_commands and return NULL in this case.
>
> ===
> For 0004 patch.
> 3. The command tags that are not supported for deparsing in the tests.
> ```
>         FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
>                 -- Some TABLE commands generate sequence-related commands, also deparse them.
>                 WHERE command_tag in ('ALTER FOREIGN TABLE', 'ALTER TABLE',
>                                                           'CREATE FOREIGN TABLE', 'CREATE TABLE',
>                                                           'CREATE TABLE AS', 'DROP FOREIGN TABLE',
>                                                           'DROP TABLE', 'ALTER SEQUENCE',
>                                                           'CREATE SEQUENCE', 'DROP SEQUENCE')
> ```
> Since foreign table is not supported yet in the current patch set, it seems that
> we need to remove "FOREIGN TABLE" related command tag. If so, I think the
> following three files need to be modified:
> - test_ddl_deparse_regress/sql/test_ddl_deparse.sql
> - test_ddl_deparse_regress/t/001_compare_dumped_results.pl
> - test_ddl_deparse_regress/t/002_regress_tests.pl
>
> ~~~
>
> 4. The different test items between meson and Makefile.
> It seems that we should keep the same SQL files and the same order of SQL files
> in test_ddl_deparse_regress/meson.build and test_ddl_deparse_regress/Makefile.
>
> ===
> For 0004 && 0008 patches.
> 5. The test cases in the test file test_ddl_deparse_regress/t/001_compare_dumped_results.pl.
> ```
> # load test cases from the regression tests
> -my @regress_tests = split /\s+/, $ENV{REGRESS};
> +#my @regress_tests = split /\s+/, $ENV{REGRESS};
> +my @regress_tests = ("create_type", "create_schema", "create_rule", "create_index");
> ```
> I think @regress_tests should include all SQL files, instead of just four. BTW,
> the old way (using `split /\s+/, $ENV{REGRESS}`) doesn't work in meson.
>

Wang-san, Thank You for your feedback. In the latest version, we have
pulled out CTAS and the test_ddl_deparse_regress module. I will
revisit your comments once we plan to put these modules back.

thanks
Shveta