Horton

Thread

  1. remove unnecessary explicit type conversion (to char) for appendStringInfoChar function calls

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2025-04-11T17:36:48Z

    Hi,
In the current master code, 3 places we are using appendStringInfoChar
call with explicit type conversion into char. This is not needed as
all other places, we are using direct character to append.

--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -302,7 +302,7 @@ InteractiveBackend(StringInfo inBuf)
         */

        /* Add '\0' to make it look the same as message case. */
-       appendStringInfoChar(inBuf, (char) '\0');
+       appendStringInfoChar(inBuf, '\0');

Here, I am attaching a small patch to fix these 3 type conversions on head.

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

  2. Re: remove unnecessary explicit type conversion (to char) for appendStringInfoChar function calls

    Andrew Dunstan <andrew@dunslane.net> — 2025-04-12T18:26:29Z

    On 2025-04-11 Fr 1:36 PM, Mahendra Singh Thalor wrote:
> Hi,
> In the current master code, 3 places we are using appendStringInfoChar
> call with explicit type conversion into char. This is not needed as
> all other places, we are using direct character to append.
>
> --- a/src/backend/tcop/postgres.c
> +++ b/src/backend/tcop/postgres.c
> @@ -302,7 +302,7 @@ InteractiveBackend(StringInfo inBuf)
>           */
>
>          /* Add '\0' to make it look the same as message case. */
> -       appendStringInfoChar(inBuf, (char) '\0');
> +       appendStringInfoChar(inBuf, '\0');
>
> Here, I am attaching a small patch to fix these 3 type conversions on head.
>


Seems odd that this one is necessary at all. Doesn't a StringInfo always 
have a trailing null byte?


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  3. Re: remove unnecessary explicit type conversion (to char) for appendStringInfoChar function calls

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2025-04-12T19:10:54Z

    On Sat, 12 Apr 2025 at 23:56, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-04-11 Fr 1:36 PM, Mahendra Singh Thalor wrote:
> > Hi,
> > In the current master code, 3 places we are using appendStringInfoChar
> > call with explicit type conversion into char. This is not needed as
> > all other places, we are using direct character to append.
> >
> > --- a/src/backend/tcop/postgres.c
> > +++ b/src/backend/tcop/postgres.c
> > @@ -302,7 +302,7 @@ InteractiveBackend(StringInfo inBuf)
> >           */
> >
> >          /* Add '\0' to make it look the same as message case. */
> > -       appendStringInfoChar(inBuf, (char) '\0');
> > +       appendStringInfoChar(inBuf, '\0');
> >
> > Here, I am attaching a small patch to fix these 3 type conversions on
head.
> >
>
>
> Seems odd that this one is necessary at all. Doesn't a StringInfo always
> have a trailing null byte?
>
>
> cheers
>
>
> andrew
>
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>

Yes, we already have a NULL byte in the message but as per current code, we
can't remove this call because msg->cursor is coded as per extra NULL byte.

diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
> index 1cc126772f7..979ba9e48cf 100644
> --- a/src/backend/libpq/pqformat.c
> +++ b/src/backend/libpq/pqformat.c
> @@ -589,11 +589,11 @@ pq_getmsgstring(StringInfo msg)
>          * message.
>          */
>         slen = strlen(str);
> -       if (msg->cursor + slen >= msg->len)
> +       if (msg->cursor + slen > msg->len)
>                 ereport(ERROR,
>                                 (errcode(ERRCODE_PROTOCOL_VIOLATION),
>                                  errmsg("invalid string in message")));
> -       msg->cursor += slen + 1;
> +       msg->cursor += slen;
>
>         return pg_client_to_server(str, slen);
>  }
> @@ -618,11 +618,11 @@ pq_getmsgrawstring(StringInfo msg)
>          * message.
>          */
>         slen = strlen(str);
> -       if (msg->cursor + slen >= msg->len)
> +       if (msg->cursor + slen > msg->len)
>                 ereport(ERROR,
>                                 (errcode(ERRCODE_PROTOCOL_VIOLATION),
>                                  errmsg("invalid string in message")));
> -       msg->cursor += slen + 1;
> +       msg->cursor += slen;
>
>         return str;
>  }
>

We need many more changes if we want to remove NULL byte call.

Above changes will fix errors of initdb but other tests are failing as code
expects this extra null byte in string.

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

  4. Re: remove unnecessary explicit type conversion (to char) for appendStringInfoChar function calls

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-04-13T12:12:05Z

    On 2025-Apr-12, Andrew Dunstan wrote:

> Seems odd that this one is necessary at all. Doesn't a StringInfo always
> have a trailing null byte?

AFAICT what this is doing that's significant, is increment StringInfo->len.
Before doing it, the NUL byte is not part of the message; afterwards it
is.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them."    (Freeman Dyson)

  5. Re: remove unnecessary explicit type conversion (to char) for appendStringInfoChar function calls

    Andrew Dunstan <andrew@dunslane.net> — 2025-04-13T14:14:15Z

    On 2025-04-13 Su 8:12 AM, Álvaro Herrera wrote:
> On 2025-Apr-12, Andrew Dunstan wrote:
>
>> Seems odd that this one is necessary at all. Doesn't a StringInfo always
>> have a trailing null byte?
> AFAICT what this is doing that's significant, is increment StringInfo->len.
> Before doing it, the NUL byte is not part of the message; afterwards it
> is.
>


That make sense, but then it would be nice to have the accompanying 
comment a bit clearer.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com