review - pg_stat_statements

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:

* 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