References: 
 <CAAD30ULtoGp8L_GKbV15Wnm+X5r=SE7MOnYHuqBr396m26jJSA@mail.gmail.com>
 <202203162206.7spggyktx63e@alvherre.pgsql>
 <CAAD30UKRUusq8JyyHzAv71=ncN22OE8OkOOyAWvRHW3wXNjyyA@mail.gmail.com>
 <CAAD30UKTp87+kvGZYL3M2Suxq=WEvFUG24ZRT0yT9rqdkP=uMA@mail.gmail.com>
User-agent: mu4e 1.4.14; emacs 27.1
From: Japin Li <japinli@hotmail.com>
To: Zheng Li <zhengli10@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Dilip Kumar
 <dilipbalaut@gmail.com>, rajesh.rs0541@gmail.com, PostgreSQL Hackers
 <pgsql-hackers@lists.postgresql.org>
Subject: Re: Support logical replication of DDLs
In-reply-to: 
 <CAAD30UKTp87+kvGZYL3M2Suxq=WEvFUG24ZRT0yT9rqdkP=uMA@mail.gmail.com>
Date: Fri, 18 Mar 2022 18:19:38 +0800
Message-ID: 
 <MEYP282MB1669863D5C31D7F6A1D996D8B6139@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 49bb4dbe-f1a2-4744-d98c-08da08c8d24d
X-MS-TrafficTypeDiagnostic: SY4P282MB2074:EE_
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: 
	y01YRgoO0PSYjYRuiJ3KWhDGrn2xBmjxOZxm3jrES3yiQ6iBPp3uphJ4z0hsKVfVbR5SiVbzR08R43LYa1k82C68F4j5ihAl3lbgzOC4FAd7R/CwecRQ/vFpBnZ2fD73Yqvx+TQFPk0f89ujYT8yNMZsK0crOp/Sw0VKDsteTXuIBRT2Nnj+KidYrkvfsM8fN15eOcI5Tc4QT0lO/K/8oRmndmttUFJs/Gywg9MxoId+veg6uvJOxfLWCicPs3zgyIeFzkbKHRLyht65Mr/q3Xj4/MLYGimcBoNN0gzdHO9ednKf05PAXoWgFIy4yhKKQGxWmj//IV943VgpOZCBxW7pTeqZQxf+A9m3wesFFznZoek+DBFYM1wmQeVv5kqi/INSPCLPZqsy5VTnsejK5x13KFPdIPsN2Yv3b1sFO+noJZDBduxuS6TDllOLMQ3MQi3eOfUnh0BhdO0qVL0U8ykN+g352wERfod5Vj2+ao3CyBiruRWtff4IJ0YA/3UDMHWBewa+HHctYDLHDZ4EyM/8SYnNyaxCfgmnwFGmRWNr9tIHTGTw6qgOEnvN14ZjAkPIRAidxoznLuuuYJupt+ehrKA9gxZPdZuCFJNgmKvJqw6YrNXhdm0PiZzGV/7J
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?CYQ7BVyXeO72pSLSO1akoRkUh7BDp3ChZYq+T65qESZGX5qgKypRq5E4sLkh?=
 =?us-ascii?Q?DayIOTLStGqMFNaJggTVk3cE/mho74ZfdFqBYYvVbgRi6yBEKEreExgUVVgP?=
 =?us-ascii?Q?S0TkqkXzdjbvPMWxvfm9yj4Z7h3DFEucPXf1yNxql3r3HlW8nwEP94rCqIDi?=
 =?us-ascii?Q?lkzAWennI7plDmj/a5ZNytXq/w4UJaUlUbOTET6DiskQ0fW453A+a6aQgDgt?=
 =?us-ascii?Q?s3uGrS1SzHTFSgaww9zXSPVcqdgHozMLXexhevXEynYT6osLM3rrP34Kt4F3?=
 =?us-ascii?Q?5AoHsfM2XC1LjLQ3oJ9hclTKuNL/vnEdYIm8sOoADmEnrEwY1J9wkULFhTUx?=
 =?us-ascii?Q?btRDTuhoU3RCsp0lFRuwmyruhBPaRF8EODVKiif4ZkQiuobTV5lnjbfYpc0W?=
 =?us-ascii?Q?8dAZDf+OTtBV+uFiDwy0esAL7uLS0hVkaHMlvZlAAVZh4Bjd4hZIqP3jZMZC?=
 =?us-ascii?Q?IMuJYtaxZW7qAfrurrDakEh5IQ0GwdW01sDrOdlbPjZJZ6mPCHxjj8lulsdI?=
 =?us-ascii?Q?+KkrN8nTHaaDfaUgg3B9AKjb6FB4qgGXFtc+ko+kxbqfXSh4Loo+ppMmGw/1?=
 =?us-ascii?Q?Lkob1r16xmq4q2HVvLE9v/B1OdW8AQvOFcwldSn+6Fe+MmdED+L2HRwGXk6B?=
 =?us-ascii?Q?/VS59OQnmy0v7aVmAzuN2m6vnfgV48rL06IPTQDcsq1KcDztq5+hKwVMt4bR?=
 =?us-ascii?Q?ukt3iEqP0etXIyHw7adxZSFwReHsmqced5TinVhycVkJEWARyBMkkKqy2r6W?=
 =?us-ascii?Q?ZBaNhs5q6uK2ZbpJ8kKzDbp3/C0zUDOwJjZ1GoaVifSWjctYghzs928lqVPz?=
 =?us-ascii?Q?c9Mv+eCqPubsrZIifUbVrB8xHLJ4oS6npSrGn4Iib9UHMzv5hCoOq81PG40I?=
 =?us-ascii?Q?OPb4bZmy2Ogb4ThRqcauFVWYUaO8cNzNormbU2EItsetTAJK8g+aKcx3S04Q?=
 =?us-ascii?Q?78MCQft1vRRGpt2M9Tt/5g2AoJ29pdQFzaCiNHrppI9kzDsOvj99GG3h0gH+?=
 =?us-ascii?Q?ff/SxUWcCOzG9Y73/2f39Vqx0A6JSckz3cKuzn+zNQdtwKpZvRcBg8e1eGTP?=
 =?us-ascii?Q?P5uNYtKYKNlUQ0TO/D5Ep0Ni+FiRe0j/jcObCZtGGNM9pF/wR79YECvofa7j?=
 =?us-ascii?Q?p2pVNw/qEKmIgsSJfqONUkcAFRcdcib1bA=3D=3D?=
