Re: Handle infinite recursion in logical replication setup

Here are my review comments for the v30* patches:

========
v30-0001
========

1.1 <general>

I was wondering if it is better to implement a new defGetOrigin method
now instead of just using the defGetString to process the 'origin',
since you may need to do that in future anyway if the 'origin' name is
planned to become specifiable by the user. OTOH maybe you prefer to
change this code later when the time comes. I am not sure what way is
best.

~~~

1.2. src/include/replication/walreceiver.h

@@ -183,6 +183,8 @@ typedef struct
  bool streaming; /* Streaming of large transactions */
  bool twophase; /* Streaming of two-phase transactions at
  * prepare time */
+ char    *origin; /* Only publish data originating from the
+ * specified origin */
  } logical;
  } proto;
 } WalRcvStreamOptions;

Should the new comments be aligned with the other ones?

========
v30-0002
========

2.1 doc/src/sgml/ref/alter_subscription.sgml

+         <para>
+          Refer <xref linkend="sql-createsubscription-notes"/> for the usage of
+          <literal>force</literal> for <literal>copy_data</literal> parameter
+          and its interaction with the <literal>origin</literal> parameter.
          </para>

IMO it's better to say "Refer to the Notes" or "Refer to CREATE
SUBSCRIPTION Notes" instead of just "Refer Notes"

~~~

2.2 doc/src/sgml/ref/create_subscription.sgml

2.2.a
+         <para>
+          Refer <xref linkend="sql-createsubscription-notes"/> for the usage of
+          <literal>force</literal> for <literal>copy_data</literal> parameter
+          and its interaction with the <literal>origin</literal> parameter.
+         </para>

IMO it's better to say "Refer to the Notes" (same as other xref on
this page) instead of "Refer Notes"

2.2.b
@@ -316,6 +324,11 @@ CREATE SUBSCRIPTION <replaceable
class="parameter">subscription_name</replaceabl
           publisher sends changes regardless of their origin. The default is
           <literal>any</literal>.
          </para>
+         <para>
+          Refer <xref linkend="sql-createsubscription-notes"/> for the usage of
+          <literal>force</literal> for <literal>copy_data</literal> parameter
+          and its interaction with the <literal>origin</literal> parameter.
+         </para>

Ditto

~~~

2.3 src/backend/commands/subscriptioncmds.c - DefGetCopyData

+/*
+ * Validate the value specified for copy_data parameter.
+ */
+static CopyData
+DefGetCopyData(DefElem *def)

~~~

2.4

+ /*
+ * If no parameter given, assume "true" is meant.
+ */

Please modify the comment to match the recent push [1].

~~~

2.5 src/test/subscription/t/032_localonly.pl

2.5.a
+# Check Alter subscription ... refresh publication when there is a new
+# table that is subscribing data from a different publication
+$node_A->safe_psql('postgres', "CREATE TABLE tab_full1 (a int PRIMARY KEY)");
+$node_B->safe_psql('postgres', "CREATE TABLE tab_full1 (a int PRIMARY KEY)");

Unfortunately, I think tab_full1 is a terrible table name because in
my screen font the 'l' and the '1' look exactly the same so it just
looks like a typo. Maybe change it to "tab_new" or something?

2.5b
What exactly is the purpose of "full" in all these test table names?
AFAIK "full" is just some name baggage inherited from completely
different tests which were doing full versus partial table
replication. I'm not sure it is relevant here.

========
v30-0002
========

No comments.

------
[1] https://github.com/postgres/postgres/commit/8445f5a21d40b969673ca03918c74b4fbc882bf4

Kind Regards,
Peter Smith.
Fujitsu Australia