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

On Sat, 12 Apr 2025 at 23:56, Andrew Dunstan <andrew(at)dunslane(dot)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