On Wed, Apr 20, 2022 at 11:19 AM Peter Smith <smithpb2...@gmail.com> wrote: > > 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.
Added > ~~ > > 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'); Modified > ~~~ > > 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'; Removed appname and used subscription names for application name > ~~~ > > 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. Modifeid > ~~~ > > 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? Modified to clear the operations done by this test > ~~~ > > 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" Modified > 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... Slightly reworded and modified > 6c. Maybe the comment needed to be more like ############## style to > denote this (and the one that follows) is a separate test case. Modified > ~~~ > > 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" I felt subscription is correct in this case, made no change for this > ~~~ > > 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? Modified > 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. Modified > 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 Modified > ~~~ > > 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) Modified > ~~~ > > 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]) Modified > ~~~ > > 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. Modified > ~~~ > > 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". changed it to detach_node_clean_table_data > ~~~ > > 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. Modified Thanks for the comments, the v10 patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm0PmOz71O6ofhZkB0rts5Ak2HUhMuuMQoViH_LAXTBeBw%40mail.gmail.com Regards, Vignesh