MIME-Version: 1.0
References: 
 <CAAD30UKg8rXeGM8Oy_MAmxKBL_K5DiHXdeNF=hUefcu1C_6VfQ@mail.gmail.com>
 <20221006171601.6um4ey5idm4h62vf@alvherre.pgsql>
 <CAAD30UJh+LcUND+zg_A5dbQ_Bi=m_n7qUfKrOwAx2v4jBKWvKQ@mail.gmail.com>
 <CAFPTHDbVujj6C3TdMCmoBXovpc4=5Ow3i5M1_HNhmnqdiA5qSA@mail.gmail.com>
 <CAAD30UKCCpMTZd4WSSxYu-qRAEFd+0kjXTgje+EXHdo1JTkz0g@mail.gmail.com>
 <CALDaNm3F9GvQ9bPrxobhqkUKP3HmrRZGCU3EX0xt3=Ef0-Reaw@mail.gmail.com>
 <CAFPTHDY2O42ouQFMeEbPt51CWQ=zyzYhgK6B9basyd5PLaOv0A@mail.gmail.com>
 <CAFPTHDZmS2PrOyJx8OAe+Nt-Fx4-GZetJatqCEJhDNXA2orwCg@mail.gmail.com>
 <CAFiTN-tWkJW-JEGANkK+O3KXUjv_Yb5Tb+r83ujbognHG_brTA@mail.gmail.com>
 <CALDaNm08gZq9a7xnsbaJMmHmi29_kbEuyShHHfxAKLXPh6btWQ@mail.gmail.com>
 <CAFPTHDbGRpXOF-j+oemm=xkzjrh-ZnqPV7SMS_+QBPYdUwhWxg@mail.gmail.com>
 <CAAD30UL=wV7pvdwJ6w5UQUc3urHs6W2vnomK2MKUyek3rzsKcA@mail.gmail.com>
 <CAHut+Psm+9q++y8b70QTeBeZiYcfDNtc_obxSwhxukp0MFhnRA@mail.gmail.com>
 <CAHut+PsO0dwWoB4B6L3Ucd6D8ckgdXY2Sd3JK7F_3wLsXU7ZAg@mail.gmail.com>
 <CAHut+Puxo_kq2toicNK_BQdeccK3REGW-Xv8tVauFvTNku6V-w@mail.gmail.com>
 <CAHut+Pu6H6hY0JJNNRCRmFpM_3817z=0xjm-_ibP+cL85pBOpQ@mail.gmail.com>
 <CALDaNm3q_2Qfo4XbFkBT8mPz_ALS2MyuR=isP7_8Pz27JnLHiw@mail.gmail.com>
In-Reply-To: 
 <CALDaNm3q_2Qfo4XbFkBT8mPz_ALS2MyuR=isP7_8Pz27JnLHiw@mail.gmail.com>
From: li jie <ggysxcq@gmail.com>
Date: Mon, 19 Dec 2022 18:02:17 +0800
Message-ID: 
 <CAGfChW5Qbb8WKznzVqp+cyTqkzLt_vEeSe14td14et6HjnH7qw@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: vignesh C <vignesh21@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Zheng Li <zhengli10@gmail.com>,
	Ajin Cherian <itsajin@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
 "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
	Japin Li <japinli@hotmail.com>, rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAGfChW5Qbb8WKznzVqp%2BcyTqkzLt_vEeSe14td14et6HjnH7qw%40mail.gmail.com>
Precedence: bulk

I have presented some comments below:

1. AT_AddColumn

