Re: Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation) - Mailing list pgsql-hackers
From | Peter Geoghegan |
---|---|
Subject | Re: Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation) |
Date | |
Msg-id | CAEYLb_VaaO2h6uchvL4gepHBGgOPiHRisoWDaA2_SGr0vqvCHA@mail.gmail.com Whole thread Raw |
In response to | Re: Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation) (Daniel Farina <daniel@heroku.com>) |
Responses |
Re: Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
(Alvaro Herrera <alvherre@commandprompt.com>)
Re: Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation) (Tom Lane <tgl@sss.pgh.pa.us>) |
List | pgsql-hackers |
On 29 February 2012 09:05, Daniel Farina <daniel@heroku.com> wrote: > To shed some light on that hypothesis, attached is a patch whereby I > use 'semantic analysis by compiler error' to show the extent of the > reach of the changes by renaming (codebase-wide) the Const node's > location symbol. The scope whereby the error token will change > position is small and amenable to analysis. I don't see a problem, > nor wide-reaching consequences. As Peter says: probably dead code. Thanks for confirming that. I decided to benchmark this patch against the same server with shared_preload_libraries commented out. I chose a quite unsympathetic pgbench-tools benchmark - the pgbench-tools config is attached. This is the same server/configuration that I used for my recent page checksums benchmark. I've thrown the full report up on: http://pgbenchstatstatements.staticloud.com/ Executive summary: It looks like we take a 1% - 2.5% hit. On a workload like this, where parser overhead is high, that isn't bad at all, and seems at most marginally worse than classic pg_stat_statements with prepared statements, according to independently produced benchmarks that I've seen. Had I benchmarked "-M prepared", I wouldn't be surprised if there was an improvement over classic pg_stat_statements for some workloads, since the pgss_match_fn logic doesn't involve a strcmp now - it just compares scalar values. There is no question of there being a performance regression. Certainly, this patch adds a very practical feature, vastly more practical than auto_explain currently is, for example. I didn't choose the most unsympathetic of all benchmarks that could be easily conducted, which would have been a select-only workload, which executes very simple select statements only, as fast as it possibly can. I only avoided that because the tpc-b.sql workload seems to be recognised as the most useful and objective workload for general purpose benchmarks. I've attached the revision of the patch that was benchmarked. There have been a few changes, mostly bug-fixes and clean-ups, including: * Most notably, I went ahead and made the required changes to parse coercion's alteration of Const location, while also tweaking similar logic for Param location analogously, though that change was purely for consistency and not out of any practical need to do so. * Removing the unneeded alteration gave me leeway to considerably clean up the scanner logic, which doesn't care about which particular type of token is scanned anymore. There is a single invocation per query string to be canonicalised (i.e. for each first call of the query not in the shared hash table). This seems a lot more robust and correct (in terms of how it canonicalises queries like: select integer '5') than the workaround that I had in the last revision, written when it wasn't clear that I'd be able to get the core system to consistently tell the truth about Const location. * We no longer canonicalise query strings in the event of prepared statements, while still walking the query tree to compute a queryId. Of course, an additional benefit of this patch is that it allows differentiation of queries that only differ beyond track_activity_query_size bytes, which is a benefit that I want for prepared statements too. * The concept of a "sticky" entry is introduced; this prevents queries from being evicted after parse analysis/canonicalisation but before a reprieve-delivering query execution. There is still no absolute, iron-clad guarantee that this can't happen, but it is probably impossible for all practical purposes, and even when it does happen, the only consequence is that a query string with some old, uncanonicalized constants is seen, probably before being immediately evicted anyway due to the extreme set of circumstances that would have been required to produce that failure mode. If, somehow, a sticky entry is never demoted to a regular entry in the corresponding executor hook call, which ought to be impossible, that sticky entry still won't survive a restart, so problems with the shared hash table getting clogged with sticky entries should never occur. Prepared statements will add zero call entries to the table during their initial parse analysis, but these entries are not sticky, and have their "usage" value initialised just as before. * 32-bit hash values are now used. Fewer changes still to core code generally. * Merged against master - Robert's changes would have prevented my earlier patch from cleanly applying. * Even more tests! Updated regression tests attached, with a total of 289 tests. Those aside, I found the fuzz testing of third party regression tests that leverage Postgres to be useful. Daniel pointed out to me that the SQL Alchemy regression tests broke the patch due to an assertion failure. Obviously I've fixed that, so both the standard postgres and the SQL Alchemy tests do not present the patch with any difficulties. They are both fairly extensive. Thoughts? -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services
Attachment
pgsql-hackers by date: