Thread: implement query_start for pg_stat_activity
This patch implements the TODO item
- Add start time to pg_stat_activity
The implementation is straight-forward, except for two issues:
(1) The natural name for the backend function is
pg_stat_get_backend_activity_start(), which at 34 exceeds the
NAMEDATALEN limitation in previous releases of PostgreSQL. While that
limit has been raised, it seems a shame to me to not allow users to
manual lower it again -- so I renamed the function to
pg_stat_get_backend_qry_start(), and renamed the existing function that
fetches the query string from pg_stat_get_backend_activity() to
pg_stat_get_backend_qry() for consistency. If someone thinks that's the
wrong decision, let me know.
(2) I wasn't sure how to convert a struct timeval into a PostgreSQL
timestamp type, so I hacked something together involving ctime() and
timestamp_in(), but it seems clearly wrong. Can someone suggest how to
do this properly?
Cheers,
Neil
PS: Shouldn't the pg_stat_get_backend_xxx() functions be marked
"volatile"? (They're currently marked "stable")
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Neil Conway <neilc@samurai.com> writes:
> (1) The natural name for the backend function is
> pg_stat_get_backend_activity_start(), which at 34 exceeds the
> NAMEDATALEN limitation in previous releases of PostgreSQL. While that
> limit has been raised, it seems a shame to me to not allow users to
> manual lower it again -- so I renamed the function to
> pg_stat_get_backend_qry_start(), and renamed the existing function that
> fetches the query string from pg_stat_get_backend_activity() to
> pg_stat_get_backend_qry() for consistency. If someone thinks that's the
> wrong decision, let me know.
NAMEDATALEN=32 will have been history for two releases when this gets
out. I don't agree with artificially constricting a function name to
conform to an obsolete restriction --- and for *sure* I don't agree with
renaming an existing function to make it line up with a new, artificial
name ...
> (2) I wasn't sure how to convert a struct timeval into a PostgreSQL
> timestamp type, so I hacked something together involving ctime() and
> timestamp_in(), but it seems clearly wrong.
GetCurrentAbsoluteTimeUsec() followed by now() seem to do it at the
moment ... but they do look pretty historically encumbered themselves.
Care to offer a proposal for simplifying this code?
regards, tom lane
On Wed, 2003-02-19 at 01:04, Tom Lane wrote: > NAMEDATALEN=32 will have been history for two releases when this gets > out. I don't agree with artificially constricting a function name to > conform to an obsolete restriction --- and for *sure* I don't agree with > renaming an existing function to make it line up with a new, artificial > name ... Fair enough. > > (2) I wasn't sure how to convert a struct timeval into a PostgreSQL > > timestamp type, so I hacked something together involving ctime() and > > timestamp_in(), but it seems clearly wrong. > > GetCurrentAbsoluteTimeUsec() followed by now() seem to do it at the > moment ... Can you elaborate a bit? Since now() returns the time the current txn started, that doesn't seem to be what I'd need -- and isn't GetCurrentAbsoluteTimeUsec() essentially just an (ugly) wrapper over gettimeofday()? I think a similar function to timeofday() that returns a timestamp would probably be ideal... (which reminds me, we should probably think about how to replace timeofday() with something that doesn't return a string). > Care to offer a proposal for simplifying this code? To be honest, I wouldn't know where to start -- I don't really know much about the intricacies of datetime manipulation... Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Neil Conway <neilc@samurai.com> writes:
>> Care to offer a proposal for simplifying this code?
> To be honest, I wouldn't know where to start -- I don't really know much
> about the intricacies of datetime manipulation...
Me either. Tom Lockhart was our datetime-meister, but he's gafiated.
We need someone new to take charge of that code... there's definitely
work remaining to be done there...
regards, tom lane
On Wed, 2003-02-19 at 00:27, Neil Conway wrote:
> This patch implements the TODO item
>
> - Add start time to pg_stat_activity
I've attached a revised version of the patch with the following changes:
- use longer function names, as suggested by Tom
- (unrelated) replace some instances where snprintf() was called with a
constant buffer size with sizeof
- documentation improvements
- update regression tests
- change the method of finding the current date & time -- it's still an
ugly hack, although arguably less so. Now we do:
AbsoluteTime sec;
int usec;
sec = GetCurrentAbsoluteTimeUsec(&usec);
/* when displaying the time */
Timestamp result;
#ifdef HAVE_INT64_TIMESTAMP
result = (((sec - ((date2j(2000, 1, 1) - date2j(1970, 1, 1)) * 86400))
* INT64CONST(1000000)) + usec);
#else
result = (sec + (usec * 1.0e-6) - ((date2j(2000, 1, 1) -
date2j(1970, 1, 1)) * 86400));
#endif
which is the same method now() uses internally.
Cheers,
Neil
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Neil Conway <neilc@samurai.com> writes:
> - change the method of finding the current date & time -- it's still an
> ugly hack, although arguably less so. Now we do:
Can't we push that into some subroutine exported by one of the
datetime-related utils/adt files? It seems a bad idea for pgstatfuncs.c
to get so cozy with the internal representation of some other module's
datatype.
regards, tom lane
On Wed, 2003-02-19 at 01:45, Neil Conway wrote: > On Wed, 2003-02-19 at 01:04, Tom Lane wrote: > > NAMEDATALEN=32 will have been history for two releases when this gets > > out. I don't agree with artificially constricting a function name to > > conform to an obsolete restriction --- and for *sure* I don't agree with > > renaming an existing function to make it line up with a new, artificial > > name ... > > Fair enough. > > > > (2) I wasn't sure how to convert a struct timeval into a PostgreSQL > > > timestamp type, so I hacked something together involving ctime() and > > > timestamp_in(), but it seems clearly wrong. > > > > GetCurrentAbsoluteTimeUsec() followed by now() seem to do it at the > > moment ... > > Can you elaborate a bit? Since now() returns the time the current txn > started, that doesn't seem to be what I'd need -- and isn't > GetCurrentAbsoluteTimeUsec() essentially just an (ugly) wrapper over > gettimeofday()? > > I think a similar function to timeofday() that returns a timestamp would > probably be ideal... (which reminds me, we should probably think about > how to replace timeofday() with something that doesn't return a string). > > > Care to offer a proposal for simplifying this code? > > To be honest, I wouldn't know where to start -- I don't really know much > about the intricacies of datetime manipulation... I did a quick patch a while ago that would simply use Abstime -- setting query_start and connection_start (when the backend was fired up). Another thing it did was to change pg_stat_activity into a single SRF removing a good chunk of the other junk. Anyway, after using it for a while on a test system. I removed the Usec precision as short queries disappear quickly enough for this not to be interesting nor was there much point in sending the time from the backend to the stat collector, as even a 5 second delay in receiving the UDP packet didn't make any difference in the information I was looking for -- enough information to make an snmp event about a rogue query or connection. Anyway, attached is what I've been using for curiosity sakes. Updated for -TIP and it compiles but I didn't test to see if it still works. -- Rod Taylor <rbt@rbt.ca> PGP Key: http://www.rbt.ca/rbtpub.asc
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> On Wed, 2003-02-19 at 00:27, Neil Conway wrote:
> > This patch implements the TODO item
> >
> > - Add start time to pg_stat_activity
>
> I've attached a revised version of the patch with the following changes:
>
> - use longer function names, as suggested by Tom
>
> - (unrelated) replace some instances where snprintf() was called with a
> constant buffer size with sizeof
>
> - documentation improvements
>
> - update regression tests
>
> - change the method of finding the current date & time -- it's still an
> ugly hack, although arguably less so. Now we do:
>
> AbsoluteTime sec;
> int usec;
>
> sec = GetCurrentAbsoluteTimeUsec(&usec);
>
> /* when displaying the time */
> Timestamp result;
>
> #ifdef HAVE_INT64_TIMESTAMP
> result = (((sec - ((date2j(2000, 1, 1) - date2j(1970, 1, 1)) * 86400))
> * INT64CONST(1000000)) + usec);
> #else
> result = (sec + (usec * 1.0e-6) - ((date2j(2000, 1, 1) -
> date2j(1970, 1, 1)) * 86400));
> #endif
>
> which is the same method now() uses internally.
>
> Cheers,
>
> Neil
> --
> Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
>
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied. Thanks. Initdb forced. New output: test=> select pg_stat_activity.*; datid | datname | procpid | usesysid | usename | current_query | query_start -------+---------+---------+----------+----------+---------------+---------------------------- 17054 | test | 1147 | 1 | postgres | <IDLE> | 2003-03-20 03:01:51.432215 (1 row) --------------------------------------------------------------------------- Neil Conway wrote: > On Wed, 2003-02-19 at 00:27, Neil Conway wrote: > > This patch implements the TODO item > > > > - Add start time to pg_stat_activity > > I've attached a revised version of the patch with the following changes: > > - use longer function names, as suggested by Tom > > - (unrelated) replace some instances where snprintf() was called with a > constant buffer size with sizeof > > - documentation improvements > > - update regression tests > > - change the method of finding the current date & time -- it's still an > ugly hack, although arguably less so. Now we do: > > AbsoluteTime sec; > int usec; > > sec = GetCurrentAbsoluteTimeUsec(&usec); > > /* when displaying the time */ > Timestamp result; > > #ifdef HAVE_INT64_TIMESTAMP > result = (((sec - ((date2j(2000, 1, 1) - date2j(1970, 1, 1)) * 86400)) > * INT64CONST(1000000)) + usec); > #else > result = (sec + (usec * 1.0e-6) - ((date2j(2000, 1, 1) - > date2j(1970, 1, 1)) * 86400)); > #endif > > which is the same method now() uses internally. > > Cheers, > > Neil > -- > Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC > > [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Also, TODO item now marked as done! --------------------------------------------------------------------------- Neil Conway wrote: > On Wed, 2003-02-19 at 00:27, Neil Conway wrote: > > This patch implements the TODO item > > > > - Add start time to pg_stat_activity > > I've attached a revised version of the patch with the following changes: > > - use longer function names, as suggested by Tom > > - (unrelated) replace some instances where snprintf() was called with a > constant buffer size with sizeof > > - documentation improvements > > - update regression tests > > - change the method of finding the current date & time -- it's still an > ugly hack, although arguably less so. Now we do: > > AbsoluteTime sec; > int usec; > > sec = GetCurrentAbsoluteTimeUsec(&usec); > > /* when displaying the time */ > Timestamp result; > > #ifdef HAVE_INT64_TIMESTAMP > result = (((sec - ((date2j(2000, 1, 1) - date2j(1970, 1, 1)) * 86400)) > * INT64CONST(1000000)) + usec); > #else > result = (sec + (usec * 1.0e-6) - ((date2j(2000, 1, 1) - > date2j(1970, 1, 1)) * 86400)); > #endif > > which is the same method now() uses internally. > > Cheers, > > Neil > -- > Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC > > [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073