Re: logicalrep_message_type throws an error

On Mon, Jul 3, 2023 at 6:32 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Mon, Jul 3, 2023, at 7:30 AM, Ashutosh Bapat wrote:
>
> logicalrep_message_type() is used to convert logical message type code
> into name while prepared error context or details. Thus when this
> function is called probably an ERROR is already raised. If
> logicalrep_message_type() gets an unknown message type, it will throw
> an error, which will suppress the error for which we are building
> context or details. That's not useful. I think instead
> logicalrep_message_type() should return "unknown" when it encounters
> an unknown message type and let the original error message be thrown
> as is.
>
>
> Hmm. Good catch. The current behavior is:
>
> ERROR: invalid logical replication message type "X"
> LOG: background worker "logical replication worker" (PID 71800) exited with exit code 1
>
> ... that hides the details. After providing a default message type:
>
> ERROR: invalid logical replication message type "X"
> CONTEXT: processing remote data for replication origin "pg_16638" during message type "???" in transaction 796, finished at 0/16266F8
>

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()? Isn't it
better to return the 'action' from the function
logicalrep_message_type() for unknown type? That way the information
could be a bit better and we can easily catch the code bug as well.

--
With Regards,
Amit Kapila.