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
