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

On Thu, Apr 28, 2022 at 7:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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"

Modified

> 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.

cleaning the data in all nodes to keep the test simpler

> ~~~
>
> 2.
>
> +# Subroutine for verify the data is replicated successfully.
> +sub verify_data
> +{
>
> 2a. Typo: "for verify"  -> "to verify"

Modified

> 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.

Modified

> ~~~
>
> 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).

Modified

> ~~~
>
> 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.

Modified

> ~~
>
> 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"?

Modified

> ~~~
>
> 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"?

Modified

The v11 patch attached at [1] has the fixes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm2hRyXc4DKPLnBgem_LHBmy%3DWXFEsm9-xhckye1YXcpPA%40mail.gmail.com

Regards,
Vignesh