Re: review - pg_stat_statements

Hello

2013/12/1 Peter Geoghegan <pg(at)heroku(dot)com>

> I've produced another revision, attached. Changes:
>
> * Fixes the compiler warnings on your environment.
>
> * Normalizes query string with the shared lock held (not always, just
> in case its needed). In master, this doesn't have to happen with a
> shared lock held, so because of this, but also because of the file
> writing there is probably *some* small regression against master when
> the cache is under lots of pressure. Since we have to append the file,
> and need to do so with the shared lock held, there is no way to check
> that the normalized text is really needed (i.e. that the entry doesn't
> exist already) while taking the opportunity, with the shared lock
> already held, to create a shared-lock-only query text file entry. This
> is logical; we shouldn't normalize query texts unless we know we'll
> need them, even if this implies that we must do so with a shared lock
> held when we do, because that ought to be highly infrequent.
>
> * pg_stat_statemens.max default is increased to 5,000. It feels like
> this should be increased in light of the drastically reduced memory
> footprint of pg_stat_statements. Besides, the best way of handling
> whatever modest regression I may have created is to simply make cache
> pressure very light, so that new entries are created infrequently. As
> I've mentioned previously, any regression I may have created could not
> possibly negatively affect something like a pgbench workload with only
> a handful of distinct queries, because the price is only paid once per
> new entry, and even then only the duration of holding a shared lock is
> potentially increased (the shared lock alone doesn't prevent existing
> entries from being updated, so are not too costly). Actually, this
> isn't 100% true - there is an extremely remote chance of writing a
> query text to file while an exclusive lock is held - but it really is
> exceedingly rare and unlikely.
>
> * Adds a new, deliberately undocumented parameter to the
> pg_stat_statements() function. This is currently completely useless,
> because of course we don't expose the query hash, which is why I
> haven't created a new extension version (The queryid-exposing patch I
> recently marked "ready for committer" has that - pgss version 1.2 -
> and since this change's inclusion is entirely predicated on getting
> that other patch in, it wouldn't have made sense to do anything more
> than just show what I meant by modifying the existing pgss version,
> 1.1). If we have the queryid to work off of, this becomes a useful
> feature for authors of third party snapshot-consuming tools, that now
> need only lazily fetch query texts for new entries (so when they
> observe a new entry, they know that there next snapshot should ask for
> query texts to fill in what they need). It's a performance feature for
> their use-case, since I feel that supporting those kinds of tools is a
> really useful direction for pg_stat_statements. The user-visible
> behavior of the pg_stat_statements view is unaffected.
>
>
It looks well

I found one small issue.

I had to fix CREATE VIEW statement, due wrong parameter name

-- Register a view on the function for ease of use.
CREATE VIEW pg_stat_statements AS
SELECT * FROM pg_stat_statements(showtext := TRUE);

After this fix it should be ready for commit

Regards

Pavel

>
> --
> Peter Geoghegan
>