RE: Handle infinite recursion in logical replication setup

On Thu, Jun 16, 2022 6:18 PM vignesh C <vignesh21@gmail.com> wrote:
> 
> Thanks for the comments, the attached v21 patch has the changes for the
> same.
> 

Thanks for updating the patch. Here are some comments.

0002 patch
==============
1.
+          publisher to only send changes that originated locally. Setting
+          <literal>origin</literal> to <literal>any</literal> means that that
+          the publisher sends any changes regardless of their origin. The
+          default is <literal>any</literal>.

It seems there's a redundant "that" at the end of second line.

2.
+    <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>suborigin</structfield> <type>text</type>
+      </para>
+      <para>
+       Possible origin values are <literal>local</literal> or
+       <literal>any</literal>. The default is <literal>any</literal>.
+       If <literal>local</literal>, the subscription will request the publisher
+       to only send changes that originated locally. If <literal>any</literal>
+       the publisher sends any changes regardless of their origin.
+      </para></entry>
+     </row>.

A comma can be added after "If any".

3.
@@ -4589,6 +4598,8 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo)
     if (strcmp(subinfo->subdisableonerr, "t") == 0)
         appendPQExpBufferStr(query, ", disable_on_error = true");
 
+    appendPQExpBuffer(query, ", origin = %s", subinfo->suborigin);
+
     if (strcmp(subinfo->subsynccommit, "off") != 0)
         appendPQExpBuffer(query, ", synchronous_commit = %s", fmtId(subinfo->subsynccommit));

Do we need to append anything if it's the default value ("any")? I saw that some
other parameters, they will be appended only if they are not the default value.

0003 patch
==============
1. 
in create_subscription.sgml:
          (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>.)

In this description about "connect" parameter in CREATE SUBSCIPTION document,
maybe it would be better to change "copy_data to true" to "copy_data to
true/force".

2.
+    appendStringInfoString(&cmd,
+                           "SELECT DISTINCT N.nspname AS schemaname,\n"
+                           "                C.relname AS tablename,\n"
+                           "                PS.srrelid as replicated\n"
+                           "FROM pg_publication P,\n"
+                           "     LATERAL pg_get_publication_tables(P.pubname) GPT\n"
+                           "     LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
+                           "     pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
+                           "WHERE C.oid = GPT.relid AND P.pubname IN (");

"PS.srrelid as replicated" can be modified to "PS.srrelid AS replicated".

Besides, I think we can filter out the tables which are not subscribing data in
this SQL statement, then later processing can be simplified.

Something like:
SELECT DISTINCT N.nspname AS schemaname,
                                C.relname AS tablename
FROM pg_publication P,
         LATERAL pg_get_publication_tables(P.pubname) GPT
         LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),
         pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE C.oid = GPT.relid AND P.pubname IN ('pa') AND PS.srrelid IS NOT NULL;

0004 patch
==============
1. Generic steps for adding a new node to an existing set of nodes

+    Step-2: Lock the required tables of the new node in EXCLUSIVE mode until
+    the setup is complete. (This lock is necessary to prevent any modifications

+    Step-4. Lock the required tables of the existing nodes except the first node
+    in EXCLUSIVE mode until the setup is complete. (This lock is necessary to

Should "in EXCLUSIVE mode" be modified to "in <literal>EXCLUSIVE</literal>
mode"?

2. Generic steps for adding a new node to an existing set of nodes

+    data. There is no need to lock the required tables on
+    <literal>node1</literal> because any data changes made will be synchronized
+    while creating the subscription with <literal>copy_data = force</literal>).

I think it would be better to say "on the first node" here, instead of "node1",
because in this section, node1 is not mentioned before.

Regards,
Shi yu