Re: pl/pgSQL, get diagnostics and big data - Mailing list pgsql-hackers

* Andreas 'ads' Scherbaum wrote:

> one of our customers approached us and complained, that GET DIAGNOSTICS
> row_count returns invalid results if the number of rows is > 2^31. It's

> Attached patch expands the row_count to 64 bit.
>
> diagnostics=# select testfunc_pg((2^32 + 50000)::bigint);
>   testfunc_pg
> -------------
>    4295017296
> (1 row)

This is my first patch review, but I have to get my feet wet at some 
point, and this is a nice, small patch to do that.

Following the checklist from the wiki:

- Is the patch in context format: Yes.

- Does it apply cleanly to master: Yes.

- Does it include reasonable tests, doc patches, etc.: No. While  it would be nice if it had some, a test that inserts
2^32rows  will take a while and can hardly be called reasonable.
 


The patch is designed to expand the size of the "affected records" count 
in the command tag from 32 to 64 bits.

- Does it do that: Yes.

- Do we want that: Yes, because it is motivated by reports from users  who have queries like that in real life.

- Do we already have it: No.

- Does it follow SQL spec or community-agreed behavior: This is not  covered by the SQL standard and there has not, to
myknowledge, been  any discussion on this point on -hackers. It is, however, the  obvious approach to solving the
specificissue.
 

- Does it include pg_dump support: n/a

- Are there dangers: Existing applications and client libraries must  support the increased maximum size (up to nine
additionaldigits) and  maximum value. libpq apparently does not parse the command tag, only  stores it as a string for
retrievalby PQcmdStatus(), so it is not  affected in terms of parsing the value, and for storage, it uses a
64-characterbuffer, which will overflow if the command name part of  the tag exceeds 32 characters (63 - 19 [row count]
-10 [OID] - 2  [spaces]). The longest command name I can think of is "REFRESH  MATERIALIZED VIEW" which, at 25
characters,stays comfortably below  this limit, and does not include a row count anyway.
 

- Have all the bases been covered: The patch changes all locations  where the command tag is formatted, and where the
recordcount is  retrieved by PL/pgSQL.
 

- Does the patch follow the coding guidelines: I believe so.

- Are there portability issues/Will it work on Windows/BSD etc.:
  No, it will not work correctly on Windows when built with MSVC,  although it may work with MinGW.
  +++ postgresql-9.5.0/src/backend/tcop/pquery.c  @@ -195,7 +195,7 @@   {     case CMD_SELECT:
snprintf(completionTag,COMPLETION_TAG_BUFSIZE,  -             "SELECT %u", queryDesc->estate->es_processed);  +
   "SELECT %lu", queryDesc->estate->es_processed);
 

  %lu formats unsigned long. "long" is problematic in terms of  portability, because sizeof(long) is different
everywhere.It is 32  bits on Windows and on 32-bit *nix, and 64 bits on 64-bit *nix.
 
  I added the following line to the INSERT formatting in pquery.c:
queryDesc->estate->es_processed += 471147114711LL;
  This number is 0x6DB28E70D7; so inserting one row should return  "INSERT 0 2995679448" (0xB28E70D8):
postgres=# insert into t1 values (0);INSERT 0 2995679448
  To fix this, I think it will be enough to change the format strings to  use "%zu" instead of "%lu". pg_snprintf() is
selectedby configure if  the platform's snprintf() does not support the "z" conversion. I tried  this, and it appears
towork:
 
postgres=# insert into t1 values (0);INSERT 0 471147114712
  I have looked for other uses of "%lu", and found none that may cause  the same issue; apparently they are all used
withvalues that clearly  have 32-bit type; actually, most of them are used to format error  codes in Windows-specific
code.

- Are the comments sufficient and accurate: Yes.

- Does it do what it says, correctly: Yes, except for the Windows thing.

- Does it produce compiler warnings: No. First, pg_snprintf() does not  use the system implementation, and second, a
warning(C4477) for  this kind of format string mismatch was only added in VS2015, which  is not officially supported
(itworks for me).
 

- Can you make it crash: No. The problematic argument always appears  last in the sprintf() calls, so the format string
issueshould not  be exploitable.
 

I did not run the regression tests or do the "performance" sections 
after I found the Windows issue. I do not think it will negatively 
affect performance, though.

In all, if replacing four "l"s with "z"s is indeed enough, I think this 
patch is an appropriate solution for solving the underlying issue.

-- 
Christian