stored procedure stats in collector

Hi,

> Good. I'll bring the patch up to date with HEAD.

Did you post an updated patch to HEAD?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Howdy,

> Did you post an updated patch to HEAD?
>

No, but I guess its just about time. Hopefully I'll have
something to show next week.

regards,
Martin

Attached is a patch that enables tracking function calls through
the stats subsystem. Original discussion:
http://archives.postgresql.org/pgsql-hackers/2007-07/msg00377.php

Introduces new guc variable - track_functions. Possible values are:
none - no collection, default
pl - tracks procedural language functions
all - tracks procedural, SQL and C (not internal) functions

The results are visible from pg_stat_get_function_* functions and
pg_stat_user_functions view.

regards,
Martin

Hi,

On Sun, 23 Mar 2008, Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> writes:
> Attached is a patch that enables tracking function calls through
> the stats subsystem. Original discussion:
> http://archives.postgresql.org/pgsql-hackers/2007-07/msg00377.php
>
> Introduces new guc variable - track_functions. Possible values are:
> none - no collection, default
> pl - tracks procedural language functions
> all - tracks procedural, SQL and C (not internal) functions

I might have missed the discussion, but I'd love to see a more flexible
interface for configuration parameters. For instance, it'd be great if
we can specify which procedural languages to track in the `pl' GUC.
Moreover, if it'd be possible to specify which specific functions we
want to try, then that would be awesome as well.

For instance, possible configuration combinations for track_functions
can be:

`pl:*' - Tracks procedural, SQL and C (not internal)
functions in the `public' schema.
`pl:pgsql' - Tracks only PL/pgSQL functions.
`pl:scheme' - Tracks only PL/scheme functions.
`foo(int, int)' - Tracks related `foo' function in the public
schema.
`stock.foo(int, int)' - Tracks related `foo' function in the `stock'
schema.
`pl:stock.*' - Tracks procedural, SQL and C (not internal)
functions in the `stock' schema.

Syntax can obviously be much more consistent. (These are just what I
come up with for the very moment.) And I'm aware of the overhead and
complexity(?) this sort of scheme will bring, but I really want to use
such a useful feature with mentioned flexibilities.

Regards.

On Mar 23, 2008, at 9:25 PM, Volkan YAZICI wrote:

> Hi,
>
> On Sun, 23 Mar 2008, Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> writes:
>> Attached is a patch that enables tracking function calls through
>> the stats subsystem. Original discussion:
>> http://archives.postgresql.org/pgsql-hackers/2007-07/msg00377.php
>>
>> Introduces new guc variable - track_functions. Possible values are:
>> none - no collection, default
>> pl - tracks procedural language functions
>> all - tracks procedural, SQL and C (not internal) functions
>
> I might have missed the discussion, but I'd love to see a more
> flexible
> interface for configuration parameters. For instance, it'd be great if
> we can specify which procedural languages to track in the `pl' GUC.
> Moreover, if it'd be possible to specify which specific functions we
> want to try, then that would be awesome as well.
>
> For instance, possible configuration combinations for track_functions
> can be:
>
> `pl:*' - Tracks procedural, SQL and C (not internal)
> functions in the `public' schema.
> `pl:pgsql' - Tracks only PL/pgSQL functions.
> `pl:scheme' - Tracks only PL/scheme functions.
> `foo(int, int)' - Tracks related `foo' function in the public
> schema.
> `stock.foo(int, int)' - Tracks related `foo' function in the `stock'
> schema.
> `pl:stock.*' - Tracks procedural, SQL and C (not internal)
> functions in the `stock' schema.
>
> Syntax can obviously be much more consistent. (These are just what I
> come up with for the very moment.) And I'm aware of the overhead and
> complexity(?) this sort of scheme will bring, but I really want to use
> such a useful feature with mentioned flexibilities.
>
>

this patch is quite cool already.
it would be even cooler if we could define on a per-function basis.
eg. CREATE FUNCTION ... TRACK | NOTRACK
in addition to that we could define a GUC defining whether TRACK or
NOTRACK is used as default.
in many cases you are only interested in a special set of functions
anyway.
as every operator is basically a procedure in postgres, i am not
quite happy about the per-language approach.

best regards,

hans

