From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, Fabrice Chapuis <fabrice636861(at)gmail(dot)com>, Euler Taveira <euler(at)eulerto(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, Petr Jelinek <petr(dot)jelinek(at)enterprisedb(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Ajin Cherian <itsajin(at)gmail(dot)com> |
Subject: | Re: Rework LogicalOutputPluginWriterUpdateProgress |
Date: | 2023-03-10 05:47:08 |
Message-ID: | CAHut+Pvry4986Nr-uaTPPXoqNQV_nMV-z34ozjekxgF+0kEwGw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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()?
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
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2023-03-10 05:58:29 | Re: Sub-millisecond [autovacuum_]vacuum_cost_delay broken |
Previous Message | jacktby@gmail.com | 2023-03-10 05:33:25 | How to get the real type use oid in internal codes? |