Re: Handle infinite recursion in logical replication setup - Mailing list pgsql-hackers

Below are my review comments for the v9-0002 patch (TAP test part only).

(The order of my comments is a bit muddled because I jumped around in
the file a bit while reviewing it).

======

1. create_subscription - missing comment.

+sub create_subscription
+{
+ my ($node_subscriber, $node_publisher, $sub_name, $node_connstr,
+ $application_name, $pub_name, $copy_data_val)
+   = @_;

Maybe add a comment for this subroutine and describe the expected parameters.

~~

2. create_subscription - hides the options

IMO the "create_subscription" subroutine is hiding too many of the
details, so now it is less clear (from the caller's POV) what the test
is really doing. Perhaps the options could be passed more explicitly
to this subroutine.

e.g. Instead of

create_subscription($node_B, $node_A, $appname_B1, $node_A_connstr,
       $appname_B1, 'tap_pub_A', 'on');

perhaps explicitly set the WITH options like:

create_subscription($node_B, $node_A, $appname_B1, $node_A_connstr,
       $appname_B1, 'tap_pub_A', 'local_only = on, copy_data = on');

~~~

3. the application names are confusing

+my $appname_A1 = 'tap_sub_A_B';
+my $appname_A2 = 'tap_sub_A_C';
+my $appname_B1 = 'tap_sub_B_A';
+my $appname_B2 = 'tap_sub_B_C';
+my $appname_C1 = 'tap_sub_C_A';
+my $appname_C2 = 'tap_sub_C_B';

I found the application names using '1' and '2' to be a bit confusing.
i.e  it was unnecessarily hard to associate them (in my head) with
their relevant subscriptions. IMO it would be easier to name them
using letters like below:

SUGGESTED
my $appname_AB = 'tap_sub_A_B';
my $appname_AC = 'tap_sub_A_C';
my $appname_BA = 'tap_sub_B_A';
my $appname_BC = 'tap_sub_B_C';
my $appname_CA = 'tap_sub_C_A';
my $appname_CB = 'tap_sub_C_B';

~~~

4. create_subscription - passing the $appname 2x.

+create_subscription($node_B, $node_A, $appname_B1, $node_A_connstr,
+       $appname_B1, 'tap_pub_A', 'on');


It seemed confusing that the $app_name is passed twice.

IMO should rename all those $appname_XX vars (see previous comment) to
be $subname_XX, and just pass that create_subscription instead.

my $subname_AB = 'tap_sub_A_B';
my $subname_AC = 'tap_sub_A_C';
my $subname_BA = 'tap_sub_B_A';
my $subname_BC = 'tap_sub_B_C';
my $subname_CA = 'tap_sub_C_A';
my $subname_CB = 'tap_sub_C_B';

Then create_subscription subroutine should have one less argument.
Just add a comment saying that the appname is always assigned the same
value as the subname.

~~~

5. cleanup part - seems a bit misleading

+# cleanup
+$node_B->safe_psql(
+       'postgres', "
+        DROP SUBSCRIPTION $appname_B2");
+$node_C->safe_psql(
+     'postgres', "
+        DELETE FROM tab_full");
+$node_B->safe_psql(
+     'postgres', "
+        DELETE FROM tab_full where a = 13");

Is that comment misleading? IIUC this is not really cleaning up
everything. It is just a cleanup of the previous Node_C test part
isn't it?

~~~

6. Error case (when copy_data is true)

+# Error when creating susbcription with local_only and copy_data as true when
+# the publisher has replicated data

6a. Typo "susbcription"

6b. That comment maybe needs some more explanation - eg. say that
since Node_A is already subscribing to Node_B so when Node_B makes
another subscription to Node_A the copy doesn't really know if the
data really originated from Node_A or not...

6c. Maybe the comment needed to be more like ############## style to
denote this (and the one that follows) is a separate test case.

~~~

7. Error case (when copy_data is force)

+# Creating subscription with local_only and copy_data as force should be
+# successful when the publisher has replicated data

7a. Same typo "subscription"

~~~

8. Add 3rd node when the existing node has some data

8a. Maybe that comment needs to be expanded more. This isn't just
"adding a node" - you are joining it to the others as another
bi-directional participant in a 3-way group. And the comment should
clarify exactly which nodes have the initial data. Both the existing 2
you are joining to? The new one only? All of them?

8b. IMO is would be much clearer to do SELECT from all the nodes at
the start of this test just to re-confirm what is all the initial data
on these nodes before joining the 3rd node.

NOTE - These same review comments apply to the other test combinations too
- Add 3rd node when the existing node has no data
- Add 3rd node when the new node has some data

~~~

9. Inserted data

+# insert some data in all the nodes
+$node_A->safe_psql('postgres', "INSERT INTO tab_full VALUES (13);");
+$node_B->safe_psql('postgres', "INSERT INTO tab_full VALUES (21);");
+$node_C->safe_psql('postgres', "INSERT INTO tab_full VALUES (31);");

They seemed strange values (13, 21, 31) to add. I couldn't work out
the "meaning" of them.

Wouldn't values like 13, 23, 33 make more sense (e.g pattern is 10 x
node# + something)

~~~

10. verify_data($node_A, $node_B, $node_C);

All the expected values are too buried in this subroutine which makes
it hard to read. I think it would be easier to read (from the caller's
POV) if you can pass the *expected* values as another arg into the
verify_data subroutine.

e.g. is something like this possible?
verify_data($node_A, $node_B, $node_C, [11,12,13])

~~~

11. ALTER for ignoring  'truncate'

+$node_C->safe_psql('postgres',
+       "ALTER PUBLICATION tap_pub_C SET (publish='insert,update,delete');");
+
+$node_C->safe_psql('postgres', "TRUNCATE tab_full");
+
+create_subscription($node_C, $node_A, $appname_C1, $node_A_connstr,
+       $appname_C1, 'tap_pub_A', 'force');
+create_subscription($node_C, $node_B, $appname_C2, $node_B_connstr,
+       $appname_C2, 'tap_pub_B', 'off');
+
+#include truncates now
+$node_C->safe_psql('postgres',
+       "ALTER PUBLICATION tap_pub_C SET
(publish='insert,update,delete,truncate');"
+);

Do those create_subscription calls need to be encapsulated by the
ALTER like that? I did not think so. It looks a bit more complex if
done this way.

~~~

12. clean_subscriber_contents - misnamed?

This subroutine feels a bit misnamed. It seems to be doing lots of
things like detaching the Node_C and deleting all table data from all
nodes. That all seems quite different from just "clean subscriber
contents".

~~~

13. table initial data?

+clean_subscriber_contents($node_A, $node_B, $node_C);
+
+##########################################################################
+# Add 3rd node when the new node has some data
+##########################################################################

But does this test case *really* have some data? I am not so sure.
Doesn't the preceding "clean_subscriber_contents" call remove all the
data that might have been there? That is why I think all the tests out
to have SELECT (previous comment #8) so they can re-confirm what data
is really in those tables before doing each test part.


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