Re: Handle infinite recursion in logical replication setup

On Mon, Jun 6, 2022 at 2:29 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> 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.

Currently we only support local and any. But this was designed so that
it can be extended to support origin names. Users can provide a
particular origin name to be filtered.
The same was also discussed in pg unconference as mentioned in [1]

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

Modified

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

Modified

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

Modified

Thanks for the comments, the attached v19 patch has the changes for the same.
[1] - https://wiki.postgresql.org/wiki/PgCon_2022_Developer_Unconference#Logical_Replication_Origin_Filtering_and_Consistency

Regards,
Vignesh