Re: logicalrep_message_type throws an error

On Wed, Jul 5, 2023 at 7:54 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Wed, Jul 5, 2023 at 7:07 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> >
> > On Wed, Jul 5, 2023, at 7:56 AM, Alvaro Herrera wrote:
> >
> > On 2023-Jul-05, Amit Kapila wrote:
> >
> > > I think after returning "???" from logicalrep_message_type(), the
> > > above is possible when we get the error: "invalid logical replication
> > > message type "X"" from apply_dispatch(), right? If so, then what about
> > > the case when we forgot to handle some message in
> > > logicalrep_message_type() but handled it in apply_dispatch()?
>
> apply_dispatch() has a default case in switch() so it can
> theoretically forget to handle some message type. I think we should
> avoid default case in that function to catch missing message type in
> that function. But if logicalrep_message_type() doesn't use default
> case, it won't forget to handle a known message type. So what you are
> suggesting is not possible.
>

Right, but still I feel it would be better to return actual action.

> It might happen that the upstream may send an unknown message type
> that both apply_dispatch() and logicalrep_message_type() can not
> handle.
>
> > ERROR: invalid logical replication message type "X"
> > CONTEXT: processing remote data for replication origin "pg_16638" during message type "??? (88)" in transaction 796, finished at 0/1626698
> >
> > IMO it could be confusing if we provide two representations of the same data (X
> > and 88). Since we already provide "X" I don't think we need "88". Another
> > option, is to remove "X" from apply_dispatch() and add "??? (88)" to
> > logicalrep_message_type().
>
> I think we don't need message type to be mentioned in the context for
> an error about invalid message type. I think what needs to be done is
> to set
> apply_error_callback_arg.command = 0 before calling ereport in the
> default case of apply_dispatch().
> apply_error_callback() will just return without providing a context.
>

Hmm, this looks a bit hacky to me.

--
With Regards,
Amit Kapila.