Re: Handle infinite recursion in logical replication setup

On Mon, Jun 20, 2022 at 9:22 AM shiy(dot)fnst(at)fujitsu(dot)com
<shiy(dot)fnst(at)fujitsu(dot)com> wrote:
>
> 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.

Modified

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

Modified

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

Modified

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

Modified

> 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;

Modified

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

Modified

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

Modified

Thanks for the comment, the v22 patch attached has the changes for the same.

Regards,
Vignesh