On Tue, Jun 28, 2022 at 7:29 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for the v24* patch set.
>
> Now, these few comments are all trivial and non-functional. Apart from
> these, everything looks good to me.
>
> ========
> v24-0001
> ========
>
> No comments. LGTM
>
> ========
> V24-0002
> ========
>
> 2.1 doc/src/sgml/ref/create_subscription.sgml
>
> + <para>
> + Specifies whether the subscription will request the publisher to only
> + send changes that originated locally, or to send any changes
> + regardless of origin. Setting <literal>origin</literal> to
> + <literal>local</literal> means that the subscription will request the
> + publisher to only send changes that originated locally. Setting
> + <literal>origin</literal> to <literal>any</literal> means that the
> + publisher sends any changes regardless of their origin. The default
> + is <literal>any</literal>.
> + </para>
>
> 2.1a.
> IMO remove the word "any" from "any changes". Then the text will match
> what is written in catalogs.sgml.
> "send any changes regardless of origin" -> "send changes regardless of
> origin" (occurs 2x)
Modified
> 2.1b.
> This same text is cut/paste to the commit message so that can also be updated.
Modified
>
> ~~~
>
> 2.2 src/backend/commands/subscriptioncmds.c
>
> + /*
> + * Even though "origin" parameter allows only "local" and "any"
> + * values, the "origin" parameter type is implemented as string
> + * type instead of boolean to extend the "origin" parameter to
> + * support filtering of origin name specified by the user in the
> + * later versions.
> + */
>
> 2.2a.
> SUGGESTION
> Even though "origin" parameter allows only "local" and "any" values,
> it is implemented as a string type so that the parameter can be
> extended in future versions to support filtering using origin names
> specified by the user.
Modified
> 2.2b.
> This same text is cut/paste to the commit message so that can also be updated.
Modified
>
> ========
> v24-0003
> ========
>
> 3.1 Commit message
>
> v24-0004
> ========
>
> No comments. LGTM
>
> ------
> Kind Regards,
> Peter Smith.
> This patch does a couple of things:
> change 1) Checks and throws an error if 'copy_data = on' and 'origin =
> local' but the publication tables were also replicating from other publishers.
> change 2) Adds 'force' value for copy_data parameter.
>
> ~
>
> "replicating" -> "replicated"
> "change 1)" -> "1)"
> "change 2)" -> "2)"
Modified
Thanks for the comments, the attached v25 patch has the changes for the same.
Regards,
Vignesh