--
Cybertec Schönig & Schönig GmbH
PostgreSQL Solutions and Support
Gröhrmühlgasse 26, 2700 Wiener Neustadt
Tel: +43/1/205 10 35 / 340
www.postgresql-support.de, www.postgresql-support.com

Howdy,

> Moreover, if it'd be possible to specify which specific functions we
> want to try, then that would be awesome as well.
>
> For instance, possible configuration combinations for track_functions
> can be:
>
> `pl:*' - Tracks procedural, SQL and C (not internal)

It is probably more efficient to track all functions and then use filters
on the stats view. That way the filters can be arbitrarily complex and
are out of the way of critical code path.

Selective filtering could also be implemented using per-function guc
variables. For example, set "track_functions = none" system wide and
then for specific functions:

alter function foo() set track_functions = "all";

Now I just realized that the current patch doesn't handle this quite
correctly. Modified patch attached.

Hans, this should be equivalent to the TRACK / NOTRACK you proposed? If so,
then we can do without the grammar change and just use the per-function guc.

Regards,
Martin

Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> writes:
> Now I just realized that the current patch doesn't handle this quite
> correctly. Modified patch attached.

I'm starting to look through this now, and I notice that the various
code paths in execQual.c are not too consistent about whether it counts
as a call if a strict function is skipped over because of NULL
arguments. I'm inclined to make it uniformly say that that's not a call
and is not included in the stats --- any objections?

regards, tom lane

I wrote:
> I'm starting to look through this now,

I found another fairly big problem, which is that this stuff isn't even
going to begin to compile on Windows.

What I think we should do about that is forget tracking getrusage()'s
user/system/real time and just track elapsed time. We have the
technology to get that in a portable fashion (cf the well-proven
instrument.c code). Such a decision would also alleviate two of the
biggest negatives of this patch, which are the runtime overhead and
the extent to which it's going to bloat the pgstats file.

Thoughts?

regards, tom lane

Tom Lane wrote:
> I wrote:
>> I'm starting to look through this now,
>
> I found another fairly big problem, which is that this stuff isn't even
> going to begin to compile on Windows.

Where exactly is taht problem? In getrusage()? We have a getrusage() in
src/port that works fine on Windows, no?

> What I think we should do about that is forget tracking getrusage()'s
> user/system/real time and just track elapsed time. We have the
> technology to get that in a portable fashion (cf the well-proven
> instrument.c code). Such a decision would also alleviate two of the
> biggest negatives of this patch, which are the runtime overhead and
> the extent to which it's going to bloat the pgstats file.

Those argument makes a lot of sense, though. A bloated pgstats file can
be a real problem. And I don't see that information as being all that
helpful anyway.

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> I found another fairly big problem, which is that this stuff isn't even
>> going to begin to compile on Windows.

> Where exactly is taht problem? In getrusage()? We have a getrusage() in
> src/port that works fine on Windows, no?

Huh ... I'd forgotten about that ... although it seems to work only for
rather small values of "work", since the WIN32 code path isn't paying
attention to the "who" argument.

>> What I think we should do about that is forget tracking getrusage()'s
>> user/system/real time and just track elapsed time.

> Those argument makes a lot of sense, though.

Yeah, the real bottom line here is whether we are buying anything that's
worth another two kernel calls per function. Given all the buffering
and offloading of I/O to bgwriter that we try to do, it's hard to argue
that stime/utime measurements for the current backend really mean a lot.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Tom Lane wrote:
>>> I found another fairly big problem, which is that this stuff isn't even
>>> going to begin to compile on Windows.
>
>> Where exactly is taht problem? In getrusage()? We have a getrusage() in
>> src/port that works fine on Windows, no?
>
> Huh ... I'd forgotten about that ... although it seems to work only for
> rather small values of "work", since the WIN32 code path isn't paying
> attention to the "who" argument.

True, but it works for this case :-)

>>> What I think we should do about that is forget tracking getrusage()'s
>>> user/system/real time and just track elapsed time.
>
>> Those argument makes a lot of sense, though.
>
> Yeah, the real bottom line here is whether we are buying anything that's
> worth another two kernel calls per function. Given all the buffering
> and offloading of I/O to bgwriter that we try to do, it's hard to argue
> that stime/utime measurements for the current backend really mean a lot.

Agreed.

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> Huh ... I'd forgotten about that ... although it seems to work only for
>> rather small values of "work", since the WIN32 code path isn't paying
>> attention to the "who" argument.

> True, but it works for this case :-)

Shouldn't we at least make it fail with EINVAL if "who" doesn't match
whichever semantics this code is able to implement?

[ not relevant to the immediate patch, I suppose, but it might save some
tears later. ]

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Tom Lane wrote:
>>> Huh ... I'd forgotten about that ... although it seems to work only for
>>> rather small values of "work", since the WIN32 code path isn't paying
>>> attention to the "who" argument.
>
>> True, but it works for this case :-)
>
> Shouldn't we at least make it fail with EINVAL if "who" doesn't match
> whichever semantics this code is able to implement?
>
> [ not relevant to the immediate patch, I suppose, but it might save some
> tears later. ]

Yeah, we only ever call it asking for our own process, but I guess we
might at some point in the future change that, so it can't hurt.. Want
me to do it, or will you?

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> Shouldn't we at least make it fail with EINVAL if "who" doesn't match
>> whichever semantics this code is able to implement?

> Yeah, we only ever call it asking for our own process, but I guess we
> might at some point in the future change that, so it can't hurt.. Want
> me to do it, or will you?

Please do, I'm going to bed ...

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Tom Lane wrote:
>>> Shouldn't we at least make it fail with EINVAL if "who" doesn't match
>>> whichever semantics this code is able to implement?
>
>> Yeah, we only ever call it asking for our own process, but I guess we
>> might at some point in the future change that, so it can't hurt.. Want
>> me to do it, or will you?
>
> Please do, I'm going to bed ...

Done.

//Magnus

> What I think we should do about that is forget tracking getrusage()'s
> user/system/real time and just track elapsed time. We have the
> technology to get that in a portable fashion (cf the well-proven
> instrument.c code). Such a decision would also alleviate two of the
> biggest negatives of this patch, which are the runtime overhead and
> the extent to which it's going to bloat the pgstats file.
>

I find the utime/stime quite useful, compared with the actual time it
enables us to distinguish waiters (remote calls, sleeps, etc) from the
actual CPU hogs. The difference is also very visible for IO bound
functions. At least in our case it is a very useful tool for diagnosing
performance issues and the overhead is not really visible.

Perhaps the track_functions should just be set to none, or enabled selectively
(session, function guc, user guc) for the environments where getrusage()
is particularly expensive? Maybe a note in the docs that tracking is
potentially expensive, and should be used carefully in production env.

Regards,
Martin

[ redirecting to pghackers for wider discussion ]

Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> writes:
>> What I think we should do about that is forget tracking getrusage()'s
>> user/system/real time and just track elapsed time.

> I find the utime/stime quite useful, compared with the actual time it
> enables us to distinguish waiters (remote calls, sleeps, etc) from the
> actual CPU hogs.

Well, what it is going to cost us to have that is double the space
in the pgstats file (64 bytes per function instead of 32) --- and that
isn't a choice we can flip with a GUC. This is a hot button for a
lot of people because we know that bloat in the pgstats file translates
directly to continual I/O overhead. (Perhaps that'll be fixed by the
time this patch hits production, but I'm not holding my breath.)

The runtime overhead is pretty daunting also. It's not just twice as
many kernel calls, it's which ones you are making. On a lot of modern
machines gettimeofday() is optimized to not enter the kernel at all,
while getrusage() will hardly be. [ click click, test test ] On my
x86_64 Fedora 8 machine, it appears that gettimeofday() requires about
60 nsec per call, whereas getrusage(RUSAGE_SELF) requires 788 nsec.

One other point here is that accuracy of the results is questionable.
On Windows we will certainly find that gettimeofday is useless and
we need to use QueryPerformanceCounter instead (see the code in
instrument.h/.c). I wonder what the accuracy of GetProcessTimes
is and whether it will even deliver answers consistent with
QueryPerformanceCounter. On Unix-ish machines the corresponding
worry is that getrusage results might be tracked only to the clock
tick and not any finer grain.

Double the pgstats storage and a dozen times as much runtime overhead
in exchange for questionable numbers is a pretty hard sell. I remain
of the opinion that we should just track elapsed time.

regards, tom lane

Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> writes:
> Now I just realized that the current patch doesn't handle this quite
> correctly. Modified patch attached.

Applied with revisions as per discussion.

regards, tom lane