Make stream_prepare an optional callback
Hi Hackers,
As part of commit 0aa8a0
<https://github.com/postgres/postgres/commit/0aa8a01d04c8fe200b7a106878eebc3d0af9105c>
,
new plugin methods (callbacks) were defined for enabling two_phase commits.
5 callbacks were required:
* begin_prepare
* prepare
* commit_prepared
* rollback_prepared
* stream_prepare
and 1 callback was optional:
* filter_prepare
I don't think stream_prepare should be made a required callback for
enabling two_phase commits. stream_prepare callback is required when a
logical replication slot is configured both for streaming in-progress
transactions and two_phase commits. Plugins can and should be allowed to
disallow this combination of allowing both streaming and two_phase at the
same time. In which case, stream_prepare should be an optional callback.
Attaching a patch that makes this change. Let me know if you have any
comments.
regards,
Ajin Cherian
Fujitsu Australia.
Attachments:
0001-Make-stream_prepare_cb-an-optional-callback.patchapplication/octet-stream; name=0001-Make-stream_prepare_cb-an-optional-callback.patchDownload
From 6e897fab2ce393ff5bdb32d3f380e331316a4959 Mon Sep 17 00:00:00 2001
From: Ajin Cherian <ajinc@fast.au.fujitsu.com>
Date: Mon, 8 Mar 2021 03:09:46 -0500
Subject: [PATCH] Make stream_prepare_cb an optional callback.
Previously stream_prepare_cb was a required callback. Change documentation and code
to make it an optional callback. This allows plugins to not allow the enabling of
streaming and two_phase at the same time in logical replication.
Author: Ajin Cherian
---
doc/src/sgml/logicaldecoding.sgml | 16 ++++++++--------
src/backend/replication/logical/logical.c | 9 +++------
2 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/doc/src/sgml/logicaldecoding.sgml b/doc/src/sgml/logicaldecoding.sgml
index 80eb96d..a63fa2d 100644
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -468,9 +468,9 @@ typedef void (*LogicalOutputPluginInit) (struct OutputPluginCallbacks *cb);
An output plugin may also define functions to support streaming of large,
in-progress transactions. The <function>stream_start_cb</function>,
<function>stream_stop_cb</function>, <function>stream_abort_cb</function>,
- <function>stream_commit_cb</function>, <function>stream_change_cb</function>,
- and <function>stream_prepare_cb</function>
- are required, while <function>stream_message_cb</function> and
+ <function>stream_commit_cb</function>, and <function>stream_change_cb</function>,
+ are required, while <function>stream_prepare_cb</function>,
+ <function>stream_message_cb</function> and
<function>stream_truncate_cb</function> are optional.
</para>
@@ -478,9 +478,9 @@ typedef void (*LogicalOutputPluginInit) (struct OutputPluginCallbacks *cb);
An output plugin may also define functions to support two-phase commits,
which allows actions to be decoded on the <command>PREPARE TRANSACTION</command>.
The <function>begin_prepare_cb</function>, <function>prepare_cb</function>,
- <function>stream_prepare_cb</function>,
<function>commit_prepared_cb</function> and <function>rollback_prepared_cb</function>
- callbacks are required, while <function>filter_prepare_cb</function> is optional.
+ callbacks are required, while <function>stream_prepare_cb</function> and
+ <function>filter_prepare_cb</function> are optional.
</para>
</sect2>
@@ -1195,9 +1195,9 @@ stream_commit_cb(...); <-- commit of the streamed transaction
provide additional callbacks. There are multiple two-phase commit callbacks
that are required, (<function>begin_prepare_cb</function>,
<function>prepare_cb</function>, <function>commit_prepared_cb</function>,
- <function>rollback_prepared_cb</function> and
- <function>stream_prepare_cb</function>) and an optional callback
- (<function>filter_prepare_cb</function>).
+ <function>rollback_prepared_cb</function>) and
+ optional callbacks, (<function>stream_prepare_cb</function> and
+ <function>filter_prepare_cb</function>).
</para>
<para>
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 37b75de..5324851 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -1326,6 +1326,9 @@ stream_prepare_cb_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn,
Assert(ctx->streaming);
Assert(ctx->twophase);
+ if (ctx->callbacks.stream_prepare_cb == NULL)
+ return;
+
/* Push callback + info on the error context stack */
state.ctx = ctx;
state.callback_name = "stream_prepare";
@@ -1340,12 +1343,6 @@ stream_prepare_cb_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn,
ctx->write_xid = txn->xid;
ctx->write_location = txn->end_lsn;
- /* in streaming mode with two-phase commits, stream_prepare_cb is required */
- if (ctx->callbacks.stream_prepare_cb == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical streaming at prepare time requires a stream_prepare_cb callback")));
-
ctx->callbacks.stream_prepare_cb(ctx, txn, prepare_lsn);
/* Pop the error context stack */
--
1.8.3.1
On Mon, Mar 8, 2021 at 2:43 PM Ajin Cherian <itsajin@gmail.com> wrote:
Hi Hackers,
As part of commit 0aa8a0 , new plugin methods (callbacks) were defined for enabling two_phase commits.
5 callbacks were required:
* begin_prepare
* prepare
* commit_prepared
* rollback_prepared
* stream_prepareand 1 callback was optional:
* filter_prepareI don't think stream_prepare should be made a required callback for enabling two_phase commits. stream_prepare callback is required when a logical replication slot is configured both for streaming in-progress transactions and two_phase commits. Plugins can and should be allowed to disallow this combination of allowing both streaming and two_phase at the same time. In which case, stream_prepare should be an optional callback.
Sounds reasonable to me. I also don't see a reason why we need to make
this a necessary callback. Some plugin authors might just want 2PC
without streaming support.
Markus, others working on logical decoding plugins, do you have any
opinion on this?
--
With Regards,
Amit Kapila.
On 09.03.21 07:40, Amit Kapila wrote:
Sounds reasonable to me. I also don't see a reason why we need to make
this a necessary callback. Some plugin authors might just want 2PC
without streaming support.
Sounds okay to me. Probably means we'll have to check for this callback
and always skip the prepare for streamed transactions, w/o even
triggering filter_prepare, right? (Because the extension requesting not
to filter it, but not providing the corresponding callback does not make
sense.)
If you're going to together a patch Ajin, I'm happy to review.
Best Regards
Markus
On Tue, Mar 9, 2021 at 1:55 PM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
On 09.03.21 07:40, Amit Kapila wrote:
Sounds reasonable to me. I also don't see a reason why we need to make
this a necessary callback. Some plugin authors might just want 2PC
without streaming support.Sounds okay to me. Probably means we'll have to check for this callback
and always skip the prepare for streamed transactions,
I think so. The behavior has to be similar to other optional callbacks
like message_cb, truncate_cb, stream_truncate_cb. Basically, we don't
need to error out if those callbacks are not provided.
w/o even
triggering filter_prepare, right?
I think the filter check is before we try to send the actual message.
(Because the extension requesting not
to filter it, but not providing the corresponding callback does not make
sense.)
The extension can request two_phase without streaming.
If you're going to together a patch Ajin, I'm happy to review.
It is attached with the initial email.
--
With Regards,
Amit Kapila.
On 09.03.21 09:39, Amit Kapila wrote:
It is attached with the initial email.
Oh, sorry, I looked up the initial email, but still didn't see the patch.
I think so. The behavior has to be similar to other optional callbacks
like message_cb, truncate_cb, stream_truncate_cb. Basically, we don't
need to error out if those callbacks are not provided.
Right, but the patch proposes to error out. I wonder whether that could
be avoided.
The extension can request two_phase without streaming.
Sure. I'm worried about the case both are requested, but filter_prepare
returns false, i.e. asking for a streamed prepare without providing the
corresponding callback.
I wonder whether Postgres could deny the stream_prepare right away and
not even invoke filter_prepare. And instead just skip it because the
output plugin did not provide an appropriate callback.
An error is not as nice, but I'm okay with that as well.
Best Regards
Markus
On Tue, Mar 9, 2021 at 2:23 PM Markus Wanner <markus@bluegap.ch> wrote:
On 09.03.21 09:39, Amit Kapila wrote:
It is attached with the initial email.
Oh, sorry, I looked up the initial email, but still didn't see the patch.
I think so. The behavior has to be similar to other optional callbacks
like message_cb, truncate_cb, stream_truncate_cb. Basically, we don't
need to error out if those callbacks are not provided.Right, but the patch proposes to error out. I wonder whether that could
be avoided.
AFAICS, the error is removed by the patch as per below change:
+ if (ctx->callbacks.stream_prepare_cb == NULL)
+ return;
+
/* Push callback + info on the error context stack */
state.ctx = ctx;
state.callback_name = "stream_prepare";
@@ -1340,12 +1343,6 @@ stream_prepare_cb_wrapper(ReorderBuffer *cache,
ReorderBufferTXN *txn,
ctx->write_xid = txn->xid;
ctx->write_location = txn->end_lsn;- /* in streaming mode with two-phase commits, stream_prepare_cb is required */
- if (ctx->callbacks.stream_prepare_cb == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical streaming at prepare time requires a
stream_prepare_cb callback")));
-
The extension can request two_phase without streaming.
Sure. I'm worried about the case both are requested, but filter_prepare
returns false, i.e. asking for a streamed prepare without providing the
corresponding callback.
oh, right, in that case, it will skip the stream_prepare even though
that is required. I guess in FilterPrepare, we should check if
rbtxn_is_streamed and stream_prepare_cb is not provided, then we
return true.
--
With Regards,
Amit Kapila.
On 09.03.21 10:37, Amit Kapila wrote:
AFAICS, the error is removed by the patch as per below change:
Ah, well, that does not seem right, then. We cannot just silently
ignore the callback but not skip the prepare, IMO. That would lead to
the output plugin missing the PREPARE, but still seeing a COMMIT
PREPARED for the transaction, potentially missing changes that went out
with the prepare, no?
oh, right, in that case, it will skip the stream_prepare even though
that is required. I guess in FilterPrepare, we should check if
rbtxn_is_streamed and stream_prepare_cb is not provided, then we
return true.
Except that FilterPrepare doesn't (yet) have access to a
ReorderBufferTXN struct (see the other thread I just started).
Maybe we need to do a ReorderBufferTXNByXid lookup already prior to (or
as part of) FilterPrepare, then also skip (rather than silently ignore)
the prepare if no stream_prepare_cb callback is given (without even
calling filter_prepare_cb, because the output plugin has already stated
it cannot handle that by not providing the corresponding callback).
However, I also wonder what's the use case for an output plugin enabling
streaming and two-phase commit, but not providing a stream_prepare_cb.
Maybe the original ERROR is the simpler approach? I.e. making the
stream_prepare_cb mandatory, if and only if both are enabled (and
filter_prepare doesn't skip). (As in the original comment that says:
"in streaming mode with two-phase commits, stream_prepare_cb is required").
I guess I don't quite understand the initial motivation for the patch.
It states: "This allows plugins to not allow the enabling of streaming
and two_phase at the same time in logical replication." That's beyond
me ... "allows [..] to not allow"? Why not, an output plugin can still
reasonably request both. And that's a good thing, IMO. What problem
does the patch try to solve?
Regards
Markus
On Tue, Mar 9, 2021 at 3:41 PM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
On 09.03.21 10:37, Amit Kapila wrote:
I guess I don't quite understand the initial motivation for the patch.
It states: "This allows plugins to not allow the enabling of streaming
and two_phase at the same time in logical replication." That's beyond
me ... "allows [..] to not allow"? Why not, an output plugin can still
reasonably request both. And that's a good thing, IMO. What problem
does the patch try to solve?
AFAIU, Ajin doesn't want to mandate streaming with two_pc option. But,
maybe you are right that it doesn't make sense for the user to provide
both options but doesn't provide stream_prepare callback, and giving
an error in such a case should be fine. I think if we have to follow
Ajin's idea then we need to skip 2PC in such a case (both prepare and
commit prepare) and make this a regular transaction.
--
With Regards,
Amit Kapila.