RE: Rework LogicalOutputPluginWriterUpdateProgress - Mailing list pgsql-hackers
From | wangw.fnst@fujitsu.com |
---|---|
Subject | RE: Rework LogicalOutputPluginWriterUpdateProgress |
Date | |
Msg-id | OS3PR01MB627526095A61C0B9B70FDE5D9EBA9@OS3PR01MB6275.jpnprd01.prod.outlook.com Whole thread Raw |
In response to | Re: Rework LogicalOutputPluginWriterUpdateProgress (Amit Kapila <amit.kapila16@gmail.com>) |
List | pgsql-hackers |
On Mon, Mar 10, 2023 14:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > 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. I think this approach looks fine to me. So, I wrote a separate patch (0006) for discussing and reviewing this approach. Regards, Wang wei
pgsql-hackers by date: