Re: Handle infinite recursion in logical replication setup

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.

~~~

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.

========
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"

31b.
Also, my grammar checker recommends removing the comma after "violation"

~~~

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.

~~~

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..."

~~~

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.

========
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.

------
Kind Regards,
Peter Smith.
Fujitsu Australia