Re: Handle infinite recursion in logical replication setup

On Fri, May 20, 2022 at 3:08 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, May 18, 2022 at 10:29 AM Amit Kapila <amit.kapila16@gmail.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.
  */

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

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.

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

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.

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.

-- 
With Regards,
Amit Kapila.