Re: review - pg_stat_statements

Hello all

I did check of pg_stat_statements_ext_text.v2.2013_11_16.patch, that
introduce a external storage for query text

I got a compilation warning:

bash-4.1$ make all
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2
-c -o pg_stat_statements.o pg_stat_statements.c
pg_stat_statements.c: In function ‘query_text_retrieve’:
pg_stat_statements.c:1477:3: warning: format ‘%lu’ expects type ‘long
unsigned int’, but argument 4 has type ‘off_t’
pg_stat_statements.c: In function ‘garbage_collect_query_texts’:
pg_stat_statements.c:1796:2: warning: format ‘%ld’ expects type ‘long int’,
but argument 3 has type ‘off_t’
pg_stat_statements.c:1796:2: warning: format ‘%ld’ expects type ‘long int’,
but argument 4 has type ‘off_t’
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-g -fpic -shared -o pg_stat_statements.so pg_stat_statements.o
-L../../src/port -L../../src/common -Wl,--as-needed
-Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags

my environment:

bash-4.1$ uname -a
Linux nemesis 2.6.35.14-106.fc14.i686 #1 SMP Wed Nov 23 13:57:33 UTC 2011
i686 i686 i386 GNU/Linux

bash-4.1$ gcc --version
gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4)

Next, usual queries, and my replies:

* This patch works as was expected and proposed
* The code is well commented and respects PostgreSQL coding standards
* I didn't find any problems when I tested it
* I tried do some basic benchmark, and I didn't see any negative on
performance related to implemented feature
* 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

Peter does some warning about performance in feature proposal
http://www.postgresql.org/message-id/CAM3SWZRYYnfwXi3r3eDAKWBOYtaf1_PwGJxTAyddNSbjAfDzjA@mail.gmail.com.
The risks are not negligible mainly on environments with slow IO and
high varied load. I have no idea, how to eliminate this risks with
possibilities that we have on extensions (maybe divide global stat file to
smaller per database - it should not be solution for some configuration
too). A enough solution (for this release) should better explanation in
documentation. For future releases, we should to think about significant
refactoring - moving to core and using asynchronous stats (and stats per
database) or using specialized background worker

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

summary:
========
* compilation warning
* undocumented new risks of waiting to locks when new query is identified
and processed

Regards

Pavel

On Sun, Nov 24, 2013 at 1:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2013/11/24 Peter Geoghegan <pg(at)heroku(dot)com>

> On Sun, Nov 24, 2013 at 1:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>
>
how is a size of hash table related to exclusive locks in pgss_store? I
don't afraid about performance of pg_stat_statements(). I afraid a
performance of creating new entry and appending to file.

I didn't expected some slowdown - a benchmark was "only" verification
against some hidden cost. Is not difficult to write synthetic benchmark
that will find some differences, but a result of this benchmark will be
useless probably due minimal relation to reality.

> > * 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.
>
>
I don't expect so pg_stat_statements is on application critical path, so I
prefer mostly simple design

> > Peter mentioned a race conditions under high load. It is fixed?
>
> Yes, the version you mentioned had the fix.
>
>
ok

Regards

Pavel

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

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
>

On Sat, Nov 30, 2013 at 11:30 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(dot)stehule(at)gmail(dot)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

Hello

I waited to fix.

Now I marked patch as ready for committer

Regards

Pavel

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

> On Sat, Nov 30, 2013 at 11:30 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>