RE: Handle infinite recursion in logical replication setup

On Mon, Jun 6, 2022 1:14 PM vignesh C <vignesh21@gmail.com> wrote:
> 
> The attached v18 patch has the fixes for the same.
> 

Thanks for updating the patch, here are some comments.

0002 patch
==============
1. 
+       <varlistentry>
+        <term><literal>origin</literal> (<type>string</type>)</term>
+        <listitem>
+         <para>

It maybe better if the type of  "origin" parameter is enum,  as it cannot be any
string and only has two valid values.

2.
@@ -607,6 +626,11 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 					 LOGICALREP_TWOPHASE_STATE_PENDING :
 					 LOGICALREP_TWOPHASE_STATE_DISABLED);
 	values[Anum_pg_subscription_subdisableonerr - 1] = BoolGetDatum(opts.disableonerr);
+	if (opts.origin)
+		values[Anum_pg_subscription_suborigin - 1] =
+			CStringGetTextDatum(opts.origin);
+	else
+		nulls[Anum_pg_subscription_suborigin - 1] = true;
 	values[Anum_pg_subscription_subconninfo - 1] =
 		CStringGetTextDatum(conninfo);
 	if (opts.slot_name)

Document of "CREATE SUBSCRIPTION" says, the default value of "origin" is "any", so why not set
suborigin to "any" when user doesn't specify this parameter?


0003 patch
==============
1.
@@ -300,6 +310,11 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
           replication from the publisher. The default is
           <literal>false</literal>.
          </para>
+         <para>
+          There is some interaction between the <literal>origin</literal>
+          parameter and <literal>copy_data</literal> parameter. Refer to the
+          <xref linkend="sql-createsubscription-notes" /> for details.
+         </para>
         </listitem>
        </varlistentry>

I think this change should be put together with "origin" parameter, instead of
"disable_on_error".


0004 patch
==============
1.
+   <para>
+    Now the bidirectional logical replication setup is complete between
+    <literal>node1</literal>, <literal>node2</literal> and
+    <literal>node2</literal>. Any subsequent changes in one node will
+    replicate the changes to the other nodes.
+   </para>

I think "node1, node2 and node2" should be "node1, node2 and node3".

Regards,
Shi yu