Re: Handle infinite recursion in logical replication setup

On Thu, May 26, 2022 at 4:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 20, 2022 at 3:31 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, May 18, 2022 at 4:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > 5.
> > >  * It is quite possible that subscriber has not yet pulled data to
> > > + * the tables, but in ideal cases the table data will be subscribed.
> > > + * To keep the code simple it is not checked if the subscriber table
> > > + * has pulled the data or not.
> > > + */
> > > + if (copydata == COPY_DATA_ON && local_only && !slot_attisnull(slot, 3))
> > >
> > > Sorry, but I don't understand what you intend to say by the above
> > > comment. Can you please explain?
> >
> > When the user specifies copy_data as on, we should check if the
> > publisher has the publication tables being subscribed from a remote
> > publisher. If so throw an error as it remote origin data present.
> > Ex:
> > Node1 - pub1 for table t1 -- no data
> > Node2 - Sub1 subscribing to data from pub1
> > Node2 - pub2 for table t1 -- no data
> > Node3 - create subscription to Node2 with copy_data = ON
> >
> > In this case even though the table does not have any remote origin
> > data,  as Node2 is subscribing to data from Node1, throw an error.
> > We throw an error irrespective of data present in Node1 or not to keep
> > the code simple.
> >
>
> I think we can change the contents of comments something like: "Throw
> an error if the publisher has subscribed to the same table from some
> other publisher. We cannot differentiate between the local and
> non-local data that is present in the HEAP during the initial sync.
> Identification of local data can be done only from the WAL by using
> the origin id.  XXX: For simplicity, we don't check whether the table
> has any data or not. If the table doesn't have any data then we don't
> need to distinguish between local and non-local data so we can avoid
> throwing error in that case."

Modified

> Few more comments:
> ==================
> Patch 0002
> ======
> 1.
> + if (copydata == COPY_DATA_ON && only_local && !slot_attisnull(slot, 3))
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("CREATE/ALTER SUBSCRIPTION with only_local and copy_data as
> true is not allowed when the publisher might have replicated data,
> table:%s.%s might have replicated data in the publisher",
> +    nspname, relname),
> + errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off or force"));
>
> Can we split the error message? errmsg: table:%s.%s has replicated
> data in the publisher; errdetail:CREATE/ALTER SUBSCRIPTION with
> only_local and copy_data as true is not allowed when the publisher has
> replicated data

Modified

> 2.
> +   <para>
> +    Lock the required tables in the new node until the setup is complete.
> +   </para>
> +   <para>
> +    Create subscriptions on existing nodes pointing to publication on
> +    the new node with <literal>only_local</literal> option specified as
> +    <literal>on</literal> and <literal>copy_data</literal> specified as
> +    <literal>on</literal>.
> +   </para>
> +   <para>
> +    Wait for data to be copied from the new node to existing nodes.
> +   </para>
> +   <para>
> +    Alter the publication in new node so that the truncate operation is not
> +    replicated to the subscribers.
> +   </para>
>
> Here and at other places, we should specify that the lock mode should
> to acquire the lock on table should be EXCLUSIVE so that no concurrent
> DML is allowed on it. Also, it is better if somewhere we explain why
> and which nodes need locks?

Modified

> Patch 0001:
> ==========
> 1.
> +$node_A->append_conf(
> + 'postgresql.conf', qq(
> +max_prepared_transactions = 10
> +logical_decoding_work_mem = 64kB
> +));
>
> I don't see why you need to set these parameters. There is no test
> case that needs these parameters. Please remove these from here and
> all other similar places in 032_onlylocal.pl.

Removed it.

Thanks for the comments, the v17 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm1rMihO7daiFyLdxkqArrC%2BdtM61oPXc-XrTYBYnJg3nw%40mail.gmail.com

Regards,
Vignesh