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()?
And then it quickly becomes a slippery slope to question many other things:
- Why allow startup_cb if shutdown_cb is missing?
- Why allow change_cb if commit_cb or rollback_cb is missing?
- Why allow filter_prepare_cb if prepare_cb is missing?
- 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.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
