Re: Handle infinite recursion in logical replication setup

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

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