On Sat, May 1, 2021 at 7:58 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sat, May 1, 2021 at 12:49 PM vignesh C <vignesh21@gmail.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.
> *
>
Modified.
> 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);
>
Modified.
> 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);
>
Modified.
> 4) Below line of code is above 80-char limit:
> + else if (strcmp(defel->defname, "validate_publication") == 0
> && validate_publication)
>
Modified
> 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.
001_rep_changes.pl has the changes mainly for checking the replicated
data. I did not find an appropriate file in the current tap tests, I
preferred these tests to be in a separate file. Thoughts?
Thanks for the comments.
The Attached patch has the fixes for the same.
Regards,
Vignesh
