Re: Handle infinite recursion in logical replication setup
От | vignesh C |
---|---|
Тема | Re: Handle infinite recursion in logical replication setup |
Дата | |
Msg-id | CALDaNm22iFMnc4Mm+fbork=gcgthcYRiQcZH_baoSaNg_M-BWg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Handle infinite recursion in logical replication setup (Amit Kapila <amit.kapila16@gmail.com>) |
Ответы |
RE: Handle infinite recursion in logical replication setup
("shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>)
Re: Handle infinite recursion in logical replication setup (Peter Smith <smithpb2250@gmail.com>) Re: Handle infinite recursion in logical replication setup (Peter Smith <smithpb2250@gmail.com>) |
Список | pgsql-hackers |
On Mon, May 23, 2022 at 10:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > 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. > */ 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
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Nathan BossartДата:
Сообщение: Re: allow building trusted languages without the untrusted versions