Re: Handle infinite recursion in logical replication setup

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.

======

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.

======

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.

======

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.

~~~

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);

======

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.

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?

======

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.

======

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?

~~~

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.

~~~

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.

======

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.

======

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.

~~~

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.

------
Kind Regards,
Peter Smith.
Fujitsu Australia