Lookups: fix pgsql multiple-row, single-column return

Report & fix from James <list@xdrv.co.uk>; additional tidying and testcase by JGH

Broken-by: acec9514b1

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 4897f4a0cc15448d236bf87e3480c6c3d78e3636..d13004b8c1bbd7acef9e4f7acc18517a87bea0f1 100644 (file)
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -16,6 +16,10 @@ JH/01 Replace the store_release() internal interface with store_newblock(),
 JH/02 Disallow '/' characters in queue names specified for the "queue=" ACL
       modifier.  This matches the restriction on the commandline.
 
 JH/02 Disallow '/' characters in queue names specified for the "queue=" ACL
       modifier.  This matches the restriction on the commandline.
 

+JH/03 Fix pgsql lookup for multiple result-tuples with a single column.
+      Previously only the last row was returned.
+
+

 Exim version 4.90
 -----------------
 
 Exim version 4.90
 -----------------
 

diff --git a/src/src/lookups/pgsql.c b/src/src/lookups/pgsql.c
index 0b771f59cae389302f0bc563b35e6e85a04c7250..cece43b66b4750a744bb4eb4593f7cb477dc73f9 100644 (file)
--- a/src/src/lookups/pgsql.c
+++ b/src/src/lookups/pgsql.c
@@ -140,7 +140,7 @@ has the password removed. This copy is also used for debugging output. */
 for (i = 2; i >= 0; i--)
   {
   uschar *pp = Ustrrchr(server, '/');
 for (i = 2; i >= 0; i--)
   {
   uschar *pp = Ustrrchr(server, '/');

-  if (pp == NULL)
+  if (!pp)

     {
     *errmsg = string_sprintf("incomplete pgSQL server data: %s",
       (i == 2)? server : server_copy);
     {
     *errmsg = string_sprintf("incomplete pgSQL server data: %s",
       (i == 2)? server : server_copy);


@@ -156,18 +156,16 @@ for (i = 2; i >= 0; i--)
 start is the identification of the server (host or path). See if we have a
 cached connection to the server. */
 
 start is the identification of the server (host or path). See if we have a
 cached connection to the server. */
 

-for (cn = pgsql_connections; cn != NULL; cn = cn->next)
-  {
+for (cn = pgsql_connections; cn; cn = cn->next)

   if (Ustrcmp(cn->server, server_copy) == 0)
     {
     pg_conn = cn->handle;
     break;
     }
   if (Ustrcmp(cn->server, server_copy) == 0)
     {
     pg_conn = cn->handle;
     break;
     }

-  }

 
 /* If there is no cached connection, we must set one up. */
 
 
 /* If there is no cached connection, we must set one up. */
 

-if (cn == NULL)
+if (!cn)

   {
   uschar *port = US"";
 
   {
   uschar *port = US"";
 


@@ -178,7 +176,7 @@ if (cn == NULL)
     uschar *last_slash, *last_dot, *p;
 
     p = ++server;
     uschar *last_slash, *last_dot, *p;
 
     p = ++server;

-    while (*p != 0 && *p != ')') p++;
+    while (*p && *p != ')') p++;

     *p = 0;
 
     last_slash = Ustrrchr(server, '/');
     *p = 0;
 
     last_slash = Ustrrchr(server, '/');


@@ -191,10 +189,9 @@ if (cn == NULL)
     We have to call PQsetdbLogin with '/var/run/postgresql' as the hostname
     argument and put '5432' into the port variable. */
 
     We have to call PQsetdbLogin with '/var/run/postgresql' as the hostname
     argument and put '5432' into the port variable. */
 

-    if (last_slash == NULL || last_dot == NULL)
+    if (!last_slash || !last_dot)

       {
       {

-      *errmsg = string_sprintf("PGSQL invalid filename for socket: %s",
-        server);
+      *errmsg = string_sprintf("PGSQL invalid filename for socket: %s", server);

       *defer_break = TRUE;
       return DEFER;
       }
       *defer_break = TRUE;
       return DEFER;
       }


@@ -211,13 +208,13 @@ if (cn == NULL)
   else
     {
     uschar *p;
   else
     {
     uschar *p;

-    if ((p = Ustrchr(server, ':')) != NULL)
+    if ((p = Ustrchr(server, ':')))

       {
       *p++ = 0;
       port = p;
       }
 
       {
       *p++ = 0;
       port = p;
       }
 

-    if (Ustrchr(server, '/') != NULL)
+    if (Ustrchr(server, '/'))

       {
       *errmsg = string_sprintf("unexpected slash in pgSQL server hostname: %s",
         server);
       {
       *errmsg = string_sprintf("unexpected slash in pgSQL server hostname: %s",
         server);


@@ -280,37 +277,37 @@ else
 
 /* Run the query */
 
 
 /* Run the query */
 

-  pg_result = PQexec(pg_conn, CS query);
-  switch(PQresultStatus(pg_result))
-    {
-    case PGRES_EMPTY_QUERY:
-    case PGRES_COMMAND_OK:
-      /* The command was successful but did not return any data since it was
-      not SELECT but either an INSERT, UPDATE or DELETE statement. Tell the
-      high level code to not cache this query, and clean the current cache for
-      this handle by setting *do_cache zero. */
-
-      result = string_cat(result, US PQcmdTuples(pg_result));
-      *do_cache = 0;
-      DEBUG(D_lookup) debug_printf("PGSQL: command does not return any data "
-       "but was successful. Rows affected: %s\n", result->s);
-      break;
+pg_result = PQexec(pg_conn, CS query);
+switch(PQresultStatus(pg_result))
+  {
+  case PGRES_EMPTY_QUERY:
+  case PGRES_COMMAND_OK:
+    /* The command was successful but did not return any data since it was
+    not SELECT but either an INSERT, UPDATE or DELETE statement. Tell the
+    high level code to not cache this query, and clean the current cache for
+    this handle by setting *do_cache zero. */
+
+    result = string_cat(result, US PQcmdTuples(pg_result));
+    *do_cache = 0;
+    DEBUG(D_lookup) debug_printf("PGSQL: command does not return any data "
+      "but was successful. Rows affected: %s\n", string_from_gstring(result));
+    break;

 
 

-    case PGRES_TUPLES_OK:
-      break;
+  case PGRES_TUPLES_OK:
+    break;

 
 

-    default:
-      /* This was the original code:
-      *errmsg = string_sprintf("PGSQL: query failed: %s\n",
-                              PQresultErrorMessage(pg_result));
-      This was suggested by a user:
-      */
-
-      *errmsg = string_sprintf("PGSQL: query failed: %s (%s) (%s)\n",
-                             PQresultErrorMessage(pg_result),
-                             PQresStatus(PQresultStatus(pg_result)), query);
-      goto PGSQL_EXIT;
-    }
+  default:
+    /* This was the original code:
+    *errmsg = string_sprintf("PGSQL: query failed: %s\n",
+                            PQresultErrorMessage(pg_result));
+    This was suggested by a user:
+    */
+
+    *errmsg = string_sprintf("PGSQL: query failed: %s (%s) (%s)\n",
+                          PQresultErrorMessage(pg_result),
+                          PQresStatus(PQresultStatus(pg_result)), query);
+    goto PGSQL_EXIT;
+  }

 
 /* Result is in pg_result. Find the number of fields returned. If this is one,
 we don't add field names to the data. Otherwise we do. If the query did not
 
 /* Result is in pg_result. Find the number of fields returned. If this is one,
 we don't add field names to the data. Otherwise we do. If the query did not


@@ -329,7 +326,7 @@ for (i = 0; i < num_tuples; i++)
     result = string_catn(result, US"\n", 1);
 
   if (num_fields == 1)
     result = string_catn(result, US"\n", 1);
 
   if (num_fields == 1)

-    result = string_catn(NULL,
+    result = string_catn(result,

        US PQgetvalue(pg_result, i, 0), PQgetlength(pg_result, i, 0));
   else
     {
        US PQgetvalue(pg_result, i, 0), PQgetlength(pg_result, i, 0));
   else
     {


@@ -342,17 +339,13 @@ for (i = 0; i < num_tuples; i++)
     }
   }
 
     }
   }
 

-/* If result is NULL then no data has been found and so we return FAIL.
-Otherwise, we must terminate the string which has been built; string_cat()
-always leaves enough room for a terminating zero. */
+/* If result is NULL then no data has been found and so we return FAIL. */

 
 if (!result)
   {
   yield = FAIL;
   *errmsg = US"PGSQL: no data found";
   }
 
 if (!result)
   {
   yield = FAIL;
   *errmsg = US"PGSQL: no data found";
   }

-else
-  store_reset(result->s + result->ptr + 1);

 
 /* Get here by goto from various error checks. */
 
 
 /* Get here by goto from various error checks. */
 


@@ -367,6 +360,7 @@ if (pg_result) PQclear(pg_result);
 
 if (result)
   {
 
 if (result)
   {

+  store_reset(result->s + result->ptr + 1);

   *resultptr = string_from_gstring(result);
   return OK;
   }
   *resultptr = string_from_gstring(result);
   return OK;
   }

diff --git a/test/scripts/2620-Postgresql/2620 b/test/scripts/2620-Postgresql/2620
index 62ee31354c1a3c3ccbf80763b9d01e2934cc2238..ba3a3bcc82fd6f5445d684b85d42bb642d59a94d 100644 (file)
--- a/test/scripts/2620-Postgresql/2620
+++ b/test/scripts/2620-Postgresql/2620
@@ -13,6 +13,7 @@ system 'createdb -h localhost -p PORT_N test';
 system 'psql -h localhost -p PORT_N -d test \
  -c "CREATE TABLE them ( name text, id text );" \
  -c "INSERT INTO them VALUES ( \'Philip Hazel\', \'ph10\' );" \
 system 'psql -h localhost -p PORT_N -d test \
  -c "CREATE TABLE them ( name text, id text );" \
  -c "INSERT INTO them VALUES ( \'Philip Hazel\', \'ph10\' );" \

+ -c "INSERT INTO them VALUES ( \'Aristotle\',    \'aaaa\' );" \

  -c "INSERT INTO them VALUES ( \'\', \'nothing\' );" \
  -c "INSERT INTO them VALUES ( \'\"stquot\', \'quote2\' );" \
  -c "INSERT INTO them VALUES ( \'before\' || CHR(13) || CHR(10) || \'after\', \'newline\' );" \
  -c "INSERT INTO them VALUES ( \'\', \'nothing\' );" \
  -c "INSERT INTO them VALUES ( \'\"stquot\', \'quote2\' );" \
  -c "INSERT INTO them VALUES ( \'before\' || CHR(13) || CHR(10) || \'after\', \'newline\' );" \


@@ -37,6 +38,9 @@ ${lookup pgsql {servers=x:localhost; select name from them where id='ph10';}}
 ${lookup pgsql {servers=localhost::PORT_N:x; select name from them where id='ph10';}}
 ${lookup pgsql {servers=localhost::PORT_N/test/CALLER/:x; select name from them where id='ph10';}}
 ${lookup pgsql {servers=(DIR/pgsql/.s.PGSQL.PORT_N)/test/CALLER/:x; select name from them where id='ph10';}}
 ${lookup pgsql {servers=localhost::PORT_N:x; select name from them where id='ph10';}}
 ${lookup pgsql {servers=localhost::PORT_N/test/CALLER/:x; select name from them where id='ph10';}}
 ${lookup pgsql {servers=(DIR/pgsql/.s.PGSQL.PORT_N)/test/CALLER/:x; select name from them where id='ph10';}}

+x
+${lookup pgsql {SELECT name FROM them WHERE id IN ('ph10', 'aaaa');}}
+${lookup pgsql {SELECT *    FROM them WHERE id IN ('ph10', 'aaaa');}}

 ****
 exim -d -bh 10.0.0.0
 mail from:<a@b>
 ****
 exim -d -bh 10.0.0.0
 mail from:<a@b>

diff --git a/test/stderr/2620 b/test/stderr/2620
index adc0c64a663708bfd19f4661b7770e0a31e95531..b0bed620945752c665bf629bd510addfc98405f7 100644 (file)
--- a/test/stderr/2620
+++ b/test/stderr/2620
@@ -159,6 +159,30 @@ database lookup required for servers=(TESTSUITE/pgsql/.s.PGSQL.1223)/test/CALLER
 PostgreSQL query: servers=(TESTSUITE/pgsql/.s.PGSQL.1223)/test/CALLER/:x; select name from them where id='ph10';
 PGSQL new connection: socket=TESTSUITE/pgsql/.s.PGSQL.1223 database=test user=CALLER
 lookup yielded: Philip Hazel
 PostgreSQL query: servers=(TESTSUITE/pgsql/.s.PGSQL.1223)/test/CALLER/:x; select name from them where id='ph10';
 PGSQL new connection: socket=TESTSUITE/pgsql/.s.PGSQL.1223 database=test user=CALLER
 lookup yielded: Philip Hazel

+search_open: pgsql "NULL"
+  cached open
+search_find: file="NULL"
+  key="SELECT name FROM them WHERE id IN ('ph10', 'aaaa');" partial=-1 affix=NULL starflags=0
+LRU list:
+internal_search_find: file="NULL"
+  type=pgsql key="SELECT name FROM them WHERE id IN ('ph10', 'aaaa');"
+database lookup required for SELECT name FROM them WHERE id IN ('ph10', 'aaaa');
+PostgreSQL query: SELECT name FROM them WHERE id IN ('ph10', 'aaaa');
+PGSQL using cached connection for localhost:1223/test/CALLER
+lookup yielded: Philip Hazel
+Aristotle
+search_open: pgsql "NULL"
+  cached open
+search_find: file="NULL"
+  key="SELECT *    FROM them WHERE id IN ('ph10', 'aaaa');" partial=-1 affix=NULL starflags=0
+LRU list:
+internal_search_find: file="NULL"
+  type=pgsql key="SELECT *    FROM them WHERE id IN ('ph10', 'aaaa');"
+database lookup required for SELECT *    FROM them WHERE id IN ('ph10', 'aaaa');
+PostgreSQL query: SELECT *    FROM them WHERE id IN ('ph10', 'aaaa');
+PGSQL using cached connection for localhost:1223/test/CALLER
+lookup yielded: name="Philip Hazel" id=ph10 
+name=Aristotle id=aaaa 

 search_tidyup called
 close PGSQL connection: (TESTSUITE/pgsql/.s.PGSQL.1223)/test/CALLER
 close PGSQL connection: localhost:1223/test/CALLER
 search_tidyup called
 close PGSQL connection: (TESTSUITE/pgsql/.s.PGSQL.1223)/test/CALLER
 close PGSQL connection: localhost:1223/test/CALLER

diff --git a/test/stdout/2620 b/test/stdout/2620
index c76e65085d69ce2885fbc54982e9701bf953dea1..668eca73acf011885831fd1ca15f9ffa56d2c55a 100644 (file)
--- a/test/stdout/2620
+++ b/test/stdout/2620
@@ -28,6 +28,7 @@ INSERT 0 1
 INSERT 0 1
 INSERT 0 1
 INSERT 0 1
 INSERT 0 1
 INSERT 0 1
 INSERT 0 1

+INSERT 0 1

 > Philip Hazel
 > Philip Hazel
 > 
 > Philip Hazel
 > Philip Hazel
 > 


@@ -43,6 +44,11 @@ after" id=newline
 > Philip Hazel
 > Philip Hazel
 > Philip Hazel
 > Philip Hazel
 > Philip Hazel
 > Philip Hazel

+> x
+> Philip Hazel
+Aristotle
+> name="Philip Hazel" id=ph10 
+name=Aristotle id=aaaa 

 > 
 
 **** SMTP testing session as if from host 10.0.0.0
 > 
 
 **** SMTP testing session as if from host 10.0.0.0