review - pg_stat_statements - Mailing list pgsql-hackers
| From | Pavel Stehule |
|---|---|
| Subject | review - pg_stat_statements |
| Date | |
| Msg-id | CAFj8pRCza0jPu+Q58ajG+pXKSVyX9LWYW7J06ediPpjD3YTG_A@mail.gmail.com Whole thread Raw |
| Responses |
Re: review - pg_stat_statements
(Peter Geoghegan <pg@heroku.com>)
|
| List | pgsql-hackers |
Hello all
I did check of pg_stat_statements_ext_text.v2.2013_11_16.patch, that introduce a external storage for query text
I got a compilation warning:
bash-4.1$ make all
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o pg_stat_statements.o pg_stat_statements.c
pg_stat_statements.c: In function ‘query_text_retrieve’:
pg_stat_statements.c:1477:3: warning: format ‘%lu’ expects type ‘long unsigned int’, but argument 4 has type ‘off_t’
pg_stat_statements.c: In function ‘garbage_collect_query_texts’:
pg_stat_statements.c:1796:2: warning: format ‘%ld’ expects type ‘long int’, but argument 3 has type ‘off_t’
pg_stat_statements.c:1796:2: warning: format ‘%ld’ expects type ‘long int’, but argument 4 has type ‘off_t’
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -shared -o pg_stat_statements.so pg_stat_statements.o -L../../src/port -L../../src/common -Wl,--as-needed -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags
my environment:
bash-4.1$ uname -a
Linux nemesis 2.6.35.14-106.fc14.i686 #1 SMP Wed Nov 23 13:57:33 UTC 2011 i686 i686 i386 GNU/Linux
bash-4.1$ gcc --version
gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4)
Next, usual queries, and my replies:bash-4.1$ make all
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o pg_stat_statements.o pg_stat_statements.c
pg_stat_statements.c: In function ‘query_text_retrieve’:
pg_stat_statements.c:1477:3: warning: format ‘%lu’ expects type ‘long unsigned int’, but argument 4 has type ‘off_t’
pg_stat_statements.c: In function ‘garbage_collect_query_texts’:
pg_stat_statements.c:1796:2: warning: format ‘%ld’ expects type ‘long int’, but argument 3 has type ‘off_t’
pg_stat_statements.c:1796:2: warning: format ‘%ld’ expects type ‘long int’, but argument 4 has type ‘off_t’
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -shared -o pg_stat_statements.so pg_stat_statements.o -L../../src/port -L../../src/common -Wl,--as-needed -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags
my environment:
bash-4.1$ uname -a
Linux nemesis 2.6.35.14-106.fc14.i686 #1 SMP Wed Nov 23 13:57:33 UTC 2011 i686 i686 i386 GNU/Linux
bash-4.1$ gcc --version
gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4)
* This patch works as was expected and proposed
* The code is well commented and respects PostgreSQL coding standards
* I didn't find any problems when I tested it
* I tried do some basic benchmark, and I didn't see any negative on performance related to implemented feature
* We would to this patch - a idea of proposal is right - a shared memory can be used better than storage of possibly extra long queries
Peter does some warning about performance in feature proposal http://www.postgresql.org/message-id/CAM3SWZRYYnfwXi3r3eDAKWBOYtaf1_PwGJxTAyddNSbjAfDzjA@mail.gmail.com . The risks are not negligible mainly on environments with slow IO and high varied load. I have no idea, how to eliminate this risks with possibilities that we have on extensions (maybe divide global stat file to smaller per database - it should not be solution for some configuration too). A enough solution (for this release) should better explanation in documentation. For future releases, we should to think about significant refactoring - moving to core and using asynchronous stats (and stats per database) or using specialized background worker
Peter mentioned a race conditions under high load. It is fixed?
summary:
========
========
* compilation warning
* undocumented new risks of waiting to locks when new query is identified and processed
Regards
Pavel
pgsql-hackers by date: