Patch avoid call strlen repeatedly in loop.

[Ranier VF]

Hi,
Please can anybody review and commit this patch.

Thanks.

Ranier Vilela

--- \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 @@
charident_user[IDENT_USERNAME_MAX + 1];
pgsocketsock_fd = PGINVALID_SOCKET; /* for talking to Ident 
server */
int rc; /* Return code 
from a locally called function */
+   int ident_query_len;
boolident_return;
charremote_addr_s[NI_MAXHOST];
charremote_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);
} while (rc < 0 && errno == EINTR);
 
if (rc < 0)
@@ -3053,6 +3054,8 @@
char   *receive_buffer = (char *) _recv_pack;
int32   service = pg_hton32(RADIUS_AUTHENTICATE_ONLY);
uint8  *cryptvector;
+   int secretlen;
+   int passwdlen;
int encryptedpasswordlen;
uint8   encryptedpassword[RADIUS_MAX_PASSWORD_LENGTH];
uint8  *md5trailer;
@@ -3125,10 +3128,12 @@
memcpy(cryptvector, secret, strlen(secret));
 
/* for the first iteration, we use the Request Authenticator vector */
+secretlen = strlen(secret);
+passwdlen = strlen(passwd);
md5trailer = packet->vector;
for (i = 0; i < encryptedpasswordlen; i += RADIUS_VECTOR_LENGTH)
{
-   memcpy(cryptvector + strlen(secret), md5trailer, 
RADIUS_VECTOR_LENGTH);
+   memcpy(cryptvector + secretlen, md5trailer, 
RADIUS_VECTOR_LENGTH);
 
/*
 * .. and for subsequent iterations the result of the previous 
XOR
@@ -3136,7 +3141,7 @@
 */
md5trailer = encryptedpassword + i;
 
-   if (!pg_md5_binary(cryptvector, strlen(secret) + 
RADIUS_VECTOR_LENGTH, encryptedpassword + i))
+   if (!pg_md5_binary(cryptvector, secretlen + 
RADIUS_VECTOR_LENGTH, encryptedpassword + i))
{
ereport(LOG,
(errmsg("could not perform MD5 
encryption of password")));
@@ -3147,7 +3152,7 @@
 
for (j = i; j < i + RADIUS_VECTOR_LENGTH; j++)
{
-   if (j < strlen(passwd))
+   if (j < passwdlen)
encryptedpassword[j] = passwd[j] ^ 
encryptedpassword[j];
else
encryptedpassword[j] = '\0' ^ 
encryptedpassword[j];
@@ -3329,7 +3334,7 @@
 * Verify the response authenticator, which is calculated as
 * MD5(Code+ID+Length+RequestAuthenticator+Attributes+Secret)
 */
-   cryptvector = palloc(packetlength + strlen(secret));
+   cryptvector = palloc(packetlength + secretlen);
 
memcpy(cryptvector, receivepacket, 4);  /* code+id+length */
memcpy(cryptvector + 4, packet->vector, RADIUS_VECTOR_LENGTH);  
/* request
@@ -3338,10 +3343,10 @@
if (packetlength > RADIUS_HEADER_LENGTH)/* there may be 
no

 * attributes at all */
memcpy(cryptvector + RADIUS_HEADER_LENGTH, 
receive_buffer + RADIUS_HEADER_LENGTH, packetlength - RADIUS_HEADER_LENGTH);
-   memcpy(cryptvector + packetlength, secret, strlen(secret));
+   memcpy(cryptvector + packetlength, secret, secretlen);
 
if (!pg_md5_binary(cryptvector,
-  packetlength + 
strlen(secret),
+  packetlength + secretlen,
   encryptedpassword))
{
ereport(LOG,


auth.c.patch
Description: auth.c.patch

RE: Patch avoid call strlen repeatedly in loop.

[Ranier VF]

De: Mark Dilger 
Enviado: sábado, 9 de novembro de 2019 00:12
Para: Ranier VF; pgsql-hackers@lists.postgresql.org
Assunto: 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.cFri Nov 08 14:27:17 2019
> @@ -1815,6 +1815,7 @@
>   charident_user[IDENT_USERNAME_MAX + 1];
>   pgsocketsock_fd = PGINVALID_SOCKET; /* for talking to Ident 
> server */
>   int rc; /* Return code 
> from a locally called function */
> + int ident_query_len;
>   boolident_return;
>   charremote_addr_s[NI_MAXHOST];
>   charremote_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", "");
 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

RE: Patch avoid call strlen repeatedly in loop.

[Ranier VF]

Hi Mark,
"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", "");
 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'"

Well, I've been using snprintf, no problem for several years now.
But what you reported, I would easily solve with an assert.

assert(foolen == strlen(foo));

To make sure things would stay under control.

"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."

I find it amazing that software I admire so much, such as PostgreSQL, makes 
extensive and heavy use of functions like strlen.
Speed makes a lot of difference, for some people it is above safety.
Maybe that's why PostgreSQL loses some battles against MySQL.
Not using strlen is for educational purposes as well. Allowing is to encourage 
use!
So stupid things such as:
#define CheckComplicatedStuff (a, b) (strlen (a) > strlen (b))
for (;;) {
  if CheckComplicatedStuff (x, y) {
 break;
  }
}
They start to contaminate all the code.
Using features like strlen, the programmer begins to create easy shortcuts, but 
in the end, they are very slow.

Maybe that's why I have things in my code like:
char sql [4096];
PQexec (cn, sql);

While MySQL for example, would look like this:
char sql [4096];
int sql_len;
sql_len = snprintf (sql, sizeof (sql), "INSERT ...");
mysql_real_query (cn, sql, sql_len);

"A bit more information about the performance problem you are
encountering might make it easier to understand the motivation for this
patch."
My motivation? Speed.
Win from MySQL, always.

Anyway I'm redoing the patch with your suggestion.
What about other functions that make extensive use of strlen?

Thank you.
Ranier Vilela


De: Mark Dilger 
Enviado: sábado, 9 de novembro de 2019 00:12
Para: Ranier VF; pgsql-hackers@lists.postgresql.org
Assunto: 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.cFri Nov 08 14:27:17 2019
> @@ -1815,6 +1815,7 @@
>   charident_user[IDENT_USERNAME_MAX + 1];
>   pgsocketsock_fd = PGINVALID_SOCKET; /* for talking to Ident 
> server */
>   int rc; /* Return code 
> from a locally called function */
> + int ident_query_len;
>   boolident_return;
>   charremote_addr_s[NI_MAXHOST];
>   charremote_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", "");
 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

[PATH] spell.c (avoid call strlen repeatedly in loop.

[Ranier VF]

Hi Mark,
Another example, can you take a look?

--- \dll\postgresql-12.0\a\backend\tsearch\spell.c  Mon Sep 30 17:06:55 2019
+++ spell.c Sat Nov 09 05:55:23 2019
@@ -186,7 +186,7 @@
 #define MAX_NORM 1024
 #define MAXNORMLEN 256
 
-#define STRNCMP(s,p)   strncmp( (s), (p), strlen(p) )
+#define STRNCMP(s,p)   strncmp( (s), (p), sizeof(p) - 1 )
 #define GETWCHAR(W,L,N,T) ( ((const uint8*)(W))[ ((T)==FF_PREFIX) ? (N) : ( 
(L) - 1 - (N) ) ] )
 #define GETCHAR(A,N,T)   GETWCHAR( (A)->repl, (A)->replen, N, T )
 
@@ -1220,31 +1220,31 @@
}
 
if (STRNCMP(recoded, "COMPOUNDFLAG") == 0)
-   addCompoundAffixFlagValue(Conf, recoded + 
strlen("COMPOUNDFLAG"),
+   addCompoundAffixFlagValue(Conf, recoded + 
sizeof("COMPOUNDFLAG") - 1,
  
FF_COMPOUNDFLAG);
else if (STRNCMP(recoded, "COMPOUNDBEGIN") == 0)
-   addCompoundAffixFlagValue(Conf, recoded + 
strlen("COMPOUNDBEGIN"),
+   addCompoundAffixFlagValue(Conf, recoded + 
sizeof("COMPOUNDBEGIN") - 1,
  
FF_COMPOUNDBEGIN);
else if (STRNCMP(recoded, "COMPOUNDLAST") == 0)
-   addCompoundAffixFlagValue(Conf, recoded + 
strlen("COMPOUNDLAST"),
+   addCompoundAffixFlagValue(Conf, recoded + 
sizeof("COMPOUNDLAST") - 1,
  
FF_COMPOUNDLAST);
/* COMPOUNDLAST and COMPOUNDEND are synonyms */
else if (STRNCMP(recoded, "COMPOUNDEND") == 0)
-   addCompoundAffixFlagValue(Conf, recoded + 
strlen("COMPOUNDEND"),
+   addCompoundAffixFlagValue(Conf, recoded + 
sizeof("COMPOUNDEND") - 1,
  
FF_COMPOUNDLAST);
else if (STRNCMP(recoded, "COMPOUNDMIDDLE") == 0)
-   addCompoundAffixFlagValue(Conf, recoded + 
strlen("COMPOUNDMIDDLE"),
+   addCompoundAffixFlagValue(Conf, recoded + 
sizeof("COMPOUNDMIDDLE") - 1,
  
FF_COMPOUNDMIDDLE);
else if (STRNCMP(recoded, "ONLYINCOMPOUND") == 0)
-   addCompoundAffixFlagValue(Conf, recoded + 
strlen("ONLYINCOMPOUND"),
+   addCompoundAffixFlagValue(Conf, recoded + 
sizeof("ONLYINCOMPOUND") - 1,
  
FF_COMPOUNDONLY);
else if (STRNCMP(recoded, "COMPOUNDPERMITFLAG") == 0)
addCompoundAffixFlagValue(Conf,
- 
recoded + strlen("COMPOUNDPERMITFLAG"),
+ 
recoded + sizeof("COMPOUNDPERMITFLAG") - 1,
  
FF_COMPOUNDPERMITFLAG);
else if (STRNCMP(recoded, "COMPOUNDFORBIDFLAG") == 0)
addCompoundAffixFlagValue(Conf,
- 
recoded + strlen("COMPOUNDFORBIDFLAG"),
+ 
recoded + sizeof("COMPOUNDFORBIDFLAG") - 1,
  
FF_COMPOUNDFORBIDFLAG);
else if (STRNCMP(recoded, "FLAG") == 0)
{


spell.c.patch
Description: spell.c.patch

[PATCH] Windows port: add support to setenv function

[Ranier Vf]

According to [1], windows does not support setenv.
With the possibility of setenv going further [2], I am submitting in this
thread, the patch to add setenv support on the windows side,
It is based on pre-existing functions, and seeks to correctly emulate the
functioning of the POSIX setenv, but has not yet been tested.
regards,

Ranier Vilela

[1] https://www.postgresql.org/message-id/29478.1576537771%40sss.pgh.pa.us

[2] https://www.postgresql.org/message-id/30119.1576538578%40sss.pgh.pa.us

diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index c459a2417d..1ffddcceee 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -463,6 +463,7 @@ extern void _dosmaperr(unsigned long);
 /* in port/win32env.c */
 extern int	pgwin32_putenv(const char *);
 extern void pgwin32_unsetenv(const char *);
+extern int	pgwin32_setenv(const char *name, const char *envval, int overwrite);
 
 /* in port/win32security.c */
 extern int	pgwin32_is_service(void);
@@ -473,6 +474,7 @@ extern BOOL AddUserToTokenDacl(HANDLE hToken);
 
 #define putenv(x) pgwin32_putenv(x)
 #define unsetenv(x) pgwin32_unsetenv(x)
+#define setenv(n,v,o) pgwin32_setenv(n, v, o)
 
 /* Things that exist in MinGW headers, but need to be added to MSVC */
 #ifdef _MSC_VER
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 6f4994c96a..e9845a78c8 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -123,3 +123,35 @@ pgwin32_unsetenv(const char *name)
 	pgwin32_putenv(envbuf);
 	free(envbuf);
 }
+
+
+int
+pgwin32_setenv(const char *name, const char *envval, int overwrite)
+{
+if (name == NULL)
+return -1;
+
+if (overwrite || !getenv(name)) {
+   char *envbuf;
+   int  name_len;
+
+   name_len = strlen(name);
+   if (envval != NULL) {
+   envbuf = (char *) malloc(name_len + strlen(envval) + 2);
+	   if (!envbuf)
+		   return -1;
+ 
+	   sprintf(envbuf, "%s=%s", name, envval);
+   } else {
+   envbuf = (char *) malloc(name_len + 2);
+	   if (!envbuf)
+		   return -1;
+ 
+	   sprintf(envbuf, "%s=", name);
+   }
+	   pgwin32_putenv(envbuf);
+	   free(envbuf);
+}
+
+return 0;
+}

Re: Windows port minor fixes

[Ranier Vf]

Em qua., 18 de dez. de 2019 às 15:59, Stephen Frost 
escreveu:

> >Alright, well, oddly enough, *this* email included the other headers and
> >appears threaded properly (in mutt, at least).
>
> >Did you do something different when replying to this email vs. the other
> >emails you've been replying to?
>
> Well, live outlook, is kinda dumb at that point, when responds, he add
only person email, not list email.
So the answer goes only to the person.

Also- it's custom here to "reply-all" and not to just reply to the list.
>
Ok, adjusted.

regards,
Ranier Vilela

[PATCH] remove expression always false

[Ranier Vf]

Hi,
 || curpages <= 0
expression is always false and can be safely removed.
Reasons:
1. curpages is uint32 type
2. its already test if is zero before.
3. Never be negative

regards,
Ranier Vilela
diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index 5d3f5c3f54..bf200b6180 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -640,7 +640,7 @@ table_block_relation_estimate_size(Relation rel, int32 *attr_widths,
 	 * the last VACUUM are most likely not marked all-visible.  But costsize.c
 	 * wants it converted to a fraction.
 	 */
-	if (relallvisible == 0 || curpages <= 0)
+	if (relallvisible == 0)
 		*allvisfrac = 0;
 	else if ((double) relallvisible >= curpages)
 		*allvisfrac = 1;
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 5e889d1861..651a06ff2f 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -1040,7 +1040,7 @@ estimate_rel_size(Relation rel, int32 *attr_widths,
 			 * pages added since the last VACUUM are most likely not marked
 			 * all-visible.  But costsize.c wants it converted to a fraction.
 			 */
-			if (relallvisible == 0 || curpages <= 0)
+			if (relallvisible == 0)
 *allvisfrac = 0;
 			else if ((double) relallvisible >= curpages)
 *allvisfrac = 1;