Re: [PATCHES] stored procedure stats in collector
Magnus Hagander [EMAIL PROTECTED] 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
Re: [PATCHES] stored procedure stats in collector
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] 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
Re: [PATCHES] stored procedure stats in collector
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] stored procedure stats in collector
Magnus Hagander [EMAIL PROTECTED] 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] stored procedure stats in collector
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] stored procedure stats in collector
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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] stored procedure stats in collector
Martin Pihlak [EMAIL PROTECTED] 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] stored procedure stats in collector
Martin Pihlak [EMAIL PROTECTED] 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
Re: [PATCHES] stored procedure stats in collector
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
Re: [PATCHES] stored procedure stats in collector
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
Re: [PATCHES] stored procedure stats in collector
On Mar 23, 2008, at 9:25 PM, Volkan YAZICI wrote: Hi, On Sun, 23 Mar 2008, Martin Pihlak [EMAIL PROTECTED] 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
[PATCHES] stored procedure stats in collector
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 *** ./doc/src/sgml/config.sgml.orig 2008-03-22 20:15:30.0 +0200 --- ./doc/src/sgml/config.sgml 2008-03-23 15:00:27.0 +0200 *** *** 3322,3327 --- 3322,3343 /listitem /varlistentry + varlistentry id=guc-track-functions xreflabel=track_functions + termvarnametrack_functions/varname (typestring/type)/term + indexterm +primaryvarnametrack_functions/ configuration parameter/primary + /indexterm + listitem +para + Enables tracking of time and cpu usage of stored procedures. Specify + literalpl/literal to count only procedural language functions, + literalall/literal to also track SQL and C language functions. The + default is literalnone/literal. Only superusers can change this + setting. +/para + /listitem + /varlistentry + varlistentry id=guc-update-process-title xreflabel=update_process_title termvarnameupdate_process_title/varname (typeboolean/type)/term indexterm *** ./doc/src/sgml/monitoring.sgml.orig 2008-03-23 13:20:25.0 +0200 --- ./doc/src/sgml/monitoring.sgml 2008-03-23 14:55:28.0 +0200 *** *** 151,156 --- 151,161 /para para +The parameter xref linkend=guc-track-functions enables tracking of +stored procedure usage. + /para + + para Normally these parameters are set in filenamepostgresql.conf/ so that they apply to all server processes, but it is possible to turn them on or off in individual sessions using the xref *** *** 370,375 --- 375,390 entrySame as structnamepg_statio_all_sequences/, except that only user sequences are shown./entry /row + + row + entrystructnamepg_stat_user_functions//entry + entryFor all called functions, function OID, schema, name, number + of calls, total time, total cpu, self time and cpu. Self time is the + amount of time spent in the function itself, total time includes the + time spent in child functions. Time values in milliseconds. + /entry + /row + /tbody /tgroup /table *** *** 655,660 --- 670,733 /row row + entryliteralfunctionpg_stat_get_function_calls/function(typeoid/type)/literal/entry + entrytypebigint/type/entry + entry +Number of times the function has been called. + /entry + /row + + row + entryliteralfunctionpg_stat_get_function_rtime/function(typeoid/type)/literal/entry + entrytypebigint/type/entry + entry + Total wall clock time spent in the function. In microseconds, includes + the time spent by child functions. + /entry + /row + + row + entryliteralfunctionpg_stat_get_function_stime/function(typeoid/type)/literal/entry + entrytypebigint/type/entry + entry + Total system mode CPU time spent in the function. + /entry + /row + + row + entryliteralfunctionpg_stat_get_function_utime/function(typeoid/type)/literal/entry + entrytypebigint/type/entry + entry + Total user mode CPU time spent in the function. + /entry + /row + + row + entryliteralfunctionpg_stat_get_function_self_rtime/function(typeoid/type)/literal/entry + entrytypebigint/type/entry + entry + Time spent by only this function. Time spent in child functions + is excluded. + /entry + /row + + row + entryliteralfunctionpg_stat_get_function_self_stime/function(typeoid/type)/literal/entry + entrytypebigint/type/entry + entry + System mode CPU spent by this function. + /entry + /row + + row + entryliteralfunctionpg_stat_get_function_self_utime/function(typeoid/type)/literal/entry + entrytypebigint/type/entry + entry + User mode CPU spent by this function. + /entry + /row + + row entryliteralfunctionpg_stat_get_backend_idset/function()/literal/entry entrytypesetof integer/type/entry entry *** ./src/backend/catalog/system_views.sql.orig 2008-03-20 10:13:23.0 +0200 --- ./src/backend/catalog/system_views.sql 2008-03-22 16:27:38.0 +0200 *** *** 376,381 --- 376,395 pg_stat_get_db_tuples_deleted(D.oid) AS tup_deleted FROM pg_database D; + CREATE OR REPLACE VIEW pg_stat_user_functions AS +
Re: [PATCHES] stored procedure stats in collector
Hi, On Sun, 23 Mar 2008, Martin Pihlak [EMAIL PROTECTED] 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. - Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
