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

Started by Mahendra Singh Thalor9 months ago5 messages

mahi6run@gmail.com

9 months ago

1 attachment(s)

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

Attachments:

v01_remove-unnecessary-type-conversion-into-char-for-appendStringInfoChar.patchapplication/octet-stream; name=v01_remove-unnecessary-type-conversion-into-char-for-appendStringInfoChar.patchDownload

From 4f7f3fa8133086f0efcc6fc458626ec97c831870 Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <mahi6run@gmail.com>
Date: Fri, 11 Apr 2025 22:50:47 +0530
Subject: [PATCH] remove unnecessary type conversion into char for
 appendStringInfoChar function calls.

On head, we are using 3 places appendStringInfoChar call with explicit type
conversion into char.
This is not needed as many places we are using direct character to append.

Ex: appendStringInfoChar(inBuf, (char) '\0');
   appendStringInfoChar(inBuf, '\0');
---
 src/backend/tcop/postgres.c  | 2 +-
 src/bin/pg_dump/pg_restore.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index dc4c600922d..e99fb1c1ff6 100644
--- 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');
 
 	/*
 	 * if the query echo flag was given, print the query..
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 24a44b81a95..556361dbcc6 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -898,12 +898,12 @@ read_one_statement(StringInfo inBuf, FILE *pfile)
 
 		if (c == ';')
 		{
-			appendStringInfoChar(inBuf, (char) ';');
+			appendStringInfoChar(inBuf, ';');
 			break;
 		}
 
 		if (c == '\n')
-			appendStringInfoChar(inBuf, (char) '\n');
+			appendStringInfoChar(inBuf, '\n');
 	}
 
 	destroyStringInfo(&q);
-- 
2.39.3

Andrew Dunstan

andrew@dunslane.net

9 months ago

In reply to: Mahendra Singh Thalor (#1)

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

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

Mahendra Singh Thalor

mahi6run@gmail.com

9 months ago

In reply to: Andrew Dunstan (#2)

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

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

Álvaro Herrera

alvherre@kurilemu.de

9 months ago

In reply to: Andrew Dunstan (#2)

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

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)

Andrew Dunstan

andrew@dunslane.net

9 months ago

In reply to: Álvaro Herrera (#4)

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

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