Re: Identify missing publications from publisher while create/alter subscription.

On Sat, May 1, 2021 at 12:49 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> Thanks for the comments, Attached patch has the fixes for the same.
> Thoughts?

Few more comments on v5:

1) Deletion of below empty new line is spurious:
-
/*
* Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
*

2) I think we could just do as below to save indentation of the code
for validate_publication == true.
static void
+connect_and_check_pubs(Subscription *sub, List *publications,
+ bool validate_publication)
+{
+ char *err;
+
+ if (validate_pulication == false )
+ return;
+
+ /* Load the library providing us libpq calls. */
+ load_file("libpqwalreceiver", false);

3) To be consistent, either we pass in validate_publication to both
connect_and_check_pubs and check_publications, return immediately from
them if it is false or do the checks outside. I suggest to pass in the
bool parameter to check_publications like you did for
connect_and_check_pubs. Or remove validate_publication from
connect_and_check_pubs and do the check outside.
+ if (validate_publication)
+ check_publications(wrconn, publications);
+ if (check_pub)
+ check_publications(wrconn, sub->publications);

4) Below line of code is above 80-char limit:
+ else if (strcmp(defel->defname, "validate_publication") == 0
&& validate_publication)

5) Instead of adding a new file 021_validate_publications.pl for
tests, spawning a new test database which would make the overall
regression slower, can't we add with the existing database nodes in
0001_rep_changes.pl? I would suggest adding the tests in there even if
the number of tests are many, I don't mind.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com