Re: Make stream_prepare an optional callback

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