Re: Patch avoid call strlen repeatedly in loop.

On 11/8/19 9:41 AM, Ranier VF wrote:
> --- \dll\postgresql-12.0\a\backend\libpq\auth.c    Mon Sep 30 17:06:55 2019
> +++ auth.c    Fri Nov 08 14:27:17 2019
> @@ -1815,6 +1815,7 @@
>       char        ident_user[IDENT_USERNAME_MAX + 1];
>       pgsocket    sock_fd = PGINVALID_SOCKET; /* for talking to Ident server */
>       int            rc;                /* Return code from a locally called function */
> +    int            ident_query_len;
>       bool        ident_return;
>       char        remote_addr_s[NI_MAXHOST];
>       char        remote_port[NI_MAXSERV];
> @@ -1913,7 +1914,7 @@
>       }
>   
>       /* The query we send to the Ident server */
> -    snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
> +    ident_query_len = snprintf(ident_query, sizeof(ident_query), "%s,%s\r\n",
>                remote_port, local_port);
>   
>       /* loop in case send is interrupted */
> @@ -1921,7 +1922,7 @@
>       {
>           CHECK_FOR_INTERRUPTS();
>   
> -        rc = send(sock_fd, ident_query, strlen(ident_query), 0);
> +        rc = send(sock_fd, ident_query, ident_query_len, 0);

Hello Ranier,

In general, writing a string with snprintf and then calling strlen on 
that same string is not guaranteed to give the same lengths.  You can 
easily construct a case where they differ:

     char foo[3] = {0};
     int foolen;
     foolen = snprintf(foo, sizeof(foo), "%s", "xxxxxxxx");
     printf("strlen(foo) = %u, foolen = %u, foo = '%s'\n", strlen(foo), 
foolen, foo);

Using standard snprintf (and not pg_snprintf), I get:

     strlen(foo) = 2, foolen = 8, foo = 'xx'

Perhaps an analysis of the surrounding code would prove that in all 
cases this particular snprintf will return the same result that 
strlen(ident_query) would return, but I don't care to do the analysis. 
I think the way it is coded is easier to read, and probably more robust 
against future changes, even if your proposed change happens to be safe 
today.

As for calling strlen(ident_query) just once, caching that result, and 
then looping, I don't immediately see a problem, but I also don't expect 
that loop to run more than one iteration except under unusual instances. 
  Do you find that send() gets interrupted a lot?  Is 
strlen(ident_query) taking long enough to be significant compared to how 
long send() takes?

A bit more information about the performance problem you are 
encountering might make it easier to understand the motivation for this 
patch.

-- 
Mark Dilger