Re: Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)

On 29 February 2012 09:05, Daniel Farina <daniel(at)heroku(dot)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