On Fri, Mar 10, 2023 at 11:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Mar 10, 2023 at 3:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Mar 9, 2023 at 10:56 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > 2. rollback_prepared_cb_wrapper
> > >
> > > /*
> > > * If the plugin support two-phase commits then rollback prepared callback
> > > * is mandatory
> > > + *
> > > + * FIXME: This should have been caught much earlier.
> > > */
> > > if (ctx->callbacks.rollback_prepared_cb == NULL)
> > > ereport(ERROR,
> > >
> > > ~
> > >
> > > Why is this seemingly unrelated FIXME still in the patch?
> > >
> >
> > After reading this Fixme comment and the error message ("logical
> > replication at prepare time requires a %s callback
> > rollback_prepared_cb"), I think we can move this and a similar check
> > in function commit_prepared_cb_wrapper() to prepare_cb_wrapper()
> > function. This is because there is no use of letting prepare pass when
> > we can't do a rollback or commit prepared. What do you think?
> >
>
> My first impression was it sounds like a good idea to catch the
> missing callbacks early as you said.
>
> But if you decide to check for missing commit/rollback callbacks early
> in prepare_cb_wrapper(), then won't you also want to have equivalent
> checking done earlier for stream_prepare_cb_wrapper()?
>
Yeah, probably or we can leave the lazy checking as it is. In the
ideal case, we could check for the presence of all the callbacks in
StartupDecodingContext() but we delay it to find the missing methods
later. One possibility is that we check for any missing method in
StartupDecodingContext() if any one of prepare/streaming calls are
present but not sure if that is any better than the current
arrangement.
> And then it quickly becomes a slippery slope to question many other things:
> - Why allow startup_cb if shutdown_cb is missing?
>
I am not sure if there is a hard dependency between these two but
their callers do check for Null before invoking those.
> - Why allow change_cb if commit_cb or rollback_cb is missing?
We have a check for change_cb and commit_cb in LoadOutputPlugin. Do we
have rollback_cb() defined at all?
> - Why allow filter_prepare_cb if prepare_cb is missing?
>
I am not so sure about this but If prepare gets filtered, we don't
need to invoke prepare_cb.
> - etc.
>
> ~
>
> So I am wondering if the HEAD code lazy-check of the callback only at
> the point where it is needed was actually a deliberate design choice
> just to be simpler - e.g. we don't need to be so concerned about any
> other callback dependencies.
>
Yeah, changing that probably needs some more thought. I have mentioned
one of the possibilities above.
--
With Regards,
Amit Kapila.