> + tmpobj =3D new_objtree_VA("ADD %{objtype}s %{definition}s", 3,
[ IF NOT EXISTS ] is missing here.

2.  AT_DropColumn
> + tmpobj =3D new_objtree_VA("DROP %{objtype}s %{column}I", 3,
[ IF EXISTS ] is missing here.

3. AT_DropConstraint
> + tmpobj =3D new_objtree_VA("DROP CONSTRAINT %{constraint}I", 2,
[ IF EXISTS ] is missing here.

4. AT_DetachPartition
> + tmpobj =3D new_objtree_VA("DETACH PARTITION %{partition_identity}D", 2,
[ CONCURRENTLY | FINALIZE ] is missing here.

5. deparse_CreateSeqStmt
> + ret =3D new_objtree_VA("CREATE %{persistence}s SEQUENCE %{identity}D %{=
definition: }s", 3,
[ IF NOT EXISTS ] is missing here.

6. deparse_IndexStmt
> + ret =3D new_objtree_VA("CREATE %{unique}s INDEX %{concurrently}s %{if_n=
ot_exists}s %{name}I ON %{table}D USING %{index_am}s (%{definition}s)", 7,
[ INCLUDE ] and [ ONLY ] are  missing here.

7. deparse_RuleStmt
> + foreach(cell, actions)
> + list =3D lappend(list, new_string_object(lfirst(cell)));

if (actions =3D=3D NIL)
 list =3D lappend(list, new_string_object("NOTHING"));
else
{
  foreach(cell, actions)
  list =3D lappend(list, new_string_object(lfirst(cell)));
}

8. AT_AddIndexConstraint
> + tmpobj =3D new_objtree_VA("ADD CONSTRAINT %{name}I %{constraint_type}s =
USING INDEX %{index_name}I %{deferrable}s %{init_deferred}s", 6,
> + "type", ObjTypeString, "add constraint using index",
> + "name", ObjTypeString, get_constraint_name(constrOid),
> + "constraint_type", ObjTypeString,
> + istmt->deferrable ? "DEFERRABLE" : "NOT DEFERRABLE",

"constraint_type", ObjTypeString,
istmt->primary ? "PRIMARY KEY" : "UNIQUE",

9. regress test

Zheng Li <zhengli10@gmail.com> =E4=BA=8E2022=E5=B9=B412=E6=9C=8812=E6=97=A5=
=E5=91=A8=E4=B8=80 12:58=E5=86=99=E9=81=93=EF=BC=9A


>
> Hi,
>
> Attached please find the DDL deparser testing module in the v45-0007
> patch, this testing module is written by Runqi Tian in [1] with minor
> modification from myself. I think we can
> start adding more tests to the module now that we're getting close to
> finish implementing the DDL deparser.
>
> This testing module ddl_deparse_regress aims to achieve the following
> four testing goals for the DDL deparser:
>     1. Test that the generated JSON blob is expected using SQL tests.
>     2. Test that the re-formed DDL command is expected using SQL tests.
>     3. Test that the re-formed DDL command has the same effect as the
> original command
>        by comparing the results of pg_dump, using the SQL tests in 1 and =
2.
>     4. Test that any new DDL syntax is handled by the DDL deparser by
> capturing and deparsing
>        DDL commands ran by pg_regress.
>
> 1 and 2 is tested with SQL tests, by comparing the deparsed JSON blob
> and the re-formed command.
> 3 is tested with TAP framework in t/001_compare_dumped_results.pl
> 4 is tested with TAP framework and pg_regress in 002_regress_tests.pl,
> the execution is currently commented out because it will fail due
> unimplemented commands in the DDL deparser.
>
> [1] https://www.postgresql.org/message-id/flat/CAH8n8_jMTunxxtP4L-3tc%3DG=
Namg%3Dmg1X%3DtgHr9CqqjjzFLwQng%40mail.gmail.com
>

The test patch is very useful.
 I see that the sql case in test_ddl_deparse_regress is similar to the
one in test_ddl_deparse.
Why don't we merge the test cases in test_ddl_deparse_regress into
test_ddl_deparse,
as the sql case can be completely reused with the sql files in test_ddl_dep=
arse?
I believe this will make the tests more comprehensive and reduce redundancy=
.


Regards,
li jie