Thread: Remove unnecessary static type qualifiers
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
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
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
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.
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
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
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
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], [2], [3], but for consistency I think we should, I'm not sure if
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