Re: Handle infinite recursion in logical replication setup

On Wed, Aug 24, 2022 at 7:27 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> Since there was no objections to change it to throw a warning, I have
> made the changes for the same.
>

Review comments for v42-0001*
==========================
1. Can we improve the query in check_pub_table_subscribed() so that it
doesn't fetch any table that is already part of the subscription on
the subscriber? This may be better than what the patch is doing which
is to first fetch such information and then skip it. If forming this
query turns out to be too complex then we can retain your method as
well but I feel it is worth trying to optimize the query used in the
patch.

2. I thought that it may be better to fetch all the tables that have
the possibility to have data from more than one origin but maybe the
list will be too long especially for FOR ALL TABLE types of cases. Is
this the reason you have decided to give a WARNING as soon as you see
any such table? If so, probably adding a comment for it can be good.

3.
+ $node_publisher->wait_for_catchup($sub_name);
+
+ # also wait for initial table sync to finish
+ $node_subscriber->poll_query_until('postgres', $synced_query)
+ or die "Timed out while waiting for subscriber to synchronize data";
....
....
....
+$node_B->safe_psql(
+ 'postgres', "
+ ALTER SUBSCRIPTION $subname_BA REFRESH PUBLICATION");
+
+$node_B->poll_query_until('postgres', $synced_query)
+ or die "Timed out while waiting for subscriber to synchronize data";

You can replace this and any similar code in the patch with a method
used in commit 0c20dd33db.

4.
+###############################################################################
+# 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 pre-existing
+# data and the new node (node_C) does not have any data.
+###############################################################################
+$result = $node_A->safe_psql('postgres', "SELECT * FROM tab ORDER BY 1;");
+is($result, qq(), 'Check existing data');
+
+$result = $node_B->safe_psql('postgres', "SELECT * FROM tab ORDER BY 1;");
+is($result, qq(), 'Check existing data');
+
+$result = $node_C->safe_psql('postgres', "SELECT * FROM tab ORDER BY 1;");
+is($result, qq(), 'Check existing data');

The comments say that for this test, two of the nodes have some
pre-existing data but I don't see the same in the test results. The
test following this one will also have a similar effect. BTW, I am not
sure all the new three node tests added by this patch are required
because I don't see if they have additional code coverage or test
anything which is not tested without those. This has doubled the
amount of test timing for this test file which is okay if these tests
make any useful contribution but otherwise, it may be better to remove
these. Am, I missing something?

--
With Regards,
Amit Kapila.