Re: Handle infinite recursion in logical replication setup

On Mon, May 23, 2022 at 10:01 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, May 20, 2022 at 3:08 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Wed, May 18, 2022 at 10:29 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > >
> > > Few comments on v13-0001
> > > ======================
> > > 1.
> > > + *
> > > + * FIXME: LOGICALREP_PROTO_LOCALONLY_VERSION_NUM needs to be bumped to 4 in
> > > + * PG16.
> > > ...
> > > @@ -477,6 +489,12 @@ pgoutput_startup(LogicalDecodingContext *ctx,
> > > OutputPluginOptions *opt,
> > > else
> > > ctx->twophase_opt_given = true;
> > >
> > > + if (data->local_only && data->protocol_version <
> > > LOGICALREP_PROTO_LOCALONLY_VERSION_NUM)
> > > + ereport(ERROR,
> > > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > + errmsg("requested proto_version=%d does not support local_only, need
> > > %d or higher",
> > > + data->protocol_version, LOGICALREP_PROTO_LOCALONLY_VERSION_NUM)));
> > >
> > > What is the need to change the protocol version for this parameter? As
> > > per my understanding, we only need a different protocol version when
> > > we are sending some new message or some additional information in an
> > > existing message as we have done for the streaming/two_phase options
> > > which doesn't seem to be the case here.
> >
> > Modified
> >
>
> It seems you forgot to remove the comments after removing the code
> corresponding to the above. See below.
> + *
> + * LOGICALREP_PROTO_LOCALONLY_VERSION_NUM is the minimum protocol version with
> + * support for sending only locally originated data from the publisher.
> + * Introduced in PG16.
> + *
> + * FIXME: LOGICALREP_PROTO_LOCALONLY_VERSION_NUM needs to be bumped to 4 in
> + * PG16.
> */

Removed it.

> Few other comments on 0001
> ========================
> 1.
> + * Return true if data has originated remotely when only_local option is
> + * enabled, false otherwise.
>
> Can we slightly change the comment to:"Return true if the data source
> (origin) is remote and user has requested only local data, false
> otherwise."

Modified

> 2.
> +SELECT pg_replication_origin_session_reset();
> +SELECT pg_drop_replication_slot('regression_slot_only_local');
> +SELECT pg_replication_origin_drop('regress_test_decoding:
> regression_slot_only_local');
> \ No newline at end of file
>
> At the end of the file, there should be a new line.

I checked the other regression sql files and found that a newline is
not added at the end of the file. I did not make any change for this.
Thoughts?

> 3.
> + <structfield>subonlylocal</structfield> <type>bool</type>
> + </para>
> + <para>
> + If true, subscription will request the publisher to send locally
> + originated changes at the publisher node, or send any publisher node
> + changes regardless of their origin
>
> The case where this option is not set is not clear from the
> description. Can we change the description to: "If true, the
> subscription will request that the publisher send locally originated
> changes. False indicates that the publisher sends any changes
> regardless of their origin."

Modified

> 4. This new option 'subonlylocal' is placed before 'substream' in docs
> (catalogs.sgml) and after it in structures in pg_subscription.h. I
> suggest adding it after 'subdisableonerr' in docs and
> pg_subscription.h. Also, adjust other places in subscriber-side code
> to place it consistently.

Modified

> 5.
> @@ -4516,6 +4524,8 @@ getSubscriptions(Archive *fout)
> pg_strdup(PQgetvalue(res, i, i_subtwophasestate));
> subinfo[i].subdisableonerr =
> pg_strdup(PQgetvalue(res, i, i_subdisableonerr));
> + subinfo[i].subonlylocal =
> + pg_strdup(PQgetvalue(res, i, i_subonlylocal));
>
> /* Decide whether we want to dump it */
> selectDumpableObject(&(subinfo[i].dobj), fout);
> @@ -4589,6 +4599,9 @@ dumpSubscription(Archive *fout, const
> SubscriptionInfo *subinfo)
> if (strcmp(subinfo->subdisableonerr, "t") == 0)
> appendPQExpBufferStr(query, ", disable_on_error = true");
>
> + if (strcmp(subinfo->subonlylocal, "t") == 0)
> + appendPQExpBufferStr(query, ", only_local = true");
> +
> if (strcmp(subinfo->subsynccommit, "off") != 0)
> appendPQExpBuffer(query, ", synchronous_commit = %s",
> fmtId(subinfo->subsynccommit));
>
> diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
> index 1d21c2906f..ddb855fd16 100644
> --- a/src/bin/pg_dump/pg_dump.h
> +++ b/src/bin/pg_dump/pg_dump.h
> @@ -661,6 +661,7 @@ typedef struct _SubscriptionInfo
> char *subdisableonerr;
> char *subsynccommit;
> char *subpublications;
> + char *subonlylocal;
> } SubscriptionInfo;
>
> To keep this part of the code consistent, I think it is better to
> place 'subonlylocal' after 'subdisableonerr' in SubscriptionInfo.

Modified

Thanks for the comments, the attached v15 patch has the fixes for the same.

Regards,
Vignesh