Re: Support logical replication of DDLs

On Tue, Jun 6, 2023 at 4:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jun 5, 2023 at 3:00 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
>
> Few assorted comments:

Hi Amit, thanks for the feedback. Addressed these in recent patch
posted  (*2023_06_08.patch)

> ===================
> 1. I see the following text in 0005 patch: "It supports most of ALTER
> TABLE command except some commands(DDL related to PARTITIONED TABLE
> ...) that are recently introduced but are not yet supported by the
> current ddl_deparser, we will support that later.". Is this still
> correct?
>

Removed this from the commit message.

> 2. I think the commit message of 0008
> (0008-ObjTree-Removal-for-multiple-commands-2023_05_22) should be
> expanded to explain why ObjTree is not required and or how you have
> accomplished the same with jsonb functions.
>

Done.

> 3. 0005* patch has the following changes in docs:
> +    <table id="ddl-replication-by-command-tag">
> +      <title>DDL Replication Support by Command Tag</title>
> +      <tgroup cols="3">
> +        <colspec colname="col1" colwidth="2*"/>
> +        <colspec colname="col2" colwidth="1*"/>
> +        <colspec colname="col3" colwidth="1*"/>
> +      <thead>
> +       <row>
> +        <entry>Command Tag</entry>
> +        <entry>For Replication</entry>
> +        <entry>Notes</entry>
> +       </row>
> +      </thead>
> +      <tbody>
> +       <row>
> +        <entry align="left"><literal>ALTER AGGREGATE</literal></entry>
> +        <entry align="center"><literal>-</literal></entry>
> +        <entry align="left"></entry>
> +       </row>
> +       <row>
> +        <entry align="left"><literal>ALTER CAST</literal></entry>
> +        <entry align="center"><literal>-</literal></entry>
> +        <entry align="left"></entry>
> ...
> ...
>
> If the patch's scope is to only support replication of table DDLs, why
> such other commands are mentioned?
>

Removed the other commands and made some adjustments.

> 4. Can you write some comments about the deparsing format in one of
> the file headers or in README?
>

Added atop ddljson.c as this file takes care of expansion based on
fmt-string added.

> 5.
> +   <para>
> +    The <literal>table_init_write</literal> event occurs just after
> the creation of
> +    table while execution of <literal>CREATE TABLE AS</literal> and
> +    <literal>SELECT INTO</literal> commands.
>
> Patch 0001 has multiple references to table_init_write trigger but it
> is introduced in the 0002 patch, so those changes either belong to
> 0002 or one of the later patches. I think that may reduce most of the
> changes in event-trigger.sgml.
>

Moved it to 0002 patch.

> 6.
> + if (relpersist == RELPERSISTENCE_PERMANENT)
> + {
> + ddl_deparse_context context;
> + char    *json_string;
> +
> + context.verbose_mode = false;
> + context.func_volatile = PROVOLATILE_IMMUTABLE;
>
> Can we write some comments as to why PROVOLATILE_IMMUTABLE is chosen here?
>

Added some comments and modified the variable name to make it more
appropriate.

> 7.
> diff --git a/src/test/modules/test_ddl_deparse_regress/t/001_compare_dumped_results.pl.orig
> b/src/test/modules/test_ddl_deparse_regress/t/001_compare_dumped_results.pl.orig
> new file mode 100644
> index 0000000000..58cf7cdf33
> --- /dev/null
> +++ b/src/test/modules/test_ddl_deparse_regress/t/001_compare_dumped_results.pl.orig
>
> Will this file require for the 0008 patch? Or is this just a leftover?
>

Sorry, added by mistake. Removed it now.

thanks
Shveta