RE: Handle infinite recursion in logical replication setup

On Thu, Jun 16, 2022 6:18 PM vignesh C <vignesh21(at)gmail(dot)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