Re: Handle infinite recursion in logical replication setup
От | vignesh C |
---|---|
Тема | Re: Handle infinite recursion in logical replication setup |
Дата | |
Msg-id | CALDaNm3+6cey0rcDft1ZUCjSUtLDM0xmU_Q+YhcsBrqe1RH8=w@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Handle infinite recursion in logical replication setup (Peter Smith <smithpb2250@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 (Dilip Kumar <dilipbalaut@gmail.com>) |
Список | pgsql-hackers |
On Wed, Jun 15, 2022 at 12:09 PM Peter Smith <smithpb2250@gmail.com> wrote: > > Here are some review comments for patch v20-0002 > > ====== > > 1. General comment > > Something subtle but significant changed since I last reviewed v18*. > Now the describe.c is changed so that the catalog will never display a > NULL origin column; it would always be "any". But now I am not sure if > it is a good idea to still allow the NULL in this catalog column while > at the same time you are pretending it is not there. I felt it might > be less confusing, and would simplify the code (e.g. remove quite a > few null checks) to have just used a single concept of the default - > e.g. Just assign the default as "any" everywhere. The column would be > defined as NOT NULL. Most of the following review comments are related > to this point. Ok, I was initially feeling having NULL value will help in the upgrade cases where we upgrade from a lower version which does not have origin to current. But setting the default value handles the upgrade scenario too. > ====== > > 2. doc/src/sgml/catalogs.sgml > > + <para> > + Possible origin values are <literal>local</literal>, > + <literal>any</literal>, or <literal>NULL</literal> if none is specified. > + If <literal>local</literal>, the subscription will request the > + publisher to only send changes that originated locally. If > + <literal>any</literal> (or <literal>NULL</literal>), the publisher sends > + any changes regardless of their origin. > + </para></entry> > > Is NULL still possible? Perhaps it would be better if it was not and > the default "any" was always written instead. Modified > ====== > > 3. src/backend/catalog/pg_subscription.c > > + if (!isnull) > + sub->origin = TextDatumGetCString(datum); > + else > + sub->origin = NULL; > + > > Maybe better to either disallow NULL in the first place or assign the > "any" here instead of NULL. Modified > ====== > > 4. src/backend/commands/subscriptioncmds.c - parse_subscription_options > > @@ -137,6 +139,8 @@ parse_subscription_options(ParseState *pstate, > List *stmt_options, > opts->twophase = false; > if (IsSet(supported_opts, SUBOPT_DISABLE_ON_ERR)) > opts->disableonerr = false; > + if (IsSet(supported_opts, SUBOPT_ORIGIN)) > + opts->origin = NULL; > > If opt->origin was assigned to "any" then other code would be simplified. Modified > ~~~ > > 5. src/backend/commands/subscriptioncmds.c - CreateSubscription > > @@ -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 > + values[Anum_pg_subscription_suborigin - 1] = > CStringGetTextDatum(LOGICALREP_ORIGIN_ANY); > > If NULL was not possible then this would just be one line: > values[Anum_pg_subscription_suborigin - 1] = CStringGetTextDatum(opts.origin); Modified > ====== > > 6. src/backend/replication/logical/worker.c > > @@ -276,6 +276,10 @@ static TransactionId stream_xid = InvalidTransactionId; > static XLogRecPtr skip_xact_finish_lsn = InvalidXLogRecPtr; > #define is_skipping_changes() > (unlikely(!XLogRecPtrIsInvalid(skip_xact_finish_lsn))) > > +/* Macro for comparing string fields that might be NULL */ > +#define equalstr(a, b) \ > + (((a) != NULL && (b) != NULL) ? (strcmp((a), (b)) == 0) : (a) == (b)) > + > > If the NULL was not allowed in the first place then I think this macro > would just become redundant. Removed it > 7. src/backend/replication/logical/worker.c - ApplyWorkerMain > > @@ -3741,6 +3746,11 @@ ApplyWorkerMain(Datum main_arg) > options.proto.logical.streaming = MySubscription->stream; > options.proto.logical.twophase = false; > > + if (MySubscription->origin) > + options.proto.logical.origin = pstrdup(MySubscription->origin); > + else > + options.proto.logical.origin = NULL; > + > > Can't the if/else be avoided if you always assigned the "any" default > in the first place? Modified > ====== > > 8. src/backend/replication/pgoutput/pgoutput.c - parse_output_parameters > > @@ -287,11 +289,13 @@ parse_output_parameters(List *options, PGOutputData *data) > bool messages_option_given = false; > bool streaming_given = false; > bool two_phase_option_given = false; > + bool origin_option_given = false; > > data->binary = false; > data->streaming = false; > data->messages = false; > data->two_phase = false; > + data->origin = NULL; > > Consider assigning default "any" here instead of NULL. This is no more required because of setting default to any and the same value will be passed as an option > ====== > > 9. src/bin/pg_dump/pg_dump.c - getSubscriptions > > + /* FIXME: 150000 should be changed to 160000 later for PG16. */ > + if (fout->remoteVersion >= 150000) > + appendPQExpBufferStr(query, " s.suborigin\n"); > + else > + appendPQExpBufferStr(query, " NULL AS suborigin\n"); > + > > Maybe say: 'any' AS suborigin? Modified > ~~~ > > 10. src/bin/pg_dump/pg_dump.c - getSubscriptions > > @@ -4517,6 +4525,11 @@ getSubscriptions(Archive *fout) > subinfo[i].subdisableonerr = > pg_strdup(PQgetvalue(res, i, i_subdisableonerr)); > > + if (PQgetisnull(res, i, i_suborigin)) > + subinfo[i].suborigin = NULL; > + else > + subinfo[i].suborigin = pg_strdup(PQgetvalue(res, i, i_suborigin)); > + > > If you disallow the NULL in the first place this condition maybe is no > longer needed. Modified > ~~~ > > 11. src/bin/pg_dump/pg_dump.c - dumpSubscription > > @@ -4589,6 +4602,9 @@ dumpSubscription(Archive *fout, const > SubscriptionInfo *subinfo) > if (strcmp(subinfo->subdisableonerr, "t") == 0) > appendPQExpBufferStr(query, ", disable_on_error = true"); > > + if (subinfo->suborigin) > + appendPQExpBuffer(query, ", origin = %s", subinfo->suborigin); > + > > If NULL cannot happen then maybe this test is also redundant. Modified > ====== > > 12. src/bin/pg_dump/t/002_pg_dump.pl > > AFAICT there is a test for no origin (default), and a test for > explicit origin = local, but there is no test case for the explicit > origin = any. Added a test for the same > ====== > > 13. src/include/catalog/pg_subscription.h > > @@ -31,6 +31,9 @@ > #define LOGICALREP_TWOPHASE_STATE_PENDING 'p' > #define LOGICALREP_TWOPHASE_STATE_ENABLED 'e' > > +#define LOGICALREP_ORIGIN_LOCAL "local" > +#define LOGICALREP_ORIGIN_ANY "any" > + > > I thought there should be a comment above these new constants. Added comments > ~~~ > > 14. src/include/catalog/pg_subscription.h > > @@ -87,6 +90,9 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId) > BKI_SHARED_RELATION BKI_ROW > > /* List of publications subscribed to */ > text subpublications[1] BKI_FORCE_NOT_NULL; > + > + /* Only publish data originating from the specified origin */ > + text suborigin; > #endif > } FormData_pg_subscription; > > Perhaps it would be better if this new column was also forced to be NOT NULL. I have set a default value so need to set NOT NULL. Thanks for the comments, the attached v21 patch has the changes for the same. Regards, Vignesh
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Amit KapilaДата:
Сообщение: Re: Replica Identity check of partition table on subscriber