X-OriginatorOrg: sct-15-20-4755-11-msonline-outlook-746f3.templateTenant
X-MS-Exchange-CrossTenant-Network-Message-Id: 
 49bb4dbe-f1a2-4744-d98c-08da08c8d24d
X-MS-Exchange-CrossTenant-AuthSource: MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Mar 2022 10:19:44.5207
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SY4P282MB2074
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: 
 <https://www.postgresql.org/message-id/MEYP282MB1669863D5C31D7F6A1D996D8B6139%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
Precedence: bulk


On Fri, 18 Mar 2022 at 08:18, Zheng Li <zhengli10@gmail.com> wrote:
> Hello,
>
> Attached please find the broken down patch set. Also fixed the failing
> TAP tests Japin reported.
>


Here are some comments for the new patches:

Seems like you forget initializing the *ddl_level_given after entering the
parse_publication_options(), see [1].


+           if (*ddl_level_given)
+               ereport(ERROR,
+                       (errcode(ERRCODE_SYNTAX_ERROR),
+                        errmsg("conflicting or redundant options")));

We can use the errorConflictingDefElem() to replace the ereport() to make the
error message more readable.

I also think that ddl = '' isn't a good way to disable DDL replication, how
about using ddl = 'none' or something else?

The test_decoding test case cannot work as expected, see below:

diff -U3 /home/px/Codes/postgresql/contrib/test_decoding/expected/ddlmessages.out /home/px/Codes/postgresql/Debug/contrib/test_decoding/results/ddlmessages.out
--- /home/px/Codes/postgresql/contrib/test_decoding/expected/ddlmessages.out    2022-03-18 08:46:57.653922671 +0800
+++ /home/px/Codes/postgresql/Debug/contrib/test_decoding/results/ddlmessages.out       2022-03-18 17:34:33.411563601 +0800
@@ -25,8 +25,8 @@
 SELECT pg_drop_replication_slot('regression_slot');
 DROP TABLE tab1;
 DROP publication mypub;
