On Mon, 29 Aug 2022 at 16:35, Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Wed, Aug 24, 2022 at 7:27 PM vignesh C <vignes...@gmail.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.
I'm analysing this, I will post a reply for this soon.
> 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.
Yes that was the reason, now I have changed it to display the
publications based on your recent comment whose count will be
significantly very much lesser compared to the number of tables.
> 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.
Modified
> 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?
Earlier we felt with the origin patch we could support joining of
nodes to existing n-wary replication setup in various scenarios, I had
added the tests for the same scenarios. But as the patch evolved, I
could understand that this patch cannot handle the add node scenarios.
I have removed these tests.
Thanks for the comments, the v44 patch attached at [1] has the changes
for the same.
[1] -
https://www.postgresql.org/message-id/CALDaNm0NRJ1O1cYcZD%3Df7NgynozFprb7zpJSayFN5rcaS44G6Q%40mail.gmail.com
Regards,
Vignesh
