Re: Make stream_prepare an optional callback

On Tue, Mar 9, 2021 at 2:23 PM Markus Wanner <markus(at)bluegap(dot)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.