On Fri, Jun 24, 2022 at 10:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for the v23* patch set.
>
> ========
> v23-0001
> ========
>
> No comments. LGTM
>
> ========
> V23-0002
> ========
>
> 2.1 src/backend/commands/subscriptioncmds.c
>
> + opts->origin = defGetString(defel);
> +
> + if ((strcmp(opts->origin, LOGICALREP_ORIGIN_LOCAL) != 0) &&
> + (strcmp(opts->origin, LOGICALREP_ORIGIN_ANY) != 0))
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("unrecognized origin value: \"%s\"", opts->origin));
> + }
>
> I thought maybe this should be ERRCODE_INVALID_PARAMETER_VALUE.
Modified
> ~~~
>
> 2.2 src/backend/replication/pgoutput/pgoutput.c
>
> + data->origin = defGetString(defel);
> + if (strcmp(data->origin, LOGICALREP_ORIGIN_LOCAL) == 0)
> + publish_local_origin = true;
> + else if (strcmp(data->origin, LOGICALREP_ORIGIN_ANY) == 0)
> + publish_local_origin = false;
> + else
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("unrecognized origin value: \"%s\"", data->origin));
> + }
>
> I thought maybe this should be ERRCODE_INVALID_PARAMETER_VALUE.
Modified
> ========
> v23-0003
> ========
>
> 3.1 Commit message
>
> After the subscription is created on node2, node1 will be synced to
> node2 and the newly synced data will be sent to node2, this process of
> node1 sending data to node2 and node2 sending data to node1 will repeat
> infinitely. If table t1 has a unique key, this will lead to a unique key
> violation, and replication won't proceed.
>
> ~
>
> 31a.
> "node2, this process of" -> "node2; this process of"
> OR
> "node2, this process of" -> "node2. This process of"
Modified
> 31b.
> Also, my grammar checker recommends removing the comma after "violation"
Modified
> ~~~
>
> 3.2 doc/src/sgml/ref/create_subscription.sgml
>
> @@ -115,7 +115,8 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
> (You cannot combine setting <literal>connect</literal>
> to <literal>false</literal> with
> setting <literal>create_slot</literal>, <literal>enabled</literal>,
> - or <literal>copy_data</literal> to <literal>true</literal>.)
> + or <literal>copy_data</literal> to
> + <literal>true</literal>/<literal>force</literal>.)
> </para>
>
> I am not sure why that last sentence needs to be in parentheses, but
> OTOH it seems to be that way already in PG15.
I feel since it is like that since PG15, let's keep it in parenthesis
like earlier. I have not made any changes for this.
> ~~~
>
> 3.3 doc/src/sgml/ref/create_subscription.sgml
>
> @@ -383,6 +398,15 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
> can have non-existent publications.
> </para>
>
> + <para>
> + If the subscription is created with <literal>origin = local</literal> and
> + <literal>copy_data = true</literal>, it will check if the publisher has
> + subscribed to the same table from other publishers and, if so, then throw an
> + error to prevent possible non-local data from being copied. The user can
> + override this check and continue with the copy operation by specifying
> + <literal>copy_data = force</literal>.
> + </para>
>
> "and, if so, then throw..." -> "and, if so, throw..."
Modified
> ~~~
>
> 3.4 src/backend/commands/subscriptioncmds.c
>
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("%s requires a boolean or \"force\"", def->defname));
> + return COPY_DATA_OFF; /* keep compiler quiet */
> +}
>
> I thought maybe this should be ERRCODE_INVALID_PARAMETER_VALUE.
Modified
> ========
> v23-0004
> ========
>
> 4.1 Commit message
>
> Document the steps for the following:
> a) Creating a two-node bidirectional replication when there is no data
> on both nodes.
> b) Adding a new node when there is no data on any of the nodes.
> c) Adding a new node when data is present on the existing nodes.
> d) Generic steps for adding a new node to an existing set of nodes.
>
> ~
>
> These pgdocs titles have changed slightly. I think this commit message
> text should use match the current pgdocs titles.
Modified
Thanks for the comments, the attached v24 patch has the changes for the same.
Regards,
Vignesh