Re: Make stream_prepare an optional callback

[Amit Kapila]

On Tue, Mar 9, 2021 at 3:41 PM Markus Wanner
 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.

Re: Make stream_prepare an optional callback

[Markus Wanner]

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

Re: Make stream_prepare an optional callback

[Amit Kapila]

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

Re: Make stream_prepare an optional callback

[Markus Wanner]

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

Re: Make stream_prepare an optional callback

[Amit Kapila]

On Tue, Mar 9, 2021 at 1:55 PM Markus Wanner
 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.

Re: Make stream_prepare an optional callback

[Markus Wanner]

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

Re: Make stream_prepare an optional callback

[Amit Kapila]

On Mon, Mar 8, 2021 at 2:43 PM Ajin Cherian  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_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.
>

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.

Make stream_prepare an optional callback

[Ajin Cherian]

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_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.


0001-Make-stream_prepare_cb-an-optional-callback.patch
Description: Binary data