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,
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.
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
