Re: pl/pgSQL, get diagnostics and big data
От | Christian Ullrich |
---|---|
Тема | Re: pl/pgSQL, get diagnostics and big data |
Дата | |
Msg-id | n9dei3$bn7$1@ger.gmane.org обсуждение исходный текст |
Ответ на | pl/pgSQL, get diagnostics and big data (Andreas 'ads' Scherbaum <adsmail@wars-nicht.de>) |
Ответы |
Re: pl/pgSQL, get diagnostics and big data
(Christian Ullrich <chris@chrullrich.net>)
Re: pl/pgSQL, get diagnostics and big data (Christian Ullrich <chris@chrullrich.net>) |
Список | 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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Filip RembiałkowskiДата:
Сообщение: Re: proposal: make NOTIFY list de-duplication optional