MIME-Version: 1.0
References: 
 <CALDaNm3zSxK1g0UTo4RjPR3JEc-vyPzANOnKEKjiW88xtUUdzA@mail.gmail.com>
 <CAJpy0uCGnRQnpNVQaApRxqN=9t5VpHRk5b+Mfdf3cqCEW=s+4A@mail.gmail.com>
 <OSZPR01MB63102C42A24D59FACF6D9CD6FD4A9@OSZPR01MB6310.jpnprd01.prod.outlook.com>
 <CAJpy0uCbwqWj+p_yj1AHyiufzAUv_H29qOaztAXxFoTqZ9WcAw@mail.gmail.com>
 <OSZPR01MB6310E0509F229AF2A9414BB1FD499@OSZPR01MB6310.jpnprd01.prod.outlook.com>
 <CAJpy0uBwCZCniPR6vh26L+wpSf4xzUN8omUa9DzF-x1CAud_xA@mail.gmail.com>
In-Reply-To: 
 <CAJpy0uBwCZCniPR6vh26L+wpSf4xzUN8omUa9DzF-x1CAud_xA@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 6 Jun 2023 16:26:25 +0530
Message-ID: 
 <CAA4eK1LOr+2O+_pWKTaa0y9vbW6whfm-8-fuBvnS6OBiaR+7TA@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: shveta malik <shveta.malik@gmail.com>
Cc: "Yu Shi (Fujitsu)" <shiy.fnst@fujitsu.com>,
 vignesh C <vignesh21@gmail.com>,
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>,
 Ajin Cherian <itsajin@gmail.com>,
	"Wei Wang (Fujitsu)" <wangw.fnst@fujitsu.com>,
 Runqi Tian <runqidev@gmail.com>,
	Peter Smith <smithpb2250@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 li jie <ggysxcq@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Masahiko Sawada <sawada.mshk@gmail.com>, Japin Li <japinli@hotmail.com>,
	rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Zheng Li <zhengli10@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1LOr%2B2O%2B_pWKTaa0y9vbW6whfm-8-fuBvnS6OBiaR%2B7TA%40mail.gmail.com>
Precedence: bulk

On Mon, Jun 5, 2023 at 3:00=E2=80=AFPM shveta malik <shveta.malik@gmail.com=
> wrote:
>

Few assorted comments:
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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?

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.

3. 0005* patch has the following changes in docs:
+    <table id=3D"ddl-replication-by-command-tag">
+      <title>DDL Replication Support by Command Tag</title>
+      <tgroup cols=3D"3">
+        <colspec colname=3D"col1" colwidth=3D"2*"/>
+        <colspec colname=3D"col2" colwidth=3D"1*"/>
+        <colspec colname=3D"col3" colwidth=3D"1*"/>
+      <thead>
+       <row>
+        <entry>Command Tag</entry>
+        <entry>For Replication</entry>
+        <entry>Notes</entry>
+       </row>
+      </thead>
+      <tbody>
+       <row>
+        <entry align=3D"left"><literal>ALTER AGGREGATE</literal></entry>
+        <entry align=3D"center"><literal>-</literal></entry>
+        <entry align=3D"left"></entry>
+       </row>
+       <row>
+        <entry align=3D"left"><literal>ALTER CAST</literal></entry>
+        <entry align=3D"center"><literal>-</literal></entry>
+        <entry align=3D"left"></entry>
...
...

If the patch's scope is to only support replication of table DDLs, why
such other commands are mentioned?

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

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.

6.
+ if (relpersist =3D=3D RELPERSISTENCE_PERMANENT)
+ {
+ ddl_deparse_context context;
+ char    *json_string;
+
+ context.verbose_mode =3D false;
+ context.func_volatile =3D PROVOLATILE_IMMUTABLE;

Can we write some comments as to why PROVOLATILE_IMMUTABLE is chosen here?

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_result=
s.pl.orig

Will this file require for the 0008 patch? Or is this just a leftover?

--=20
With Regards,
Amit Kapila.