Subject: Re: logicalrep_message_type throws an error

From: Alvaro Herrera <[email-masked]>
Date: Wed, 5 Jul 2023 12:56:39 +0200

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()? 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.

Are you suggesting that logicalrep_message_type should include the
numerical value of 'action' in the ??? message? Something like this:

 ERROR:  invalid logical replication message type "X"
 CONTEXT:  processing remote data for replication origin "pg_16638" during message type "??? (123)" in transaction 796, finished at 0/16266F8

I don't see why not -- seems easy enough, and might help somebody.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes...  Fixed.
                               http://postgr.es/m/[email-masked]

---

From: Amit Kapila <[email-masked]>
Date: Wed, 5 Jul 2023 18:17:54 +0530

>  CONTEXT:  processing remote data for replication origin "pg_16638" during message type "??? (123)" in transaction 796, finished at 0/16266F8

Yes, something like that would be better.

-- 
With Regards,
Amit Kapila.

---

From: "Euler Taveira" <[email-masked]>
Date: Wed, 05 Jul 2023 10:37:05 -0300

> CONTEXT:  processing remote data for replication origin "pg_16638" during message type "??? (123)" in transaction 796, finished at 0/16266F8

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". Another
option, is to remove "X" from apply_dispatch() and add "??? (88)" to 
logicalrep_message_type().

Opinions?

--
Euler Taveira
EDB   https://www.enterprisedb.com/

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 5 Jul 2023 19:54:36 +0530

On Wed, Jul 5, 2023 at 7:07 PM Euler Taveira <[email-masked]> wrote:
> > 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.

It might happen that the upstream may send an unknown message type
that both apply_dispatch() and logicalrep_message_type() can not
handle.

> 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.
If we need a context and have all the other necessary fields, we can
improve apply_error_callback() to provide context when
apply_error_callback_arg.command = 0 .

--
Best Wishes,
Ashutosh Bapat

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 5 Jul 2023 18:11:55 +0200

> logicalrep_message_type().

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.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Now I have my system running, not a byte was off the shelf;
It rarely breaks and when it does I fix the code myself.
It's stable, clean and elegant, and lightning fast as well,
And it doesn't cost a nickel, so Bill Gates can go to hell."

---

From: Amit Kapila <[email-masked]>
Date: Thu, 6 Jul 2023 15:00:27 +0530

> suggesting is not possible.

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

> apply_error_callback() will just return without providing a context.

Hmm, this looks a bit hacky to me.

-- 
With Regards,
Amit Kapila.