Re: logical decoding and replication of sequences, take 2

And the last patch 0008.

@@ -1180,6 +1194,13 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
... snip ...
+ if (IsSet(opts.specified_opts, SUBOPT_SEQUENCES))
+ {
+ values[Anum_pg_subscription_subsequences - 1] =
+ BoolGetDatum(opts.sequences);
+ replaces[Anum_pg_subscription_subsequences - 1] = true;
+ }
+

The list of allowed options set a few lines above this code does not contain
"sequences". Is this option missing there or this code is unnecessary? If we
intend to add "sequence" at a later time after a subscription is created, will
the sequences be synced after ALTER SUBSCRIPTION?

+ /*
+ * ignore sequences when not requested
+ *
+ * XXX Maybe we should differentiate between "callbacks not defined" or
+ * "subscriber disabled sequence replication" and "subscriber does not
+ * know about sequence replication" (e.g. old subscriber version).
+ *
+ * For the first two it'd be fine to bail out here, but for the last it

It's not clear which two you are talking about. Maybe that's because the
paragraph above is ambiguious. It is in the form of A or B and C; so not clear
which cases we are differentiating between: (A, B, C), ((A or B) and C) or (A or
(B and C)) or something else.

+ * might be better to continue and error out only when the sequence
+ * would be replicated (e.g. as part of the publication). We don't know
+ * that here, unfortunately.

Please see comments on changes to pgoutput_startup() below. We may
want to change the paragraph accordingly.

@@ -298,6 +298,20 @@ StartupDecodingContext(List *output_plugin_options,
*/
ctx->reorder->update_progress_txn = update_progress_txn_cb_wrapper;

+ /*
+ * To support logical decoding of sequences, we require the sequence
+ * callback. We decide it here, but only check it later in the wrappers.
+ *
+ * XXX Isn't it wrong to define only one of those callbacks? Say we
+ * only define the stream_sequence_cb() - that may get strange results
+ * depending on what gets streamed. Either none or both?

I don't think the current condition is correct; it will consider sequence
changes to be streamed even when sequence_cb is not defined and actually not
send those. sequence_cb is needed to send sequence changes irrespective of
whether transaction streaming is supported. But stream_sequence_cb is required
if other stream callbacks are available. Something like

if (ctx->callbacks.sequence_cb)
{
if (ctx->streaming)
{
if ctx->callbacks.stream_sequence_cb == NULL)
ctx->sequences = false;
else
ctx->sequences = true;
}
else
ctx->sequences = true;
}
else
ctx->sequences = false;

+ *
+ * XXX Shouldn't sequence be defined at slot creation time, similar
+ * to two_phase? Probably not.

I don't know why two_phase is defined at the slot creation time, so can't
comment on this. But looks like something we need to answer before committing
the patches.

+ /*
+ * We allow decoding of sequences when the option is given at the streaming
+ * start, provided the plugin supports all the callbacks for two-phase.

s/two-phase/sequences/

+ *
+ * XXX Similar behavior to the two-phase block below.

I think we need to describe sequence specific behaviour instead of pointing to
the two-phase. two-phase is part of in replication slot's on disk specification
but sequence is not. Given that it's XXX, I think you are planning to do that.

+ *
+ * XXX Shouldn't this error out if the callbacks are not defined?

Isn't this already being done in pgoutput_startup()? Should we remove this XXX.

+ /*
+ * Here, we just check whether the sequences decoding option is passed
+ * by plugin and decide whether to enable it at later point of time. It
+ * remains enabled if the previous start-up has done so. But we only
+ * allow the option to be passed in with sufficient version of the
+ * protocol, and when the output plugin supports it.
+ */
+ if (!data->sequences)
+ ctx->sequences_opt_given = false;
+ else if (data->protocol_version <
LOGICALREP_PROTO_SEQUENCES_VERSION_NUM)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("requested proto_version=%d does not
support sequences, need %d or higher",
+ data->protocol_version,
LOGICALREP_PROTO_SEQUENCES_VERSION_NUM)));
+ else if (!ctx->sequences)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("sequences requested, but not supported
by output plugin")));

If a given output plugin doesn't implement the callbacks but subscription
specifies sequences, the code will throw an error whether or not publication is
publishing sequences. Instead I think the behaviour should be same as the case
when publication doesn't include sequences even if the publisher node has
sequences. In either case publisher (the plugin or the publication) doesn't want
to publish sequence data. So subscriber's request can be ignored.

What might be good is to throw an error if the publication publishes the
sequences but there are no callbacks - both output plugin and the publication
are part of publisher node, thus it's easy for users to setup them consistently.
GetPublicationRelations can be tweaked a bit to return just tables or sequences.
That along with publication's all sequences flag should tell us whether
publication publishes any sequences or not.

That ends my first round of reviews.

--
Best Wishes,
Ashutosh Bapat