On 2023-Jul-05, Alvaro Herrera wrote:
> On 2023-Jul-05, Euler Taveira wrote:
>
> > Isn't this numerical value already exposed in the error message (X = 88)?
> > In this example, it is:
> >
> > 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".
>
> The CONTEXT message could theoretically appear in other error throws,
> not just in "invalid logical replication message". So the duplicity is
> not really a problem.
Ah, but you're thinking that if the message type is invalid, then it
will have been rejected in the "invalid logical replication message"
stage, so no invalid message type will be reported. I guess there's a
point to that argument as well.
However, I don't see that having the numerical ASCII value there causes
any harm, even if the char value is already exposed in the other
message. (I'm sure you'll agree that this is quite a minor issue.)
I doubt that each of these two prints of the enum value showing
different formats is confusing. Yes, the enum is defined in terms of
char literals, but if an actually invalid message shows up with an
uint32 value outside the printable ASCII range, the printout might
be ugly or useless.
> > Another option, is to remove "X" from apply_dispatch() and add "???
> > (88)" to logicalrep_message_type().
Now *this* would be an actively bad idea IMO.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/