Re: Handle infinite recursion in logical replication setup

Here are my review comments for v10-0002 (TAP tests part only)

FIle: src/test/subscription/t/032_localonly.pl

======

1.

+# Detach node C and clean the table contents.
+sub detach_node_clean_table_data
+{

1a. Maybe say "Detach node C from the node-group of (A, B, C) and
clean the table contents from all nodes"

1b. Actually wondered do you need to TRUNCATE from both A and B (maybe
only truncate 1 is OK since those nodes are still using MMC). OTOH
maybe your explicit way makes the test simpler.

~~~

2.

+# Subroutine for verify the data is replicated successfully.
+sub verify_data
+{

2a. Typo: "for verify" -> "to verify"

2b. The messages in this function maybe are not very appropriate. They
say 'Inserted successfully without leading to infinite recursion in
circular replication setup', but really the function is only testing
all the data is the same as 'expected'. So it could be the result of
any operation - not just Insert.

~~~

3. SELECT ORDER BY?

$result = $node_B->safe_psql('postgres', "SELECT * FROM tab_full;");
is($result, qq(11
12
13),
'Node_C data replicated to Node_B'
);

I am not sure are these OK like this or if *every* SELECT use ORDER BY
to make sure the data is in the same qq expected order? There are
multiple cases like this.

(BTW, I think this comment needs to be applied for the v10-0001 patch,
maybe not v10-0002).

~~~

4.

+###############################################################################
+# Specifying local_only 'on' which indicates that the publisher should only
+# replicate the changes that are generated locally from node_B, but in
+# this case since the node_B is also subscribing data from node_A, node_B can
+# have data originated from node_A, so throw an error in this case to prevent
+# node_A data being replicated to the node_C.
+###############################################################################

There is something wrong with the description because there is no
"node_C" in this test. You are just creating a 2nd subscription on
node A.

~~

5.

+($result, $stdout, $stderr) = $node_A->psql(
+ 'postgres', "
+ CREATE SUBSCRIPTION tap_sub_A3
+ CONNECTION '$node_B_connstr application_name=$subname_AB'
+ PUBLICATION tap_pub_B
+ WITH (local_only = on, copy_data = on)");
+like(

It seemed strange to call this 2nd subscription "tap_sub_A3". Wouldn't
it be better to call it "tap_sub_A2"?

~~~

6.

+###############################################################################
+# Join 3rd node (node_C) to the existing 2 nodes(node_A & node_B) bidirectional
+# replication setup when the existing nodes (node_A & node_B) has no data and
+# the new node (node_C) some pre-existing data.
+###############################################################################
+$node_C->safe_psql('postgres', "INSERT INTO tab_full VALUES (31);");
+
+$result = $node_A->safe_psql('postgres', "SELECT * FROM tab_full order by 1;");
+is( $result, qq(), 'Check existing data');
+
+$result = $node_B->safe_psql('postgres', "SELECT * FROM tab_full order by 1;");
+is( $result, qq(), 'Check existing data');
+
+$result = $node_C->safe_psql('postgres', "SELECT * FROM tab_full order by 1;");
+is($result, qq(31), 'Check existing data');
+
+create_subscription($node_A, $node_C, $subname_AC, $node_C_connstr,
+ 'tap_pub_C', 'copy_data = force, local_only = on');
+create_subscription($node_B, $node_C, $subname_BC, $node_C_connstr,
+ 'tap_pub_C', 'copy_data = force, local_only = on');
+

Because the Node_C does not yet have any subscriptions aren't these
cases where you didn't really need to use "force"?

------
Kind Regards,
Peter Smith.
Fujitsu Australia