Here are some review comments for v23-0001.
======
src/test/subscription/t/028_row_filter.pl
1.
+# two publications, one publishing through ancestor and another one directly
+# publsihing the partition, with different row filters
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tap_pub_viaroot_sync_1 FOR TABLE
tab_rowfilter_viaroot_part_sync WHERE (a > 15) WITH
(publish_via_partition_root)"
+);
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tap_pub_viaroot_sync_2 FOR TABLE
tab_rowfilter_viaroot_part_sync_1 WHERE (a < 15)"
+);
+
1a.
Typo "publsihing"
~
1b.
IMO these table and publication names could be better.
I thought it was confusing to have the word "sync" in these table
names and publication names. To the casual reader, it looks like these
are synchronous replication tests but they are not.
Similarly, I thought it was confusing that 2nd publication and table
have names with the word "viaroot" when the option
publish_via_partition_root is not even true.
~~~
2.
# The following commands are executed after CREATE SUBSCRIPTION, so these SQL
# commands are for testing normal logical replication behavior.
#
~
I think you should add a couple of INSERTS for the newly added table/s
also. IMO it is not only better for test completeness, but it causes
readers to question why there are INSERTS for every other table except
these ones.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
