SEO

Attached is a patch that enables tracking function calls through
the stats subsystem. Original discussion:
http://archives.postgresql.org/pgsql...7/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


-
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Hi,

On Sun, 23 Mar 2008, Martin Pihlak <martin.pihlak@gmail.com> writes:
> Attached is a patch that enables tracking function calls through
> the stats subsystem. Original discussion:
> http://archives.postgresql.org/pgsql...7/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.
`plgsql' - 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.

-
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

> Hi,
>
> On Sun, 23 Mar 2008, Martin Pihlak <martin.pihlak@gmail.com> writes:
>> Attached is a patch that enables tracking function calls through
>> the stats subsystem. Original discussion:
>> http://archives.postgresql.org/pgsql...7/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.
> `plgsql' - 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


--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Martin Pihlak <martin.pihlak@gmail.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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Magnus Hagander <magnus@hagander.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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Magnus Hagander <magnus@hagander.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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches