Re: Remove unnecessary static type qualifiers - Mailing list pgsql-hackers

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