-                                                                       data
----------------------------------------------------------------------------------------------------------------------------------------------------
+                                                                     data
+-----------------------------------------------------------------------------------------------------------------------------------------------
  DDL message: transactional: 1 prefix:  role: redacted, search_path: "$user", public, sz: 47 content:CREATE TABLE tab1 (id serial unique, data int);
  BEGIN
  sequence public.tab1_id_seq: transactional:1 last_value: 1 log_cnt: 0 is_called:0

Since the DDL message contains the username, and we try to replace the username with 'redacted' to avoid this problem,

    \o | sed 's/role.*search_path/role: redacted, search_path/g'

However, the title and dash lines may have different lengths for different
usernames.  To avoid this problem, how about using a specified username when
initializing the database for this regression test?

I try to disable the ddlmessage regression test,

make[2]: Entering directory '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'
rm -rf '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'/tmp_check && /usr/bin/mkdir -p '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'/tmp_check && cd /home/px/Codes/postgresql/Debug/../src/bin/pg_dump
 && TESTDIR='/home/px/Codes/postgresql/Debug/src/bin/pg_dump' PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/bin:/home/px/Codes/postgresql/Debug/src/bin/pg_dump:$PATH"
 LD_LIBRARY_PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/lib:$LD_LIBRARY_PATH"  PGPORT='65432' PG_REGRESS='/home/px/Codes/postgresql/Debug/src/bin/pg_dump/../../../s
rc/test/regress/pg_regress' /usr/bin/prove -I /home/px/Codes/postgresql/Debug/../src/test/perl/ -I /home/px/Codes/postgresql/Debug/../src/bin/pg_dump  t/*.pl
t/001_basic.pl ................ ok
t/002_pg_dump.pl .............. 13/?
#   Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub1'
#   at t/002_pg_dump.pl line 3916.
# Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI

#   Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub2'
#   at t/002_pg_dump.pl line 3916.
# Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI

#   Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub3'
#   at t/002_pg_dump.pl line 3916.
# Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI

#   Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub4'
#   at t/002_pg_dump.pl line 3916.
# Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI
t/002_pg_dump.pl .............. 240/?
[...]
# Review section_post_data results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI
t/002_pg_dump.pl .............. 7258/? # Looks like you failed 84 tests of 7709.
t/002_pg_dump.pl .............. Dubious, test returned 84 (wstat 21504, 0x5400)
Failed 84/7709 subtests
t/003_pg_dump_with_server.pl .. ok
t/010_dump_connstr.pl ......... ok

Test Summary Report
-------------------
t/002_pg_dump.pl            (Wstat: 21504 Tests: 7709 Failed: 84)
  Failed tests:  136-139, 362-365, 588-591, 1040-1043, 1492-1495
                1719-1722, 1946-1949, 2177-2180, 2407-2410
                2633-2636, 2859-2862, 3085-3088, 3537-3540
                3763-3766, 3989-3992, 4215-4218, 4441-4444
                5119-5122, 5345-5348, 6702-6705, 7154-7157
  Non-zero exit status: 84
Files=4, Tests=7808, 26 wallclock secs ( 0.46 usr  0.05 sys +  7.98 cusr  2.80 csys = 11.29 CPU)
Result: FAIL
make[2]: *** [Makefile:55: check] Error 1
make[2]: Leaving directory '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'
make[1]: *** [Makefile:43: check-pg_dump-recurse] Error 2
make[1]: Leaving directory '/home/px/Codes/postgresql/Debug/src/bin'
make: *** [GNUmakefile:71: check-world-src/bin-recurse] Error 2


And, when I try to use git am to apply the patch, it complains:

	$ git am ~/0001-syntax-pg_publication-pg_dump-ddl_replication.patch
	Patch format detection failed.


[1] https://www.postgresql.org/message-id/MEYP282MB1669DDF788C623B7F8B64C2EB6139%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.