Subject: Re: logicalrep_message_type throws an error

From: Alvaro Herrera <[email-masked]>
Date: Mon, 17 Jul 2023 16:24:52 +0200

On 2023-Jul-17, Ashutosh Bapat wrote:

> Prologue of psprintf() says
> 
> * Errors are not returned to the caller, but are reported via elog(ERROR)
> * in the backend, or printf-to-stderr-and-exit() in frontend builds.
> * One should therefore think twice about using this in libpq.
> 
> If an error occurs in psprintf(), it will throw an error which will
> override the original error. I think we should avoid any stuff that
> throws further errors.

Ooh, yeah, this is an excellent point.  I agree it would be better to
avoid psprintf() here and anything that adds more failure modes.  Let's
just do the thing in the original patch you submitted, to ensure all
these strings are compile-time constants; that's likely the most robust.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Pensar que el espectro que vemos es ilusorio no lo despoja de espanto,
sólo le suma el nuevo terror de la locura" (Perelandra, C.S. Lewis)

---

From: Amit Kapila <[email-masked]>
Date: Tue, 18 Jul 2023 08:45:12 +0530

On Mon, Jul 17, 2023 at 7:54 PM Alvaro Herrera <[email-masked]> wrote:
> avoid psprintf() here and anything that adds more failure modes.
>

I have tried to check whether we have such usage in any other error
callbacks. Though I haven't scrutinized each and every error callback,
I found a few of them where an error can be raised. For example,

rm_redo_error_callback()->initStringInfo()
CopyFromErrorCallback()->limit_printout_length()
shared_buffer_write_error_callback()->relpathperm()->relpathbackend()->GetRelationPath()->psprintf()

>  Let's

So will we be okay with something like the below:

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

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 18 Jul 2023 13:57:19 +0900

> > these strings are compile-time constants; that's likely the most robust.

Or can we use snprintf() writing "??? (%d)" to a fixed length char[8 +
11] allocated on the stack instead?

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: Amit Kapila <[email-masked]>
Date: Wed, 19 Jul 2023 09:01:34 +0530

> 11] allocated on the stack instead?

In the above size calculation, shouldn't it be 7 + 11 where 7 is for
(3 (???) + 1 for space + 2 for () + 1 for terminating null char) and
11 is for %d? BTW, this avoids dynamic allocation of the err string in
logicalrep_message_type() but we can't return a locally allocated
string, so do you think we should change the prototype of the function
to get this as an argument and then use it both for valid and invalid
cases? I think if there is some simpler way to achieve this then fine,
otherwise, let's return a constant string like "???" from
logicalrep_message_type() for the invalid action.

-- 
With Regards,
Amit Kapila.

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 19 Jul 2023 10:08:38 +0530

On Wed, Jul 19, 2023 at 9:01 AM Amit Kapila <[email-masked]> wrote:
> cases?

There are other places in the code which do something similar by using
statically allocated buffers like static char xya[SIZE]. We could do
that here. The caller may decide whether to pstrdup this buffer
further or just use it one time e.g. as an elog or printf argument.

As I said before, we should not even print message type in the error
context because it's unknown. Repeating that twice is useless. That
will need some changes to apply_error_callback() though.
But I am fine with "???" as well.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Amit Kapila <[email-masked]>
Date: Thu, 20 Jul 2023 09:10:45 +0530

> further or just use it one time e.g. as an elog or printf argument.

Okay, changed it accordingly. Currently, we call it only from
errcontext, so it looks reasonable to me to use static here.

> But I am fine with "???" as well.

I think in the end it won't make a big difference. So, I would like to
go with Sawada-San's suggestion to keep the message type consistent in
actual error and error context unless that requires bigger changes.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 20 Jul 2023 09:11:40 +0530

On Thu, Jul 20, 2023 at 9:10 AM Amit Kapila <[email-masked]> wrote:
> Okay, changed it accordingly.
>

oops, forgot to attach the patch.

-- 
With Regards,
Amit Kapila.

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 20 Jul 2023 22:09:31 +0530

> oops, forgot to attach the patch.

WFM

@@ -3367,7 +3367,7 @@ apply_dispatch(StringInfo s)
         default:
             ereport(ERROR,
                     (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                     errmsg("invalid logical replication message type
\"%c\"", action)));
+                     errmsg("invalid logical replication message type
\"??? (%d)\"", action)));

I think we should report character here since that's what is visible
in the code and also the message types are communicated as characters
not integers. Makes debugging easier. Context may report integer as an
additional data point.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 21 Jul 2023 09:57:30 +0900

> not integers. Makes debugging easier. Context may report integer as an

I think it could confuse us if an invalid message is not a printable character.

Regards,