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^32 rows
  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 my knowledge, been
  any discussion on this point on -hackers. It is, however, the
  obvious approach to solving the specific issue.

- 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 additional digits) and
  maximum value. libpq apparently does not parse the command tag, only
  stores it as a string for retrieval by 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 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 record count 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 selected by 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 values 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 (it works for me).

- Can you make it crash: No. The problematic argument always appears
  last in the sprintf() calls, so the format string issue should 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, 

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.

The new status of this patch is: Waiting on Author

Sent via pgsql-hackers mailing list (
To make changes to your subscription:

Reply via email to