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

On Wed, Apr 20, 2022 at 11:19 AM Peter Smith <smithpb2250@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