Remove unnecessary static type qualifiers

Started by Japin Li9 months ago8 messages

japinli@hotmail.com

9 months ago

Hi, all

When I read the libpq source code, I found unnecessary static type qualifiers
in PQsetClientEncoding().

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0258d9ace3c..300ddfffd55 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7717,7 +7717,7 @@ int
 PQsetClientEncoding(PGconn *conn, const char *encoding)
 {
 	char		qbuf[128];
-	static const char query[] = "set client_encoding to '%s'";
+	const char	query[] = "set client_encoding to '%s'";
 	PGresult   *res;
 	int			status;

--
Regrads,
Japin Li

Junwang Zhao

zhjwpku@gmail.com

9 months ago

In reply to: Japin Li (#1)

Re: Remove unnecessary static type qualifiers

On Tue, Apr 8, 2025 at 4:29 PM Japin Li <japinli@hotmail.com> wrote:

Hi, all

When I read the libpq source code, I found unnecessary static type qualifiers
in PQsetClientEncoding().

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0258d9ace3c..300ddfffd55 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7717,7 +7717,7 @@ int
PQsetClientEncoding(PGconn *conn, const char *encoding)
{
char            qbuf[128];
-       static const char query[] = "set client_encoding to '%s'";
+       const char      query[] = "set client_encoding to '%s'";
PGresult   *res;
int                     status;

I doubt that, if you remove the *static*, it will allocate more memory
on stack and need more instructions to copy the string to that area.

You can check the assembly to prove this.

Here is what I got on macos M3 chip.

with static:

0000000000015710 <_PQsetClientEncoding>:
15710: d10383ff sub sp, sp, #0xe0
15714: a90d7bfd stp x29, x30, [sp, #0xd0]
15718: 910343fd add x29, sp, #0xd0
1571c: f00002a8 adrp x8, 0x6c000 <_write+0x6c000>
15720: f941a908 ldr x8, [x8, #0x350]
15724: f9400108 ldr x8, [x8]
15728: f81f83a8 stur x8, [x29, #-0x8]
1572c: f9001fe0 str x0, [sp, #0x38]
15730: f9001be1 str x1, [sp, #0x30]
15734: f9401fe8 ldr x8, [sp, #0x38]
15738: f1000108 subs x8, x8, #0x0
1573c: 1a9f17e8 cset w8, eq
15740: 37000108 tbnz w8, #0x0, 0x15760 <_PQsetClientEncoding+0x50>
15744: 14000001 b 0x15748 <_PQsetClientEncoding+0x38>
15748: f9401fe8 ldr x8, [sp, #0x38]
1574c: b941f908 ldr w8, [x8, #0x1f8]

without:

00000000000156ec <_PQsetClientEncoding>:
156ec: d10443ff sub sp, sp, #0x110
156f0: a90f6ffc stp x28, x27, [sp, #0xf0]
156f4: a9107bfd stp x29, x30, [sp, #0x100]
156f8: 910403fd add x29, sp, #0x100
156fc: f00002a8 adrp x8, 0x6c000 <_write+0x6c000>
15700: f941a908 ldr x8, [x8, #0x350]
15704: f9400108 ldr x8, [x8]
15708: f81e83a8 stur x8, [x29, #-0x18]
1570c: f9001be0 str x0, [sp, #0x30]
15710: f90017e1 str x1, [sp, #0x28]
15714: f00001c9 adrp x9, 0x50000 <_write+0x50000>
15718: 913ded29 add x9, x9, #0xf7b
1571c: 3dc00120 ldr q0, [x9]
15720: 910103e8 add x8, sp, #0x40
15724: 3d8013e0 str q0, [sp, #0x40]
15728: 3cc0c120 ldur q0, [x9, #0xc]
1572c: 3c80c100 stur q0, [x8, #0xc]
15730: f9401be8 ldr x8, [sp, #0x30]
15734: f1000108 subs x8, x8, #0x0
15738: 1a9f17e8 cset w8, eq
1573c: 37000108 tbnz w8, #0x0, 0x1575c <_PQsetClientEncoding+0x70>
15740: 14000001 b 0x15744 <_PQsetClientEncoding+0x58>
15744: f9401be8 ldr x8, [sp, #0x30]
15748: b941f908 ldr w8, [x8, #0x1f8]

--
Regrads,
Japin Li

--
Regards
Junwang Zhao

Tom Lane

tgl@sss.pgh.pa.us

9 months ago

In reply to: Junwang Zhao (#2)

Re: Remove unnecessary static type qualifiers

Junwang Zhao <zhjwpku@gmail.com> writes:

On Tue, Apr 8, 2025 at 4:29 PM Japin Li <japinli@hotmail.com> wrote:
-       static const char query[] = "set client_encoding to '%s'";
+       const char      query[] = "set client_encoding to '%s'";

I doubt that, if you remove the *static*, it will allocate more memory
on stack and need more instructions to copy the string to that area.

Yeah, it's not an improvement as written. This might be OK:

+ const char *query = "set client_encoding to '%s'";

But I kinda doubt it's worth touching.

regards, tom lane

Peter Eisentraut

peter@eisentraut.org

9 months ago

In reply to: Junwang Zhao (#2)

Re: Remove unnecessary static type qualifiers

On 08.04.25 14:22, Junwang Zhao wrote:

When I read the libpq source code, I found unnecessary static type qualifiers
in PQsetClientEncoding().
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 0258d9ace3c..300ddfffd55 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7717,7 +7717,7 @@ int
PQsetClientEncoding(PGconn *conn, const char *encoding)
{
char            qbuf[128];
-       static const char query[] = "set client_encoding to '%s'";
+       const char      query[] = "set client_encoding to '%s'";
PGresult   *res;
int                     status;
I doubt that, if you remove the *static*, it will allocate more memory
on stack and need more instructions to copy the string to that area.

To avoid creating an array on the stack, you could maybe write it with a
pointer instead, like:

const char * const query = "...";

I haven't tested whether that results in different or better compiled
code. The original code is probably good enough.

David Rowley

dgrowleyml@gmail.com

9 months ago

In reply to: Peter Eisentraut (#4)

Re: Remove unnecessary static type qualifiers

On Wed, 9 Apr 2025 at 03:46, Peter Eisentraut <peter@eisentraut.org> wrote:

To avoid creating an array on the stack, you could maybe write it with a
pointer instead, like:

const char * const query = "...";

I haven't tested whether that results in different or better compiled
code. The original code is probably good enough.

I expect it's been done the way it has to make the overflow detection
code work. The problem with having a pointer to a string constant is
that sizeof() will return the size of the pointer and not the space
needed to store the string.

We can get rid of the variable and make the overflow work by checking
the return length of snprintf. I think that's all C99 spec now...

len = snprintf(qbuf, "set client_encoding to '%s'", encoding);
/* check query buffer overflow */
if (len >= sizeof(qbuf))
return -1;

David

Junwang Zhao

zhjwpku@gmail.com

9 months ago

In reply to: David Rowley (#5)

Re: Remove unnecessary static type qualifiers

On Wed, Apr 9, 2025 at 5:22 AM David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 9 Apr 2025 at 03:46, Peter Eisentraut <peter@eisentraut.org> wrote:

To avoid creating an array on the stack, you could maybe write it with a
pointer instead, like:

const char * const query = "...";

I haven't tested whether that results in different or better compiled
code. The original code is probably good enough.

I expect it's been done the way it has to make the overflow detection
code work. The problem with having a pointer to a string constant is
that sizeof() will return the size of the pointer and not the space
needed to store the string.

We can get rid of the variable and make the overflow work by checking
the return length of snprintf. I think that's all C99 spec now...

len = snprintf(qbuf, "set client_encoding to '%s'", encoding);
/* check query buffer overflow */
if (len >= sizeof(qbuf))
return -1;

Agree, this is a better coding style. Please see if the following code makes
sense to you.

diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 0258d9ace3c..247d079faa6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7717,9 +7717,9 @@ int
 PQsetClientEncoding(PGconn *conn, const char *encoding)
 {
        char            qbuf[128];
-       static const char query[] = "set client_encoding to '%s'";
        PGresult   *res;
        int                     status;
+       int                     len;

if (!conn || conn->status != CONNECTION_OK)
return -1;
@@ -7731,12 +7731,11 @@ PQsetClientEncoding(PGconn *conn, const char *encoding)
if (strcmp(encoding, "auto") == 0)
encoding =
pg_encoding_to_char(pg_get_encoding_from_locale(NULL, true));

-       /* check query buffer overflow */
-       if (sizeof(qbuf) < (sizeof(query) + strlen(encoding)))
+       len = snprintf(qbuf, sizeof(qbuf), "set client_encoding to
'%s'", encoding);
+       if (len >= sizeof(qbuf))
                return -1;

/* ok, now send a query */
- sprintf(qbuf, query, encoding);
res = PQexec(conn, qbuf);

if (res == NULL)

David

--
Regards
Junwang Zhao

Japin Li

japinli@hotmail.com

9 months ago

In reply to: Junwang Zhao (#6)

Re: Remove unnecessary static type qualifiers

On Wed, 09 Apr 2025 at 10:34, Junwang Zhao <zhjwpku@gmail.com> wrote:

On Wed, Apr 9, 2025 at 5:22 AM David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 9 Apr 2025 at 03:46, Peter Eisentraut <peter@eisentraut.org> wrote:

To avoid creating an array on the stack, you could maybe write it with a
pointer instead, like:

const char * const query = "...";

I haven't tested whether that results in different or better compiled
code. The original code is probably good enough.

I expect it's been done the way it has to make the overflow detection
code work. The problem with having a pointer to a string constant is
that sizeof() will return the size of the pointer and not the space
needed to store the string.

We can get rid of the variable and make the overflow work by checking
the return length of snprintf. I think that's all C99 spec now...

len = snprintf(qbuf, "set client_encoding to '%s'", encoding);
/* check query buffer overflow */
if (len >= sizeof(qbuf))
return -1;

Agree, this is a better coding style. Please see if the following code makes
sense to you.
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 0258d9ace3c..247d079faa6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7717,9 +7717,9 @@ int
PQsetClientEncoding(PGconn *conn, const char *encoding)
{
char            qbuf[128];
-       static const char query[] = "set client_encoding to '%s'";
PGresult   *res;
int                     status;
+       int                     len;
if (!conn || conn->status != CONNECTION_OK)
return -1;
@@ -7731,12 +7731,11 @@ PQsetClientEncoding(PGconn *conn, const char *encoding)
if (strcmp(encoding, "auto") == 0)
encoding =
pg_encoding_to_char(pg_get_encoding_from_locale(NULL, true));
-       /* check query buffer overflow */
-       if (sizeof(qbuf) < (sizeof(query) + strlen(encoding)))
+       len = snprintf(qbuf, sizeof(qbuf), "set client_encoding to
'%s'", encoding);
+       if (len >= sizeof(qbuf))
return -1;
/* ok, now send a query */
- sprintf(qbuf, query, encoding);
res = PQexec(conn, qbuf);

if (res == NULL)

Thank you for all the explanations! The above code looks good to me, except
for a minor issue; since snprintf may return a negative value, should we
check for this?

--
Regrads,
Japin Li

Junwang Zhao

zhjwpku@gmail.com

9 months ago

In reply to: Japin Li (#7)

Re: Remove unnecessary static type qualifiers

On Wed, Apr 9, 2025 at 12:25 PM Japin Li <japinli@hotmail.com> wrote:

On Wed, 09 Apr 2025 at 10:34, Junwang Zhao <zhjwpku@gmail.com> wrote:
On Wed, Apr 9, 2025 at 5:22 AM David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 9 Apr 2025 at 03:46, Peter Eisentraut <peter@eisentraut.org> wrote:

To avoid creating an array on the stack, you could maybe write it with a
pointer instead, like:

const char * const query = "...";

I haven't tested whether that results in different or better compiled
code. The original code is probably good enough.

I expect it's been done the way it has to make the overflow detection
code work. The problem with having a pointer to a string constant is
that sizeof() will return the size of the pointer and not the space
needed to store the string.

We can get rid of the variable and make the overflow work by checking
the return length of snprintf. I think that's all C99 spec now...

len = snprintf(qbuf, "set client_encoding to '%s'", encoding);
/* check query buffer overflow */
if (len >= sizeof(qbuf))
return -1;

Agree, this is a better coding style. Please see if the following code makes
sense to you.
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 0258d9ace3c..247d079faa6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7717,9 +7717,9 @@ int
PQsetClientEncoding(PGconn *conn, const char *encoding)
{
char            qbuf[128];
-       static const char query[] = "set client_encoding to '%s'";
PGresult   *res;
int                     status;
+       int                     len;
if (!conn || conn->status != CONNECTION_OK)
return -1;
@@ -7731,12 +7731,11 @@ PQsetClientEncoding(PGconn *conn, const char *encoding)
if (strcmp(encoding, "auto") == 0)
encoding =
pg_encoding_to_char(pg_get_encoding_from_locale(NULL, true));
-       /* check query buffer overflow */
-       if (sizeof(qbuf) < (sizeof(query) + strlen(encoding)))
+       len = snprintf(qbuf, sizeof(qbuf), "set client_encoding to
'%s'", encoding);
+       if (len >= sizeof(qbuf))
return -1;
/* ok, now send a query */
- sprintf(qbuf, query, encoding);
res = PQexec(conn, qbuf);

if (res == NULL)
Thank you for all the explanations! The above code looks good to me, except
for a minor issue; since snprintf may return a negative value, should we
check for this?

Maybe, I checked the *dopr* implementation a little bit, it seems
wouldn't return
-1 for fmt %s, %c, %d, %u etc. there are some places don't check the -1, like
[1]: https://github.com/postgres/postgres/blob/master/src/backend/postmaster/launch_backend.c#L469
it worth to
change other places too.

[1]: https://github.com/postgres/postgres/blob/master/src/backend/postmaster/launch_backend.c#L469
[2]: https://github.com/postgres/postgres/blob/master/src/bin/pg_upgrade/pg_upgrade.c#L262
[3]: https://github.com/postgres/postgres/blob/master/src/bin/pg_rewind/pg_rewind.c#L983

--
Regrads,
Japin Li

--
Regards
Junwang Zhao