RE: Rework LogicalOutputPluginWriterUpdateProgress

On Mon, Mar 10, 2023 14:35 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Mar 10, 2023 at 11:17 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Fri, Mar 10, 2023 at 3:32 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Mar 9, 2023 at 10:56 AM Peter Smith <smithpb2250(at)gmail(dot)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.

I think this approach looks fine to me. So, I wrote a separate patch (0006) for
discussing and reviewing this approach.

Regards,
Wang wei