Thread: review - pg_stat_statements

On Sun, Nov 24, 2013 at 1:18 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I got a compilation warning:

I'll look into it.

> * I tried do some basic benchmark, and I didn't see any negative on
> performance related to implemented feature

You're never going to see any performance impact with something like a
regular pgbench workload. That's because you'll only ever write query
texts out when a shared lock is held. With only extreme exceptions (a
garbage collection), exceptions will never be exercised by what you're
doing, you will only block whole new entries from being added -
existing stat counters are just protected by a shared lock + spinlock.

You might consider rigging pg_stat_statements to create a new hash
value randomly (consisting of a random integer for a queryid "hash")
maybe 1% - 10% of the time. That would simulate the cache being filled
quickly, I guess, where that shared lock will conflict with the
exclusive lock, potentially showing where what I've done can hurt. I
recommend in the interest of fairness not letting it get so far as to
put the cache under continual pressure.

Now, this isn't that important a case, because having a larger hash
table makes exclusive locking/new entries less necessary, and this
work enables people to have larger hash tables. But it does matter to
some degree.

> * 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

Right. Plus the amount of storage used is pretty modest, even compared
to previous shared memory use. Each entry doesn't need to have
track_activity_query_size bytes of storage, only what it needs (though
garbage can accrue, which is a concern).

> Peter does some warning about performance in feature proposal
> http://www.postgresql.org/message-id/CAM3SWZRYYnfwXi3r3eDAKWBOYtaf1_PwGJxTAyddNSbjAfDzjA@mail.gmail.com

I'm mostly talking about the cost of the shared lock for *reading*
here, when pg_stat_statements() is called. If that was happening at
the same time as I/O for reading the query texts from file, that could
be a concern. Not enough of a concern to worry about humans doing
this, I think, but maybe for scripts.

Maybe the trick would be to copy the stats into shared memory, and
only then read texts from disk (with the shared lock released). We'd
have to be concerned about a garbage collection occurring, so we'd
need to re-acquire a shared lock again (plus a spinlock) to check that
didn't happen (which is generally very unlikely). Only when we know it
didn't happen could we display costs to the user, and that might mean
keeping all the query texts in memory, and that might matter in
extreme situations. The reason I haven't done all this already is
because it's far from clear that it's worth the trouble.

> Peter mentioned a race conditions under high load. It is fixed?

Yes, the version you mentioned had the fix.

-- 
Peter Geoghegan

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.


--
Peter Geoghegan

On Sat, Nov 30, 2013 at 11:30 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> After this fix it should be ready for commit

Whoops. I forgot to change that when I changed the name of the
parameter at the last minute. Sorry about that.

-- 
Peter Geoghegan

On Sat, Nov 30, 2013 at 11:30 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> After this fix it should be ready for commit

Version with trivial, single token fix attached. I'm not sure if you
just forgot to mark this "ready for committer" in the commitfest app,
but if not you'll want to do so now.

I defer to the judgement of a committer here -- my hope is that the
queryid-exposing patch will be committed soon, and so it becomes a
simple matter of "rebasing" (a term I use loosely) what I've done here
on top of the then master branch. I felt that I had an up-front
responsibility to not regress the aggressive-snapshot-aggregating use
case, which is why I haven't simply waited for the queryid patch to be
committed so that I could only then submit a new patch (a *separate*
patch that allows query texts to not be returned when unneeded).

Thanks
--
Peter Geoghegan