Re: Handle infinite recursion in logical replication setup

On Wed, Jun 15, 2022 at 12:09 PM Peter Smith <smithpb2250(at)gmail(dot)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