Re: Support logical replication of DDLs

On Fri, 18 Mar 2022 at 08:18, Zheng Li <zhengli10(at)gmail(dot)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.