Re: Identify missing publications from publisher while create/alter subscription. - Mailing list pgsql-hackers

From vignesh C
Subject Re: Identify missing publications from publisher while create/alter subscription.
Date
Msg-id CALDaNm0SD646kJyb7o7UjmRqGN37bUN0hL_=GLusU+=80Cv5pQ@mail.gmail.com
Whole thread Raw
In response to Re: Identify missing publications from publisher while create/alter subscription.  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Responses Re: Identify missing publications from publisher while create/alter subscription.
Re: Identify missing publications from publisher while create/alter subscription.
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: vignesh C
Date:
Subject: Re: Enhanced error message to include hint messages for redundant options error
Next
From: Tom Lane
Date:
Subject: Re: Regex performance regression induced by match-all code