On Fri, Feb 11, 2022 at 7:14 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> I have spent little time looking at the latest patch. The patch looks
> to be in good shape as it has already been reviewed by many people
> here, although I did get some comments. Please take a look and let me
> know your thoughts.
>
>
> + /* Try to connect to the publisher. */
> + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> + if (!wrconn)
> + ereport(ERROR,
> + (errmsg("could not connect to the publisher: %s", err)));
>
> I think it would be good to also include the errcode
> (ERRCODE_CONNECTION_FAILURE) here?
Modified
> --
>
> @@ -514,6 +671,8 @@ CreateSubscription(ParseState *pstate,
> CreateSubscriptionStmt *stmt,
>
> PG_TRY();
> {
> + check_publications(wrconn, publications, opts.validate_publication);
> +
>
>
> Instead of passing the opts.validate_publication argument to
> check_publication function, why can't we first check if this option is
> set or not and accordingly call check_publication function? For other
> options I see that it has been done in the similar way for e.g. check
> for opts.connect or opts.refresh or opts.enabled etc.
Modified
> --
>
> Above comment also applies for:
>
> @@ -968,6 +1130,8 @@ AlterSubscription(ParseState *pstate,
> AlterSubscriptionStmt *stmt,
> replaces[Anum_pg_subscription_subpublications - 1] = true;
>
> update_tuple = true;
> + connect_and_check_pubs(sub, stmt->publication,
> + opts.validate_publication);
>
Modified
> --
>
> + <para>
> + When true, the command verifies if all the specified publications
> + that are being subscribed to are present in the publisher and throws
> + an error if any of the publication doesn't exist. The default is
> + <literal>false</literal>.
>
> publication -> publications (in the 4th line : throw an error if any
> of the publication doesn't exist)
>
> This applies for both CREATE and ALTER subscription commands.
Modified
Thanks for the comments, the attached v14 patch has the changes for the same.
Regard,s
Vignesh
