pg_stat_statements with query tree based normalization

Attached is a patch and associated test program that adds query 
normalization of pg_stat_statements, based on transforming the query 
tree into a series of integers and using them to match against previous 
queries.  Currently pg_stat_statements only works usefully if all 
queries executed use prepared statements.  This is a troublesome 
limitation for a couple of reasons.  Prepared statements can easily have 
performance regressions compared to the unprepared version, since the 
optimizer won't necessarily be able to use as much data about MCVs etc. 
as in the regular case.  People regularly avoid those for that 
performance reason, and there are ORM limitations here too.  Someone was 
telling me the other day Ruby just added prepared statement support 
recently as one example.

That means that instead of using the existing pg_stat_statements 
extension, most sites have to log statements to a text file, and then 
parse those logs using programs like pgfouine.  The whole process is 
inefficient, makes the database look clumsy, and is a major source of 
complaints about PostgreSQL.  In many shared/managed hosting 
environments, looking at things exposed at the database-level is vastly 
easier than getting at the log files on the server too.  There's several 
legitimate complaints here that make this problem rise to be an 
advocacy/adoption issue as far as I'm concerned.

Code and this whole idea by Peter Geoghegan, with me as first round 
reviewer and diff minimizing nitpicker.  Thanks to attendees and 
sponsors of the PgWest conference for helping to fund initial 
exploration of server side query normalization using regular 
expressions.  The more complicated query tree approach used here was 
sponsored by Heroku.  Hurray for parallel development tracks, I was 
happy to take my regex idea to the bike shed and put it out of its 
misery once this one worked.

Here's a quick demo that's based on the existing sample in the docs.  
Build the server and the contrib modules for pg_stat_statements and 
pgbench, edit postgresql.conf to add:

shared_preload_libraries = 'pg_stat_statements'
pg_stat_statements.max = 10000
pg_stat_statements.track = all

Then test like this using pgbench:

createdb bench
psql -d bench -c "CREATE EXTENSION pg_stat_statements"
pgbench -i bench
psql -d bench -c "SELECT pg_stat_statements_reset()"
pgbench -c10 -t30 bench
psql -x -d bench -c "SELECT query, calls, total_time, rows FROM 
pg_stat_statements ORDER BY total_time DESC LIMIT 5;"

Unlike the example in the docs, there's no "-M prepared" here, but as 
hoped all the similar statements are grouped together anyway.  The no 
log file parsing or regex necessary output looks like this:

-[ RECORD 1 
]------------------------------------------------------------------
query      | UPDATE pgbench_branches SET bbalance = bbalance + ? WHERE 
bid = ?;
calls      | 300
total_time | 1.084803
rows       | 300
-[ RECORD 2 
]------------------------------------------------------------------
query      | UPDATE pgbench_tellers SET tbalance = tbalance + ? WHERE 
tid = ?;
calls      | 300
total_time | 0.875213
rows       | 300
...

Interestingly it even showed vacuum running against pgbench_branches in 
my test case.

I've been focused on usability testing, and we could really use someone 
familiar with the query node internals to review those changes too.  
Probably the most controversial detail here is how exactly the "query" 
field here is generated, and that's the newest code too.  I feel 
strongly that we need to have a stable string there for common use 
cases, so that successive views of this data across time or even server 
restarts will have the same key for that query's name.  That's what 
administrations really expect this sort of feature to do.  I consider it 
a feature that you can tell this form of normalization from the type 
that you get from a proper prepared statement, where these parameters 
would be named "$1" etc.  There are surely some edge cases where this 
may need escaping around, I haven't really dug into looking for those yet.

The approach Peter used adds a single integer to the Const structure in 
order to have enough information to substitute "?" in place of those.  
Adding and maintaining that is the only change outside of the extension 
made here, and that overhead is paid by everyone--not just consumers of 
this new code.  Neither of us like that, but if we're going to increase 
something Const is not a bad choice; it's not that slim of a structure 
already.  There's 7 int length fields, a Datum, and two booleans in 
there.  It's going from about 44 bytes to 48, so maybe a 10% size 
increase in that one node type.  I can't even measure a performance 
change on simple pgbench tests, run variation is way bigger.  Might be 
easier to measure if your query was mostly Const nodes now that I think 
about it.

If someone can come up with another way to implement this that avoid 
that overhead, obviously we'd do whatever we could to eliminate it.  I 
don't think it's unreasonable server bloat given how valuable this 
feature is.  As I recently mentioned in the other thread that whacks 
around this extension, it's possible to install and use most of this 
code as an extension, you just don't have this part and accordingly the 
query text is less stable.  That's not a code consideration, but 
presuming that's doesn't complicate the 9.2 implementation I hear enough 
requests for this feature that I think it is a useful bonus for the 
community,   If some form of this gets committed, I'd expect the updated 
pg_stat_statements to be fairly popular as an extension, packaged to add 
this to 8.4 through 9.1 servers.

Back to the code...an extensive regression testing program for this new 
feature is included, which has made it much easier to modify the 
underlying implementation without breaking it.  (If David Wheeler has 
read this far down, he's now laughing at me)  It verifies many different 
statements that should be considered the same are.  Running the program 
requires Python, psycopg2, and the Dell Store 2 database:  
http://pgfoundry.org/forum/forum.php?forum_id=603  Exactly how much 
testing of this should be done using the PostgreSQL regression tests 
instead is one of the things I don't have a good recommendation on yet.

The latest version of this patch right now is at 
https://github.com/greg2ndQuadrant/postgres/tree/pg_stat_statements_norm 
with their neato diff view at 
https://github.com/greg2ndQuadrant/postgres/compare/master...pg_stat_statements_norm  
That currently has only minor cleanup drift from Peter's development one 
at https://github.com/Peter2ndQuadrant/postgres/branches which is the 
place to watch for updates.  The git repo tree includes both the changes 
and the regression test program, but I don't expect the test program to 
be committed in core.

There are some known bugs/limitations that Peter and I have found in 
recent testing of the program, all of which are fixable given a bit more 
development time.  Just to really invert the status quo, my testing 
showed this doesn't report correct numbers when using prepared 
statements right now.  Peter tells me the code presently assumes that 
there is one call to the planner hook followed by calls to the normal 
executor hooks, each set of calls corresponding to one statement.  And 
that assumption falls down when prepared statements are used.  Tom 
mentioned something about synchronization issues in this area on the 
other pg_stat_statements thread too, so it's not a surprise so much as a 
known sticky point in the implementation.  Peter has a fix in mind for 
this already; I wanted to get community feedback moving rather than 
block waiting for that.

Timestamp literals aren't normalized properly yet, that just slipped 
past the submission deadline.  There are some DEBUG1 logging statements 
still around that Peter wanted to kill before submission.  I thought 
they should stay around for initial review at least.

As for documentation, I consider this so valuable to server operation 
that I think I want to mention it beyond just the notes in contrib on 
how to use it.  Still muddling over what I want to say there.

I also wonder if it makes sense to allow disabling this feature, just 
for people who want the rest of pg_stat_statements but not paying for 
this.  The main use case I could see for that are people who are using 
prepared statements, and are happy with the existing implementation.  
Again, I didn't want to dive too deep into measuring overhead when it's 
quite possible we'll get feedback that requires rework making that 
redundant.  Suggestions for anything from the usage to implementation 
approach is welcome.  As I'm sure it's clear by the end of this long 
commentary, this is considered an important feature here.

-- 
Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us