Re: pl/pgSQL, get diagnostics and big data

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       tested, failed
Spec compliant:           not tested
Documentation:            not tested

* 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-character
buffer,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
comfortablybelow 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 to
work:
   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 with
valuesthat 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 (it
worksfor 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
willnegatively 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
theunderlying issue. 

The new status of this patch is: Waiting on Author