Re: [HACKERS] IDLE in transaction introspection
On Thu, Jan 19, 2012 at 15:42, Fujii Masao wrote: > On Thu, Jan 19, 2012 at 10:31 PM, Magnus Hagander wrote: >> Applied with fairly extensive modifications. I moved things around, >> switched to using enum instead of int+#define and a few things like >> that. Also changed most of the markup in the docs - I may well have >> broken some previously good language that, so if I did and someone >> spots it, please mention it :-) > > The attached patch seems to need to be committed. Thanks, applied. > BTW, the following change in the patch is not required, but ISTM that > it's better to change that way. > > -SELECT pg_stat_get_backend_pid(s.backendid) AS procpid, > - pg_stat_get_backend_activity(s.backendid) AS current_query > +SELECT pg_stat_get_backend_pid(s.backendid) AS pid, > + pg_stat_get_backend_activity(s.backendid) AS query Agreed. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Thu, Jan 19, 2012 at 10:31 PM, Magnus Hagander wrote: > Applied with fairly extensive modifications. I moved things around, > switched to using enum instead of int+#define and a few things like > that. Also changed most of the markup in the docs - I may well have > broken some previously good language that, so if I did and someone > spots it, please mention it :-) The attached patch seems to need to be committed. BTW, the following change in the patch is not required, but ISTM that it's better to change that way. -SELECT pg_stat_get_backend_pid(s.backendid) AS procpid, - pg_stat_get_backend_activity(s.backendid) AS current_query +SELECT pg_stat_get_backend_pid(s.backendid) AS pid, + pg_stat_get_backend_activity(s.backendid) AS query Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center noname Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Wed, Jan 18, 2012 at 16:35, Scott Mead wrote: > > On Wed, Jan 18, 2012 at 8:27 AM, Magnus Hagander > wrote: >> >> On Tue, Jan 17, 2012 at 01:43, Scott Mead wrote: >> > >> > >> > On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead wrote: >> >> >> >> >> >> On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith >> >> wrote: >> >>> >> >>> On 01/12/2012 11:57 AM, Scott Mead wrote: >> >> Pretty delayed, but please find the attached patch that addresses all >> the issues discussed. >> >>> >> >>> >> >>> The docs on this v4 look like they suffered a patch order problem >> >>> here. >> >>> In the v3, you added a whole table describing the pg_stat_activity >> >>> documentation in more detail than before. v4 actually tries to remove >> >>> those >> >>> new docs, a change which won't even apply as they don't exist >> >>> upstream. >> >>> >> >>> My guess is you committed v3 to somewhere, applied the code changes >> >>> for >> >>> v4, but not the documentation ones. It's easy to do that and end up >> >>> with a >> >>> patch that removes a bunch of docs the previous patch added. I have >> >>> to be >> >>> careful to always do something like "git diff origin/master" to avoid >> >>> this >> >>> class of problem, until I got into that habit I did this sort of thing >> >>> regularly. >> >>> >> >> gak >> > >> > >> > I did a 'backwards' diff last time. This time around, I diff-ed off of >> > a >> > fresh pull of 'master' (and I did the diff in the right direction. >> > >> > Also includes whitespace cleanup and the pg_stat_replication (procpid >> > ==> >> > pid) regression fix. >> > >> >> I'm reviewing this again, and have changed a few things around. I came >> up with a question, too :-) >> >> Right now, if you turn off track activities, we put "> not enabled>" in the query text. Shouldn't this also be a state, such >> as "disabled"? It seems more consistent to me... > > > Sure, I was going the route of 'client_addr', i.e. leaving it null when not > in use just to keep screen clutter down, but I'm not married to it. Applied with fairly extensive modifications. I moved things around, switched to using enum instead of int+#define and a few things like that. Also changed most of the markup in the docs - I may well have broken some previously good language that, so if I did and someone spots it, please mention it :-) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Wed, Jan 18, 2012 at 8:27 AM, Magnus Hagander wrote: > On Tue, Jan 17, 2012 at 01:43, Scott Mead wrote: > > > > > > On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead wrote: > >> > >> > >> On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith > wrote: > >>> > >>> On 01/12/2012 11:57 AM, Scott Mead wrote: > > Pretty delayed, but please find the attached patch that addresses all > the issues discussed. > >>> > >>> > >>> The docs on this v4 look like they suffered a patch order problem here. > >>> In the v3, you added a whole table describing the pg_stat_activity > >>> documentation in more detail than before. v4 actually tries to remove > those > >>> new docs, a change which won't even apply as they don't exist upstream. > >>> > >>> My guess is you committed v3 to somewhere, applied the code changes for > >>> v4, but not the documentation ones. It's easy to do that and end up > with a > >>> patch that removes a bunch of docs the previous patch added. I have > to be > >>> careful to always do something like "git diff origin/master" to avoid > this > >>> class of problem, until I got into that habit I did this sort of thing > >>> regularly. > >>> > >> gak > > > > > > I did a 'backwards' diff last time. This time around, I diff-ed off of a > > fresh pull of 'master' (and I did the diff in the right direction. > > > > Also includes whitespace cleanup and the pg_stat_replication (procpid ==> > > pid) regression fix. > > > > I'm reviewing this again, and have changed a few things around. I came > up with a question, too :-) > > Right now, if you turn off track activities, we put " not enabled>" in the query text. Shouldn't this also be a state, such > as "disabled"? It seems more consistent to me... > Sure, I was going the route of 'client_addr', i.e. leaving it null when not in use just to keep screen clutter down, but I'm not married to it. --Scott OpenSCG, http://www.openscg.com > > -- > Magnus Hagander > Me: http://www.hagander.net/ > Work: http://www.redpill-linpro.com/ >
Re: [HACKERS] IDLE in transaction introspection
On Tue, Jan 17, 2012 at 01:43, Scott Mead wrote: > > > On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead wrote: >> >> >> On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith wrote: >>> >>> On 01/12/2012 11:57 AM, Scott Mead wrote: Pretty delayed, but please find the attached patch that addresses all the issues discussed. >>> >>> >>> The docs on this v4 look like they suffered a patch order problem here. >>> In the v3, you added a whole table describing the pg_stat_activity >>> documentation in more detail than before. v4 actually tries to remove those >>> new docs, a change which won't even apply as they don't exist upstream. >>> >>> My guess is you committed v3 to somewhere, applied the code changes for >>> v4, but not the documentation ones. It's easy to do that and end up with a >>> patch that removes a bunch of docs the previous patch added. I have to be >>> careful to always do something like "git diff origin/master" to avoid this >>> class of problem, until I got into that habit I did this sort of thing >>> regularly. >>> >> gak > > > I did a 'backwards' diff last time. This time around, I diff-ed off of a > fresh pull of 'master' (and I did the diff in the right direction. > > Also includes whitespace cleanup and the pg_stat_replication (procpid ==> > pid) regression fix. > I'm reviewing this again, and have changed a few things around. I came up with a question, too :-) Right now, if you turn off track activities, we put "" in the query text. Shouldn't this also be a state, such as "disabled"? It seems more consistent to me... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith wrote: > On 01/12/2012 11:57 AM, Scott Mead wrote: > >> Pretty delayed, but please find the attached patch that addresses all the >> issues discussed. >> > > The docs on this v4 look like they suffered a patch order problem here. > In the v3, you added a whole table describing the pg_stat_activity > documentation in more detail than before. v4 actually tries to remove > those new docs, a change which won't even apply as they don't exist > upstream. > > My guess is you committed v3 to somewhere, applied the code changes for > v4, but not the documentation ones. It's easy to do that and end up with a > patch that removes a bunch of docs the previous patch added. I have to be > careful to always do something like "git diff origin/master" to avoid this > class of problem, until I got into that habit I did this sort of thing > regularly. > > gak Okay, I'll fix that and the rules.out regression that I missed --Scott OpenSCG, http://www.openscg.com > > -- > Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD > PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com > >
Re: [HACKERS] IDLE in transaction introspection
On 01/12/2012 11:57 AM, Scott Mead wrote: Pretty delayed, but please find the attached patch that addresses all the issues discussed. The docs on this v4 look like they suffered a patch order problem here. In the v3, you added a whole table describing the pg_stat_activity documentation in more detail than before. v4 actually tries to remove those new docs, a change which won't even apply as they don't exist upstream. My guess is you committed v3 to somewhere, applied the code changes for v4, but not the documentation ones. It's easy to do that and end up with a patch that removes a bunch of docs the previous patch added. I have to be careful to always do something like "git diff origin/master" to avoid this class of problem, until I got into that habit I did this sort of thing regularly. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Thu, Dec 15, 2011 at 12:10 PM, Magnus Hagander wrote: > On Thu, Dec 15, 2011 at 13:18, Greg Smith wrote: > > This patch seems closing in on being done, but it's surely going to take > at > > least one more round of review to make sure all the naming and > documentation > > is up right. I can work on that again whenever Scott gets another > version > > necessary, and Magnus is already poking around with an eye toward > possibly > > committing the result. I think we can make this one "returned with > > feedback" for this CF, but encourage Scott to resubmit as early as > feasible > > before the next one. With some good luck, we might even close this out > > before that one even starts. > > My hope was to get a quick update from Scott and then apply it. But > feel free to set it to returned, but Scott - don't delay until the > next CF, just post it when you're ready and I'll pick it up in between > the two CFs when I find a moment. > Thanks guys, I should have something by this weekend. Sorry for the delay. --Scott > > > -- > Magnus Hagander > Me: http://www.hagander.net/ > Work: http://www.redpill-linpro.com/ >
Re: [HACKERS] IDLE in transaction introspection
On Thu, Dec 15, 2011 at 13:18, Greg Smith wrote: > This patch seems closing in on being done, but it's surely going to take at > least one more round of review to make sure all the naming and documentation > is up right. I can work on that again whenever Scott gets another version > necessary, and Magnus is already poking around with an eye toward possibly > committing the result. I think we can make this one "returned with > feedback" for this CF, but encourage Scott to resubmit as early as feasible > before the next one. With some good luck, we might even close this out > before that one even starts. My hope was to get a quick update from Scott and then apply it. But feel free to set it to returned, but Scott - don't delay until the next CF, just post it when you're ready and I'll pick it up in between the two CFs when I find a moment. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
This patch seems closing in on being done, but it's surely going to take at least one more round of review to make sure all the naming and documentation is up right. I can work on that again whenever Scott gets another version necessary, and Magnus is already poking around with an eye toward possibly committing the result. I think we can make this one "returned with feedback" for this CF, but encourage Scott to resubmit as early as feasible before the next one. With some good luck, we might even close this out before that one even starts. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Wed, Dec 7, 2011 at 17:45, Scott Mead wrote: > > On Tue, Dec 6, 2011 at 6:38 AM, Magnus Hagander wrote: >> >> On Sat, Nov 19, 2011 at 02:55, Scott Mead wrote: >> > >> > On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead wrote: >> >> >> >> On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead wrote: >> >>> >> >>> On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat wrote: >> >> On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith >> wrote: >> > On 11/15/2011 09:44 AM, Scott Mead wrote: >> >> >> >> Fell off the map last week, here's v2 that: >> >> * RUNNING => active >> >> * all states from CAPS to lower case >> >> >> > >> > This looks like what I was hoping someone would add here now. >> > Patch >> > looks >> > good, only issue I noticed was a spaces instead of a tab goof where >> > you set >> > beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c >> > >> > Missing pieces: >> > >> > -There is one regression test that uses pg_stat_activity that is >> > broken now. >> > -The documentation should list the new field and all of the states >> > it >> > might >> > include. That's a serious doc update from the minimal info >> > available >> > right >> > now. >> > >> > >> > V3 Attached: >> > >> > * Updates Documentation >> > -- Adds a separate table to cover each column of pg_stat_activity >> >> I like that a lot - we should've done taht years ago :-) >> >> For consistency, either both datname and usename should refer to their >> respective oid, or none of them. >> >> >> application_name should probably have a link to the libpq >> documentation for said parameter. >> >> "field is not set" should probably be "field is NULL" >> > > Thanks for pointing these out. > >> >> "Boolean, if the query is waiting because of a block / lock" reads >> really strange to me. "Boolean indicating if" would be a good start, >> but I'm not sure what you're trying to say with "block / lock" at all? > > > Yeah, me neither. What about: > "Boolean indicating if a query is in a wait state due to a conflict with > another backend." Maybe "Boolean indicating if a backend is currently waiting on a lock"? >> The use of single quotes in the descriptions, things like "This is the >> 'state' of" seems wrong. If we should use anything, it should be >> double quotes, but why do we need double quotes around that? And the >> reference to "think time" is just strange, IMO. >> > Yeah, that's a 'Scottism' (see, I used it again :-). I'll clean that up :-) >> In some other cases, the single quotes should probably be replaced >> with . >> >> NB: should probably be . >> > > I am actually looking for some helping in describing the " > function call" state. Any takers? Not sure how detailed you have to be - since it's a fairly uncommon thing, you can probalby get away with something as simple as "executing a fast-path function" or something like that? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Dec 6, 2011 at 6:38 AM, Magnus Hagander wrote: > On Sat, Nov 19, 2011 at 02:55, Scott Mead wrote: > > > > On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead wrote: > >> > >> On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead wrote: > >>> > >>> > >>> > >>> On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat wrote: > > On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith > wrote: > > On 11/15/2011 09:44 AM, Scott Mead wrote: > >> > >> Fell off the map last week, here's v2 that: > >> * RUNNING => active > >> * all states from CAPS to lower case > >> > > > > This looks like what I was hoping someone would add here now. Patch > > looks > > good, only issue I noticed was a spaces instead of a tab goof where > > you set > > beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c > > > > Missing pieces: > > > > -There is one regression test that uses pg_stat_activity that is > > broken now. > > -The documentation should list the new field and all of the states > it > > might > > include. That's a serious doc update from the minimal info > available > > right > > now. > > > > > > V3 Attached: > > > > * Updates Documentation > > -- Adds a separate table to cover each column of pg_stat_activity > > I like that a lot - we should've done taht years ago :-) > > For consistency, either both datname and usename should refer to their > respective oid, or none of them. > > application_name should probably have a link to the libpq > documentation for said parameter. > > "field is not set" should probably be "field is NULL" > > Thanks for pointing these out. > "Boolean, if the query is waiting because of a block / lock" reads > really strange to me. "Boolean indicating if" would be a good start, > but I'm not sure what you're trying to say with "block / lock" at all? > Yeah, me neither. What about: "Boolean indicating if a query is in a wait state due to a conflict with another backend." > > The way the list of states is built up looks really strange. I think > it should be built out of like other such places > instead - but I admit to not having checked what that actually looks > like in the output. > Agreed. I'll look at > > The use of single quotes in the descriptions, things like "This is the > 'state' of" seems wrong. If we should use anything, it should be > double quotes, but why do we need double quotes around that? And the > reference to "think time" is just strange, IMO. > > Yeah, that's a 'Scottism' (see, I used it again :-). I'll clean that up > In some other cases, the single quotes should probably be replaced > with . > > NB: should probably be . > > I am actually looking for some helping in describing the " function call" state. Any takers? > > > > * Adds 'state_change' (timestamp) column > > -- Tracks when the 'state' column is updated independently > >of when the 'query' column is updated > > Very useful. > > > > * renames procpid => pid > > Shouldn't we do this consistently if we do it? It's change in > pg_stat_get_activity() but not pg_stat_get_wal_senders()? I think we > either change in both functions, or none of them > Agreed > > > * Bug Fixes > > -- If a backend had never before issued a query, another > > session looking at pg_stat_activity would print not > > enabled> > > in the query column. > > -- query_start was being updated on a 'state' change, now only > updated > > on query > >change > > > > * Fixed 'rules' regression failure > > -- Added new columns for pg_stat_activity > > Also, I think state=-1 needs a symbolic value. STATE_UNDEFINED or > something like that. > Agreed. > > There's also a lot of random trailing whitespace in the patch - "git > diff" (which you seem to be using already, that's why I mention it) > will happily alert you about that - they should be removed. Or the > committer can clean that up of course, but it makes for less work ;) > > Thanks for pointing that out. > > The code is starting to look really good, but I think the docs > definitely need another round of work. > > Yeah, I figured they would. Thanks for going through them! -- Scott Mead OpenSCG, http://www.openscg.com -- > Magnus Hagander > Me: http://www.hagander.net/ > Work: http://www.redpill-linpro.com/ >
Re: [HACKERS] IDLE in transaction introspection
On Sat, Nov 19, 2011 at 02:55, Scott Mead wrote: > > On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead wrote: >> >> On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead wrote: >>> >>> >>> >>> On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat wrote: On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith wrote: > On 11/15/2011 09:44 AM, Scott Mead wrote: >> >> Fell off the map last week, here's v2 that: >> * RUNNING => active >> * all states from CAPS to lower case >> > > This looks like what I was hoping someone would add here now. Patch > looks > good, only issue I noticed was a spaces instead of a tab goof where > you set > beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c > > Missing pieces: > > -There is one regression test that uses pg_stat_activity that is > broken now. > -The documentation should list the new field and all of the states it > might > include. That's a serious doc update from the minimal info available > right > now. > > > V3 Attached: > > * Updates Documentation > -- Adds a separate table to cover each column of pg_stat_activity I like that a lot - we should've done taht years ago :-) For consistency, either both datname and usename should refer to their respective oid, or none of them. application_name should probably have a link to the libpq documentation for said parameter. "field is not set" should probably be "field is NULL" "Boolean, if the query is waiting because of a block / lock" reads really strange to me. "Boolean indicating if" would be a good start, but I'm not sure what you're trying to say with "block / lock" at all? The way the list of states is built up looks really strange. I think it should be built out of like other such places instead - but I admit to not having checked what that actually looks like in the output. The use of single quotes in the descriptions, things like "This is the 'state' of" seems wrong. If we should use anything, it should be double quotes, but why do we need double quotes around that? And the reference to "think time" is just strange, IMO. In some other cases, the single quotes should probably be replaced with . NB: should probably be . > * Adds 'state_change' (timestamp) column > -- Tracks when the 'state' column is updated independently > of when the 'query' column is updated Very useful. > * renames procpid => pid Shouldn't we do this consistently if we do it? It's change in pg_stat_get_activity() but not pg_stat_get_wal_senders()? I think we either change in both functions, or none of them > * Bug Fixes > -- If a backend had never before issued a query, another > session looking at pg_stat_activity would print enabled> > in the query column. > -- query_start was being updated on a 'state' change, now only updated > on query > change > > * Fixed 'rules' regression failure > -- Added new columns for pg_stat_activity Also, I think state=-1 needs a symbolic value. STATE_UNDEFINED or something like that. There's also a lot of random trailing whitespace in the patch - "git diff" (which you seem to be using already, that's why I mention it) will happily alert you about that - they should be removed. Or the committer can clean that up of course, but it makes for less work ;) The code is starting to look really good, but I think the docs definitely need another round of work. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead wrote: > > > On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat wrote: > >> On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith >> wrote: >> > On 11/15/2011 09:44 AM, Scott Mead wrote: >> >> >> >> Fell off the map last week, here's v2 that: >> >> * RUNNING => active >> >> * all states from CAPS to lower case >> >> >> > >> > This looks like what I was hoping someone would add here now. Patch >> looks >> > good, only issue I noticed was a spaces instead of a tab goof where you >> set >> > beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c >> > >> > Missing pieces: >> > >> > -There is one regression test that uses pg_stat_activity that is broken >> now. >> > -The documentation should list the new field and all of the states it >> might >> > include. That's a serious doc update from the minimal info available >> right >> > now. >> > I'm working on the doc update now, and just realized something interesting: My patch doesn't take the 'query_start' column into account. Basically, the query_start column actually corresponds to the most recent update of the 'state' field. It'd be easy enough to tie it to the 'query' field so that we are hooked in. The problem is, if I'm monitoring this, now I don't know how long I've been 'idle in transaction' for, I would only know that the last query started at 'query_start'. AFAICS: *) Adjust the query_start column to point only to the actual 'query' start time *) Allow the query_start column to be updated as part of the 'state' change *) Add a 'state_change' column (timestamp) Looking for opinions here... -- Scott Mead OpenSCG, http://www.openscg.com > >> > I know this issue has been beat up already some, but let me summarize >> and >> > extend that thinking a moment. I see two equally valid schools of >> thought >> > on how exactly to deal with introducing this change: >> > >> > -Add the new state field just as you've done it, but keep updating the >> query >> > text anyway. Do not rename current_query. Declare the overloading of >> > current_query as both a state and the query text to be deprecated in >> 9.3. >> > This would keep existing tools working fine against 9.2 and give a >> clean >> > transition period. >> > >> > -Forget about backward compatibility and just put all the breaking stuff >> > we've been meaning to do in here. If we're going to rename >> current_query to >> > query--what Scott's patch does here--that will force all code using >> > pg_stat_activity to be rewritten. This seems like the perfect time to >> also >> > change "procpid" to "pid", finally blow away that wart. >> > >> >> +1 >> > +1 for me as well. > > Anybody else? > > >> >> > I'll happily update all of the tools and samples I deal with to support >> this >> > change. Most of the ones I can think of will be simplified; they're >> already >> > parsing query_text and extracting the implicit state. Just operating >> on an >> > explicit one instead will be simpler and more robust. >> > >> >> lmk if you need help, we'll be doing this with some of our tools / >> projects anyway, it probably wouldn't hurt to coordinate. >> >> >> Robert Treat >> conjecture: xzilla.net >> consulting: omniti.com >> >> -- >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgsql-hackers >> > >
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat wrote: > On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith wrote: > > On 11/15/2011 09:44 AM, Scott Mead wrote: > >> > >> Fell off the map last week, here's v2 that: > >> * RUNNING => active > >> * all states from CAPS to lower case > >> > > > > This looks like what I was hoping someone would add here now. Patch > looks > > good, only issue I noticed was a spaces instead of a tab goof where you > set > > beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c > > > > Missing pieces: > > > > -There is one regression test that uses pg_stat_activity that is broken > now. > > -The documentation should list the new field and all of the states it > might > > include. That's a serious doc update from the minimal info available > right > > now. > > > > I know this issue has been beat up already some, but let me summarize and > > extend that thinking a moment. I see two equally valid schools of > thought > > on how exactly to deal with introducing this change: > > > > -Add the new state field just as you've done it, but keep updating the > query > > text anyway. Do not rename current_query. Declare the overloading of > > current_query as both a state and the query text to be deprecated in 9.3. > > This would keep existing tools working fine against 9.2 and give a clean > > transition period. > > > > -Forget about backward compatibility and just put all the breaking stuff > > we've been meaning to do in here. If we're going to rename > current_query to > > query--what Scott's patch does here--that will force all code using > > pg_stat_activity to be rewritten. This seems like the perfect time to > also > > change "procpid" to "pid", finally blow away that wart. > > > > +1 > +1 for me as well. Anybody else? > > > I'll happily update all of the tools and samples I deal with to support > this > > change. Most of the ones I can think of will be simplified; they're > already > > parsing query_text and extracting the implicit state. Just operating on > an > > explicit one instead will be simpler and more robust. > > > > lmk if you need help, we'll be doing this with some of our tools / > projects anyway, it probably wouldn't hurt to coordinate. > > > Robert Treat > conjecture: xzilla.net > consulting: omniti.com > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith wrote: > On 11/15/2011 09:44 AM, Scott Mead wrote: >> >> Fell off the map last week, here's v2 that: >> * RUNNING => active >> * all states from CAPS to lower case >> > > This looks like what I was hoping someone would add here now. Patch looks > good, only issue I noticed was a spaces instead of a tab goof where you set > beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c > > Missing pieces: > > -There is one regression test that uses pg_stat_activity that is broken now. > -The documentation should list the new field and all of the states it might > include. That's a serious doc update from the minimal info available right > now. > > I know this issue has been beat up already some, but let me summarize and > extend that thinking a moment. I see two equally valid schools of thought > on how exactly to deal with introducing this change: > > -Add the new state field just as you've done it, but keep updating the query > text anyway. Do not rename current_query. Declare the overloading of > current_query as both a state and the query text to be deprecated in 9.3. > This would keep existing tools working fine against 9.2 and give a clean > transition period. > > -Forget about backward compatibility and just put all the breaking stuff > we've been meaning to do in here. If we're going to rename current_query to > query--what Scott's patch does here--that will force all code using > pg_stat_activity to be rewritten. This seems like the perfect time to also > change "procpid" to "pid", finally blow away that wart. > +1 > I'll happily update all of the tools and samples I deal with to support this > change. Most of the ones I can think of will be simplified; they're already > parsing query_text and extracting the implicit state. Just operating on an > explicit one instead will be simpler and more robust. > lmk if you need help, we'll be doing this with some of our tools / projects anyway, it probably wouldn't hurt to coordinate. Robert Treat conjecture: xzilla.net consulting: omniti.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On 11/15/2011 09:44 AM, Scott Mead wrote: Fell off the map last week, here's v2 that: * RUNNING => active * all states from CAPS to lower case This looks like what I was hoping someone would add here now. Patch looks good, only issue I noticed was a spaces instead of a tab goof where you set beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c Missing pieces: -There is one regression test that uses pg_stat_activity that is broken now. -The documentation should list the new field and all of the states it might include. That's a serious doc update from the minimal info available right now. I know this issue has been beat up already some, but let me summarize and extend that thinking a moment. I see two equally valid schools of thought on how exactly to deal with introducing this change: -Add the new state field just as you've done it, but keep updating the query text anyway. Do not rename current_query. Declare the overloading of current_query as both a state and the query text to be deprecated in 9.3. This would keep existing tools working fine against 9.2 and give a clean transition period. -Forget about backward compatibility and just put all the breaking stuff we've been meaning to do in here. If we're going to rename current_query to query--what Scott's patch does here--that will force all code using pg_stat_activity to be rewritten. This seems like the perfect time to also change "procpid" to "pid", finally blow away that wart. I'll happily update all of the tools and samples I deal with to support this change. Most of the ones I can think of will be simplified; they're already parsing query_text and extracting the implicit state. Just operating on an explicit one instead will be simpler and more robust. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Thu, Nov 10, 2011 at 2:49 PM, Tom Lane wrote: > Bruce Momjian writes: > > Well, we could use an optional "details" string for that. If not, we > > are still using the magic-string approach, which I thought we didn't > > like. > > No, we're not using magic strings, we're using an enum --- maybe not an > officially declared enum type, but it's a column with a predetermined > set of possible values. It would be a magic string if it were still in > the "query" field and thus confusable with user-written queries. > Fell off the map last week, here's v2 that: * RUNNING => active * all states from CAPS to lower case -- Scott Mead OpenSCG http://www.openscg.com > >regards, tom lane > pg_stat_activity_state_query.v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Bruce Momjian writes: > Well, we could use an optional "details" string for that. If not, we > are still using the magic-string approach, which I thought we didn't > like. No, we're not using magic strings, we're using an enum --- maybe not an officially declared enum type, but it's a column with a predetermined set of possible values. It would be a magic string if it were still in the "query" field and thus confusable with user-written queries. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Tom Lane wrote: > Bruce Momjian writes: > > It might be cleaner to use booleans: > > active: t/f > > in transaction: t/f > > I don't think so, because that makes some very strict assumptions that > there are exactly four interesting states (an assumption that isn't > even true today, to judge by the activity strings we're using now). Well, we could use an optional "details" string for that. If not, we are still using the magic-string approach, which I thought we didn't like. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Bruce Momjian writes: > It might be cleaner to use booleans: > active: t/f > in transaction: t/f I don't think so, because that makes some very strict assumptions that there are exactly four interesting states (an assumption that isn't even true today, to judge by the activity strings we're using now). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Scott Mead wrote: > On Wed, Nov 2, 2011 at 4:12 AM, Albe Laurenz wrote: > > > Andrew Dunstan wrote: > > > On 11/01/2011 09:52 AM, Tom Lane wrote: > > >> I'm for just redefining the query field as "current or last > > >> query". > > > > > > +1 > > > > > >> I could go either way on whether to rename it. > > > > > > Rename it please. "current_query" will just be wrong. I'd be inclined > > > just to call it "query" or "query_string" and leave it to the docs to > > > define the exact semantics. > > > > +1 for renaming, +1 for a state column. > > I think it is overkill to keep a query history beyond that -- if you > > want that, > > you can resort to the log files. > > > > > ISTM that we're all for: > >creating a new column: state >renaming current_query => query > >State will display , , in transaction, etc... >query will display the last query that was executed. > > I've written this up in the attached patch, looking for feedback. (NB: > Originally I was using 9.1.1 release, I just did a git clone today to > generate this). It might be cleaner to use booleans: active: t/f in transaction: t/f or maybe instead of 'active': idle: t/f in transaction: t/f That avoids the magic string values for the state column. Those are much easier to query against too: WHERE NOT idle; -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Nov 5, 2011 9:02 AM, "Greg Smith" wrote: > > On 11/04/2011 05:01 PM, Tom Lane wrote: >> >> Scott Mead writes: >> >>> >>>I leave the waiting flag in place for posterity. With this in mind, is >>> the consensus: >>>RUNNING >>> or >>>ACTIVE >>> >> >> Personally, I'd go for lower case. >> > > > I was thinking it would be nice if this state looked like the WAL sender state values in pg_stat_replication, which are all lower case. For comparison those states are: > > startup > backup > catchup > streaming +1, it'll be easier to query against. > > -- > Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD > PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On 11/04/2011 05:01 PM, Tom Lane wrote: Scott Mead writes: I leave the waiting flag in place for posterity. With this in mind, is the consensus: RUNNING or ACTIVE Personally, I'd go for lower case. I was thinking it would be nice if this state looked like the WAL sender state values in pg_stat_replication, which are all lower case. For comparison those states are: startup backup catchup streaming -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 10:34 AM, Tom Lane wrote: > Magnus Hagander writes: >> I guess with the changes that showed different thing like fastpath, >> that makes sense. But if we just mapped the states that are there >> today straight off, is there any case where waiting can be true, when >> we're either idle or idle in transaction? I think not.. > > I'm not totally sure about that. I know that it's possible to see "idle > waiting" in the ps display, but that comes from getting blocked in parse > analysis before the command type has been determined. The > pg_stat_activity string is set a bit earlier, so *maybe* it's impossible > there. Still, why wire such an assumption into the structure of the > view? Robert's point about sinval catchup is another good one, though > I don't remember what that does to the pg_stat_activity display. > > BTW, a quick grep shows that there are four not two special values of > the activity string today: > > src/backend/tcop/postgres.c: 3792: > pgstat_report_activity(" in transaction (aborted)"); > src/backend/tcop/postgres.c: 3797: > pgstat_report_activity(" in transaction"); > src/backend/tcop/postgres.c: 3805: > pgstat_report_activity(""); > src/backend/tcop/postgres.c: 3925: > pgstat_report_activity(" function call"); > > It's also worth considering whether the "autovacuum:" that's prepended > by autovac_report_activity() ought to be folded into the state instead > of continuing to put something that's not valid SQL into the query > string. > Well, it would be interesting to see rows for autovacuum or replication processes showing up in pg_stat_activity with a corresponding state (autovacuum, walreciever?) and the "query" field showing what they are working on, at the risk that we'd need to build more complex parsing into the various monitoring scripts, but I guess it's no worse than before (and I guess probably easier on some level). Robert Treat play: xzilla.net work: omniti.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 3:28 PM, Robert Haas wrote: > On Fri, Nov 4, 2011 at 2:46 PM, Scott Mead wrote: >> If waiting == true, then state = WAITING >> else >> state = appropriate state > > No, I think the state and waiting columns should be completely independent. > If they aren't I don't think we need both columns. +1 for leaving them independent though. Robert Treat play: xzilla.net work: omniti.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 2:46 PM, Scott Mead wrote: > If waiting == true, then state = WAITING > else > state = appropriate state No, I think the state and waiting columns should be completely independent. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Scott Mead writes: >I leave the waiting flag in place for posterity. With this in mind, is > the consensus: >RUNNING > or >ACTIVE Personally, I'd go for lower case. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 2:31 PM, Robert Haas wrote: > On Fri, Nov 4, 2011 at 2:28 PM, Tom Lane wrote: > > Robert Haas writes: > >> Maybe there's a better term than "running", like "in progress" or > >> something of that sort. > > > > "active"? > > +1. > >Letting this one 'poll' a bit more before I post the patch, but here's what I have: If waiting == true, then state = WAITING else state = appropriate state I leave the waiting flag in place for posterity. With this in mind, is the consensus: RUNNING or ACTIVE -- Scott Mead OpenSCG http://www.openscg.com -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 2:28 PM, Tom Lane wrote: > Robert Haas writes: >> Maybe there's a better term than "running", like "in progress" or >> something of that sort. > > "active"? +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Robert Haas writes: > Maybe there's a better term than "running", like "in progress" or > something of that sort. "active"? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 12:46 PM, Marti Raudsepp wrote: > On Fri, Nov 4, 2011 at 15:42, Tom Lane wrote: >> Marti Raudsepp writes: >>> While we're already breaking everything, we could remove the "waiting" >>> column and use a state with value 'waiting' instead. > >> -1 ... I think it's useful to see the underlying state as well as the >> waiting flag. Also, this would represent breakage of part of the API >> that doesn't need to be broken. > > Well the waiting column can stay. My concern is that listing lock-wait > backends as 'running' will be misleading for users. pg_stat_activity > is a pretty common starting point for debugging problems and if > there's a new column that says a query is 'running', then I'm afraid > the current waiting 't' and 'f' values will be too subtle for users to > notice. (I find that it's too subtle already now, if you don't know > what you're looking for). Maybe there's a better term than "running", like "in progress" or something of that sort. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 15:42, Tom Lane wrote: > Marti Raudsepp writes: >> While we're already breaking everything, we could remove the "waiting" >> column and use a state with value 'waiting' instead. > -1 ... I think it's useful to see the underlying state as well as the > waiting flag. Also, this would represent breakage of part of the API > that doesn't need to be broken. Well the waiting column can stay. My concern is that listing lock-wait backends as 'running' will be misleading for users. pg_stat_activity is a pretty common starting point for debugging problems and if there's a new column that says a query is 'running', then I'm afraid the current waiting 't' and 'f' values will be too subtle for users to notice. (I find that it's too subtle already now, if you don't know what you're looking for). Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 10:34 AM, Tom Lane wrote: > Robert's point about sinval catchup is another good one, though > I don't remember what that does to the pg_stat_activity display. My thought was that sinval catchup might require acquiring a relation lock (e.g. on pg_class), and that might block waiting for a lock. Not sure it's possible, but even if it can't happen today, it doesn't seem impossible that we might want to let it happen in the future. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Magnus Hagander writes: > I guess with the changes that showed different thing like fastpath, > that makes sense. But if we just mapped the states that are there > today straight off, is there any case where waiting can be true, when > we're either idle or idle in transaction? I think not.. I'm not totally sure about that. I know that it's possible to see "idle waiting" in the ps display, but that comes from getting blocked in parse analysis before the command type has been determined. The pg_stat_activity string is set a bit earlier, so *maybe* it's impossible there. Still, why wire such an assumption into the structure of the view? Robert's point about sinval catchup is another good one, though I don't remember what that does to the pg_stat_activity display. BTW, a quick grep shows that there are four not two special values of the activity string today: src/backend/tcop/postgres.c: 3792: pgstat_report_activity(" in transaction (aborted)"); src/backend/tcop/postgres.c: 3797: pgstat_report_activity(" in transaction"); src/backend/tcop/postgres.c: 3805: pgstat_report_activity(""); src/backend/tcop/postgres.c: 3925: pgstat_report_activity(" function call"); It's also worth considering whether the "autovacuum:" that's prepended by autovac_report_activity() ought to be folded into the state instead of continuing to put something that's not valid SQL into the query string. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 9:48 AM, Magnus Hagander wrote: > On Fri, Nov 4, 2011 at 14:42, Tom Lane wrote: > > Marti Raudsepp writes: > >> While we're already breaking everything, we could remove the "waiting" > >> column and use a state with value 'waiting' instead. > > > > -1 ... I think it's useful to see the underlying state as well as the > > waiting flag. Also, this would represent breakage of part of the API > > that doesn't need to be broken. > > I guess with the changes that showed different thing like fastpath, > that makes sense. But if we just mapped the states that are there > today straight off, is there any case where waiting can be true, when > we're either idle or idle in transaction? I think not.. > Leave the waiting column and display 'WAITING' if st_watiting = 1 seems to be the clearest solution. I can see people getting confused by waiting = 't' and state='RUNNING'. > > > >> Also, returning these as text seems a little lame. Should there be an > >> enum type for that? > > > > Perhaps, but we don't really use enum types in any other system views, > > so inventing one here would be out of character. > > Yeha, that seems inconsistent. Using a single character might work - > but it's not particularly user-friendly to do that in the view itself. > I'll nuke the '<>', which is definitely an improvement, anything more complex seems like it'll require fairly wordy documentation. -- Scott Mead OpenSCG http://www.openscg.com > > -- > Magnus Hagander > Me: http://www.hagander.net/ > Work: http://www.redpill-linpro.com/ > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 9:48 AM, Magnus Hagander wrote: > On Fri, Nov 4, 2011 at 14:42, Tom Lane wrote: >> Marti Raudsepp writes: >>> While we're already breaking everything, we could remove the "waiting" >>> column and use a state with value 'waiting' instead. >> >> -1 ... I think it's useful to see the underlying state as well as the >> waiting flag. Also, this would represent breakage of part of the API >> that doesn't need to be broken. > > I guess with the changes that showed different thing like fastpath, > that makes sense. But if we just mapped the states that are there > today straight off, is there any case where waiting can be true, when > we're either idle or idle in transaction? I think not.. Maybe if we get a sinval reset while we're just sitting there? Not sure. In any case, I agree with Tom. I think that most people will be happy to have a "query" column and a "state" column rather than a "current_query" column that amalgamates the two, but I see no reason to tinker with the waiting column if the changes we want to make don't otherwise require it. >>> Also, returning these as text seems a little lame. Should there be an >>> enum type for that? >> >> Perhaps, but we don't really use enum types in any other system views, >> so inventing one here would be out of character. > > Yeha, that seems inconsistent. Using a single character might work - > but it's not particularly user-friendly to do that in the view itself. +1 for keeping it as text, but loosing the angle brackets. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 14:42, Tom Lane wrote: > Marti Raudsepp writes: >> While we're already breaking everything, we could remove the "waiting" >> column and use a state with value 'waiting' instead. > > -1 ... I think it's useful to see the underlying state as well as the > waiting flag. Also, this would represent breakage of part of the API > that doesn't need to be broken. I guess with the changes that showed different thing like fastpath, that makes sense. But if we just mapped the states that are there today straight off, is there any case where waiting can be true, when we're either idle or idle in transaction? I think not.. >> Also, returning these as text seems a little lame. Should there be an >> enum type for that? > > Perhaps, but we don't really use enum types in any other system views, > so inventing one here would be out of character. Yeha, that seems inconsistent. Using a single character might work - but it's not particularly user-friendly to do that in the view itself. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Marti Raudsepp writes: > While we're already breaking everything, we could remove the "waiting" > column and use a state with value 'waiting' instead. -1 ... I think it's useful to see the underlying state as well as the waiting flag. Also, this would represent breakage of part of the API that doesn't need to be broken. > Also, returning these as text seems a little lame. Should there be an > enum type for that? Perhaps, but we don't really use enum types in any other system views, so inventing one here would be out of character. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Wed, Nov 2, 2011 at 20:18, Scott Mead wrote: > State will display , , in transaction, etc... While we're already breaking everything, we could remove the "waiting" column and use a state with value 'waiting' instead. Also, returning these as text seems a little lame. Should there be an enum type for that? Or we could return single-character mnemonics like some pg_catalog tables do, and substitute them in the view. Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Fri, Nov 4, 2011 at 03:27, Fujii Masao wrote: > On Thu, Nov 3, 2011 at 3:18 AM, Scott Mead wrote: >> ISTM that we're all for: >> creating a new column: state >> renaming current_query => query >> State will display , , in transaction, etc... >> query will display the last query that was executed. > > The greater/less-than-sign is still required in the State display? +1 for removing that. I think the only reason for them to be there in the first place was to decrease the chance of matching up against a real query - but that's not going to happen if they are in a separate field... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Thu, Nov 3, 2011 at 3:18 AM, Scott Mead wrote: > ISTM that we're all for: > creating a new column: state > renaming current_query => query > State will display , , in transaction, etc... > query will display the last query that was executed. The greater/less-than-sign is still required in the State display? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Wed, Nov 2, 2011 at 4:12 AM, Albe Laurenz wrote: > Andrew Dunstan wrote: > > On 11/01/2011 09:52 AM, Tom Lane wrote: > >> I'm for just redefining the query field as "current or last > >> query". > > > > +1 > > > >> I could go either way on whether to rename it. > > > > Rename it please. "current_query" will just be wrong. I'd be inclined > > just to call it "query" or "query_string" and leave it to the docs to > > define the exact semantics. > > +1 for renaming, +1 for a state column. > I think it is overkill to keep a query history beyond that -- if you > want that, > you can resort to the log files. > > ISTM that we're all for: creating a new column: state renaming current_query => query State will display , , in transaction, etc... query will display the last query that was executed. I've written this up in the attached patch, looking for feedback. (NB: Originally I was using 9.1.1 release, I just did a git clone today to generate this). -- Scott Mead OpenSCG http://www.openscg.com > Yours, > Laurenz Albe > pg_stat_activity_state_query.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Andrew Dunstan wrote: > On 11/01/2011 09:52 AM, Tom Lane wrote: >> I'm for just redefining the query field as "current or last >> query". > > +1 > >> I could go either way on whether to rename it. > > Rename it please. "current_query" will just be wrong. I'd be inclined > just to call it "query" or "query_string" and leave it to the docs to > define the exact semantics. +1 for renaming, +1 for a state column. I think it is overkill to keep a query history beyond that -- if you want that, you can resort to the log files. Yours, Laurenz Albe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 01, 2011 at 10:13:52AM -0400, Andrew Dunstan wrote: > > > On 11/01/2011 09:52 AM, Tom Lane wrote: > >Simon Riggs writes: > >>Why not leave it exactly as it is, and add a previous_query column? > >>That gives you exactly what you need without breaking anything. > >That would cost twice as much shared memory for query strings, and twice > >as much time to update the strings, for what seems pretty marginal > >value. I'm for just redefining the query field as "current or last > >query". > > +1 > > >I could go either way on whether to rename it. > > Rename it please. "current_query" will just be wrong. I'd be > inclined just to call it "query" or "query_string" and leave it to > the docs to define the exact semantics. +1 on providing the most-recent-query info +1 on the state/query split as a means +1 rename from current_query (i.e. break it) personalbikeshedcolor: query_string Personal opinion on "backwards compatability" matches Robert's: things that affect admin functionality are less of an issue than those that impact user (i.e. coder) SQL. And definitely break it: I may chose to fix it by bodging in a view for the old behavior myself, but that's my choice. Perhaps provide an example view def in change notes if you really want to. For myself, when making fixes to monitor scripts for this type of change, my reaction is probably "Yes, finally, I'm not regexing on the d*mn query string anymore!" Ross P.S. though apparently it doesn't bother me enough to create and submit a patch myself. :-( -- Ross Reedstrom, Ph.D. reeds...@rice.edu Systems Engineer & Admin, Research Scientistphone: 713-348-6166 Connexions http://cnx.orgfax: 713-348-3665 Rice University MS-375, Houston, TX 77005 GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane wrote: > Robert Haas writes: > > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane wrote: > >> That would cost twice as much shared memory for query strings, and twice > >> as much time to update the strings, for what seems pretty marginal > >> value. I'm for just redefining the query field as "current or last > >> query". > > > Not really. You could just store it once in shared memory, and put > > the complexity in the view definition. > > I understood the proposal to be "store the previous query in addition > to the current-query-if-any". If that's not what was meant, then my > objection was incorrect. However, like you, I'm pretty dubious of > having two mostly-redundant fields in the view definition, just because > of window width issues. > The biggest reason I dislike the multi-field approach is because it limits us to only the [single] previous_query in the system with all the overhead we talked about (memory, window width and messing with system catalogs in general). That's actually why I implemented it the way I did, just by appending the last query on the end of the string when it's in transaction. Marti wrote: I'd very much like to see a more generic solution: a runtime query log > facility that can be queried in any way you want. pg_stat_statements > comes close, but is limited too due to its (arbitrary, I find) > deduplication -- you can't query for "10 last statements from process > N" since it has no notion of processes, just users and databases. This is what I'd really like to see (just haven't had time as it is a much bigger project). The next question my devs ask is "what were the last five queries that ran"... "can you show me an overview of an entire transaction" etc... That being said, having the previous_query available feels like it fixes about 80% of the *problem*; transaction profiling, or looking back 10 / 15 / 20 queries is [immensely] useful, but I find that the bigger need is the ability to short-circuit dba / dev back-n-forth by just saying "Your app refused to commit/rollback after running query XYZ". Robert Wrote: > Yeah. Otherwise, people who are parsing the hard-coded strings > and are likely to get confused. I would be interested ( and frankly very surprised ) to find out if many monitoring tools are actually parsing that field. Most that I see just dump whatever is in current_query to the user. I would imaging that, so long as the server obeyed pgstat_track_activity_size most tools would behave nicely. Now... all that being said, I've implemented the 'previous_query' column and (maybe just for my own benefit), is the PostgreSQL community interested in the patch? -- Scott Mead OpenSCG http://www.openscg.com > >regards, tom lane >
Re: [HACKERS] IDLE in transaction introspection
On 11/01/2011 09:52 AM, Tom Lane wrote: Simon Riggs writes: Why not leave it exactly as it is, and add a previous_query column? That gives you exactly what you need without breaking anything. That would cost twice as much shared memory for query strings, and twice as much time to update the strings, for what seems pretty marginal value. I'm for just redefining the query field as "current or last query". +1 I could go either way on whether to rename it. Rename it please. "current_query" will just be wrong. I'd be inclined just to call it "query" or "query_string" and leave it to the docs to define the exact semantics. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 1:20 PM, Magnus Hagander wrote: > On Tue, Nov 1, 2011 at 18:11, Scott Mead wrote: >> >> On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane wrote: >>> >>> Robert Haas writes: >>> > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane wrote: >>> >> That would cost twice as much shared memory for query strings, and >>> >> twice >>> >> as much time to update the strings, for what seems pretty marginal >>> >> value. I'm for just redefining the query field as "current or last >>> >> query". >>> >>> > Not really. You could just store it once in shared memory, and put >>> > the complexity in the view definition. >>> >>> I understood the proposal to be "store the previous query in addition >>> to the current-query-if-any". If that's not what was meant, then my >>> objection was incorrect. However, like you, I'm pretty dubious of >>> having two mostly-redundant fields in the view definition, just because >>> of window width issues. >> >> The biggest reason I dislike the multi-field approach is because it limits >> us to only the [single] previous_query in the system with all the overhead >> we talked about (memory, window width and messing with system catalogs in >> general). That's actually why I implemented it the way I did, just by >> appending the last query on the end of the string when it's in >> transaction. > > Well, by appending it in that field, you require the end > user/monitoring app to parse out the string anyway, so you're not > exactly making life easier on the consumer of the information.. > +1 > >>> Marti wrote: >>> >>> I'd very much like to see a more generic solution: a runtime query log >>> facility that can be queried in any way you want. pg_stat_statements >>> comes close, but is limited too due to its (arbitrary, I find) >>> deduplication -- you can't query for "10 last statements from process >>> N" since it has no notion of processes, just users and databases. >> >> This is what I'd really like to see (just haven't had time as it is a much >> bigger project). The next question my devs ask is "what were the last five >> queries that ran"... "can you show me an overview of an entire transaction" >> etc... >> That being said, having the previous_query available feels like it fixes >> about 80% of the *problem*; transaction profiling, or looking back 10 / 15 / >> 20 queries is [immensely] useful, but I find that the bigger need is the >> ability to short-circuit dba / dev back-n-forth by just saying "Your app >> refused to commit/rollback after running query XYZ". > > This would be great, but as you say, it's a different project. > +1 (but I'd like to see that different project) > Perhaps something could be made along the line of each backend keeping > it's *own* set of old queries, and then making it available to a > specific function ("pg_get_last_queries_for_backend(nnn)") - since > this is not the type of information you'd ask for often, it would be > ok if getting this data took longer.. And you seldom want "give me the > last 5 queries for each backend at once". > > >>> Robert Wrote: >>> Yeah. Otherwise, people who are parsing the hard-coded strings >>> and are likely to get confused. >> >> I would be interested ( and frankly very surprised ) to find out if many >> monitoring tools are actually parsing that field. Most that I see just dump >> whatever is in current_query to the user. I would imaging that, so long as >> the server obeyed pgstat_track_activity_size most tools would behave nicely. > > Really? I'd assume every single monitoring tool that graphs how many > active connections you have (vs idle vs idle in transaction) would do > just this. > Having written and/or patched dozens of these types of things, your spot on; all of the ones with anything other than the most brain dead of monitoring parse for IDLE and in transaction. That said, I'm happy to see the {active|idle|idle in txn} status field and "query_string" fields show up and break backwards compatibility. Updating the tools will be simple for those who need it, and make a view to work around it will be simple for those who don't. Happy to add an example view definition to the docs if it will help. Robert Treat conjecture: xzilla.net consulting: omniti.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 18:40, Scott Mead wrote: > > > On Tue, Nov 1, 2011 at 1:20 PM, Magnus Hagander wrote: >> >> On Tue, Nov 1, 2011 at 18:11, Scott Mead wrote: >> > >> > On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane wrote: >> >> >> >> Robert Haas writes: >> >> > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane wrote: >> >> >> That would cost twice as much shared memory for query strings, and >> >> >> twice >> >> >> as much time to update the strings, for what seems pretty marginal >> >> >> value. I'm for just redefining the query field as "current or last >> >> >> query". >> >> >> >> > Not really. You could just store it once in shared memory, and put >> >> > the complexity in the view definition. >> >> >> >> I understood the proposal to be "store the previous query in addition >> >> to the current-query-if-any". If that's not what was meant, then my >> >> objection was incorrect. However, like you, I'm pretty dubious of >> >> having two mostly-redundant fields in the view definition, just because >> >> of window width issues. >> > >> > The biggest reason I dislike the multi-field approach is because it >> > limits >> > us to only the [single] previous_query in the system with all the >> > overhead >> > we talked about (memory, window width and messing with system catalogs >> > in >> > general). That's actually why I implemented it the way I did, just by >> > appending the last query on the end of the string when it's in >> > transaction. >> >> Well, by appending it in that field, you require the end >> user/monitoring app to parse out the string anyway, so you're not >> exactly making life easier on the consumer of the information.. >> >> >> >> Marti wrote: >> >> >> >> I'd very much like to see a more generic solution: a runtime query log >> >> facility that can be queried in any way you want. pg_stat_statements >> >> comes close, but is limited too due to its (arbitrary, I find) >> >> deduplication -- you can't query for "10 last statements from process >> >> N" since it has no notion of processes, just users and databases. >> > >> > This is what I'd really like to see (just haven't had time as it is a >> > much >> > bigger project). The next question my devs ask is "what were the last >> > five >> > queries that ran"... "can you show me an overview of an entire >> > transaction" >> > etc... >> > That being said, having the previous_query available feels like it >> > fixes >> > about 80% of the *problem*; transaction profiling, or looking back 10 / >> > 15 / >> > 20 queries is [immensely] useful, but I find that the bigger need is the >> > ability to short-circuit dba / dev back-n-forth by just saying "Your app >> > refused to commit/rollback after running query XYZ". >> >> This would be great, but as you say, it's a different project. >> >> Perhaps something could be made along the line of each backend keeping >> it's *own* set of old queries, and then making it available to a >> specific function ("pg_get_last_queries_for_backend(nnn)") > > Yeah, I was kind of thinking this too, I just feel dirty adding a (n > * track_activity_query_size) overhead to shared memory for tracking that if > we're already concerned about adding just a 'previous_query' string. It's > easy enough to either hard-code or set a limit on 'n', but, if I were to do > that, is it something that would be accepted? (my ability to code > not-withstanding :-) No, I meant storing it in backend local memory, and then transfer it upon request. That would remove locking requirements etc. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 1:20 PM, Magnus Hagander wrote: > On Tue, Nov 1, 2011 at 18:11, Scott Mead wrote: > > > > On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane wrote: > >> > >> Robert Haas writes: > >> > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane wrote: > >> >> That would cost twice as much shared memory for query strings, and > >> >> twice > >> >> as much time to update the strings, for what seems pretty marginal > >> >> value. I'm for just redefining the query field as "current or last > >> >> query". > >> > >> > Not really. You could just store it once in shared memory, and put > >> > the complexity in the view definition. > >> > >> I understood the proposal to be "store the previous query in addition > >> to the current-query-if-any". If that's not what was meant, then my > >> objection was incorrect. However, like you, I'm pretty dubious of > >> having two mostly-redundant fields in the view definition, just because > >> of window width issues. > > > > The biggest reason I dislike the multi-field approach is because it > limits > > us to only the [single] previous_query in the system with all the > overhead > > we talked about (memory, window width and messing with system catalogs > in > > general). That's actually why I implemented it the way I did, just by > > appending the last query on the end of the string when it's in > > transaction. > > Well, by appending it in that field, you require the end > user/monitoring app to parse out the string anyway, so you're not > exactly making life easier on the consumer of the information.. > > > >> Marti wrote: > >> > >> I'd very much like to see a more generic solution: a runtime query log > >> facility that can be queried in any way you want. pg_stat_statements > >> comes close, but is limited too due to its (arbitrary, I find) > >> deduplication -- you can't query for "10 last statements from process > >> N" since it has no notion of processes, just users and databases. > > > > This is what I'd really like to see (just haven't had time as it is a > much > > bigger project). The next question my devs ask is "what were the last > five > > queries that ran"... "can you show me an overview of an entire > transaction" > > etc... > > That being said, having the previous_query available feels like it > fixes > > about 80% of the *problem*; transaction profiling, or looking back 10 / > 15 / > > 20 queries is [immensely] useful, but I find that the bigger need is the > > ability to short-circuit dba / dev back-n-forth by just saying "Your app > > refused to commit/rollback after running query XYZ". > > This would be great, but as you say, it's a different project. > > Perhaps something could be made along the line of each backend keeping > it's *own* set of old queries, and then making it available to a > specific function ("pg_get_last_queries_for_backend(nnn)") Yeah, I was kind of thinking this too, I just feel dirty adding a (*n * track_activity_query_size) *overhead to shared memory for tracking that if we're already concerned about adding just a 'previous_query' string. It's easy enough to either hard-code or set a limit on 'n', but, if I were to do that, is it something that would be accepted? (my ability to code not-withstanding :-) > - since > this is not the type of information you'd ask for often, it would be > ok if getting this data took longer.. And you seldom want "give me the > last 5 queries for each backend at once". > I'm more concerned with the overhead of managing that array every single time that a backend updates its status than I am on retrieval. I guess if the array were small enough and the the logic clean, it could end up having about the same overhead as what I'm doing now (obviously, I'm still gobbling more memory). Doing that *would* allow us to get away from concerns about breaking monitoring tools and the like. -- Scott Mead OpenSCG http://www.openscg.com > > > >> Robert Wrote: > >> Yeah. Otherwise, people who are parsing the hard-coded strings > >> and are likely to get confused. > > > > I would be interested ( and frankly very surprised ) to find out if many > > monitoring tools are actually parsing that field. Most that I see just > dump > > whatever is in current_query to the user. I would imaging that, so long > as > > the server obeyed pgstat_track_activity_size most tools would behave > nicely. > > Really? I'd assume every single monitoring tool that graphs how many > active connections you have (vs idle vs idle in transaction) would do > just this. > > -- > Magnus Hagander > Me: http://www.hagander.net/ > Work: http://www.redpill-linpro.com/ >
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 18:11, Scott Mead wrote: > > On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane wrote: >> >> Robert Haas writes: >> > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane wrote: >> >> That would cost twice as much shared memory for query strings, and >> >> twice >> >> as much time to update the strings, for what seems pretty marginal >> >> value. I'm for just redefining the query field as "current or last >> >> query". >> >> > Not really. You could just store it once in shared memory, and put >> > the complexity in the view definition. >> >> I understood the proposal to be "store the previous query in addition >> to the current-query-if-any". If that's not what was meant, then my >> objection was incorrect. However, like you, I'm pretty dubious of >> having two mostly-redundant fields in the view definition, just because >> of window width issues. > > The biggest reason I dislike the multi-field approach is because it limits > us to only the [single] previous_query in the system with all the overhead > we talked about (memory, window width and messing with system catalogs in > general). That's actually why I implemented it the way I did, just by > appending the last query on the end of the string when it's in > transaction. Well, by appending it in that field, you require the end user/monitoring app to parse out the string anyway, so you're not exactly making life easier on the consumer of the information.. >> Marti wrote: >> >> I'd very much like to see a more generic solution: a runtime query log >> facility that can be queried in any way you want. pg_stat_statements >> comes close, but is limited too due to its (arbitrary, I find) >> deduplication -- you can't query for "10 last statements from process >> N" since it has no notion of processes, just users and databases. > > This is what I'd really like to see (just haven't had time as it is a much > bigger project). The next question my devs ask is "what were the last five > queries that ran"... "can you show me an overview of an entire transaction" > etc... > That being said, having the previous_query available feels like it fixes > about 80% of the *problem*; transaction profiling, or looking back 10 / 15 / > 20 queries is [immensely] useful, but I find that the bigger need is the > ability to short-circuit dba / dev back-n-forth by just saying "Your app > refused to commit/rollback after running query XYZ". This would be great, but as you say, it's a different project. Perhaps something could be made along the line of each backend keeping it's *own* set of old queries, and then making it available to a specific function ("pg_get_last_queries_for_backend(nnn)") - since this is not the type of information you'd ask for often, it would be ok if getting this data took longer.. And you seldom want "give me the last 5 queries for each backend at once". >> Robert Wrote: >> Yeah. Otherwise, people who are parsing the hard-coded strings >> and are likely to get confused. > > I would be interested ( and frankly very surprised ) to find out if many > monitoring tools are actually parsing that field. Most that I see just dump > whatever is in current_query to the user. I would imaging that, so long as > the server obeyed pgstat_track_activity_size most tools would behave nicely. Really? I'd assume every single monitoring tool that graphs how many active connections you have (vs idle vs idle in transaction) would do just this. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
2011/11/1 Marti Raudsepp : > On Mon, Oct 31, 2011 at 23:37, Scott Mead wrote: >> So I wrote the attached patch, it just turns in transaction into: >> " in transaction\n: Previous: ". After seeing >> how quickly our dev's fixed the issue once they saw prepared statement XYZ, > > Solving this problem is a good idea, but I don't particularly like the > proposed solution. I think the proposed state/query split is going to > make pg_stat_activity more confusing, especially if even idle > connections get a query string. And if we display the last query > there, why not the one before that? etc. (Adding a "state" column > might not be a bad idea though) > > I'd very much like to see a more generic solution: a runtime query log > facility that can be queried in any way you want. pg_stat_statements > comes close, but is limited too due to its (arbitrary, I find) > deduplication -- you can't query for "10 last statements from process > N" since it has no notion of processes, just users and databases. I like the idea to have a pg_stat_activity with an history, it can be in another view with a GUC to define how many queries to keep per pid. > > So far my developers are simply grepping pg_log, but you can't use the > full power of SQL there. I know there's csvlog, but the pains aren't > big enough to make attempt to use that. > > Regards, > Marti > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Robert Haas writes: > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane wrote: >> That would cost twice as much shared memory for query strings, and twice >> as much time to update the strings, for what seems pretty marginal >> value. I'm for just redefining the query field as "current or last >> query". > Not really. You could just store it once in shared memory, and put > the complexity in the view definition. I understood the proposal to be "store the previous query in addition to the current-query-if-any". If that's not what was meant, then my objection was incorrect. However, like you, I'm pretty dubious of having two mostly-redundant fields in the view definition, just because of window width issues. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On 2011-11-01 21:13, Andrew Dunstan wrote: Rename it please. "current_query" will just be wrong. I'd be inclined just to call it "query" or "query_string" and leave it to the docs to define the exact semantics. I think "query" for a query that isn't ongoing would be just as wrong. How about "last_query"? Jeroen -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 23:37, Scott Mead wrote: > So I wrote the attached patch, it just turns in transaction into: > " in transaction\n: Previous: ". After seeing > how quickly our dev's fixed the issue once they saw prepared statement XYZ, Solving this problem is a good idea, but I don't particularly like the proposed solution. I think the proposed state/query split is going to make pg_stat_activity more confusing, especially if even idle connections get a query string. And if we display the last query there, why not the one before that? etc. (Adding a "state" column might not be a bad idea though) I'd very much like to see a more generic solution: a runtime query log facility that can be queried in any way you want. pg_stat_statements comes close, but is limited too due to its (arbitrary, I find) deduplication -- you can't query for "10 last statements from process N" since it has no notion of processes, just users and databases. So far my developers are simply grepping pg_log, but you can't use the full power of SQL there. I know there's csvlog, but the pains aren't big enough to make attempt to use that. Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane wrote: > Simon Riggs writes: >> Why not leave it exactly as it is, and add a previous_query column? > >> That gives you exactly what you need without breaking anything. > > That would cost twice as much shared memory for query strings, and twice > as much time to update the strings, for what seems pretty marginal > value. I'm for just redefining the query field as "current or last > query". Not really. You could just store it once in shared memory, and put the complexity in the view definition. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 1:52 PM, Tom Lane wrote: > Simon Riggs writes: >> Why not leave it exactly as it is, and add a previous_query column? > >> That gives you exactly what you need without breaking anything. > > That would cost twice as much shared memory for query strings, and twice > as much time to update the strings, for what seems pretty marginal > value. I'm for just redefining the query field as "current or last > query". I could go either way on whether to rename it. That's a good reason. > If anyone's really hot about backward compatibility, it would not be > very hard to create a view that replicates the old behavior working > from a "state" column and a current-or-last-query column. I'm in favour of change, when that has a purpose, just like you. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
Simon Riggs writes: > Why not leave it exactly as it is, and add a previous_query column? > That gives you exactly what you need without breaking anything. That would cost twice as much shared memory for query strings, and twice as much time to update the strings, for what seems pretty marginal value. I'm for just redefining the query field as "current or last query". I could go either way on whether to rename it. If anyone's really hot about backward compatibility, it would not be very hard to create a view that replicates the old behavior working from a "state" column and a current-or-last-query column. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 8:41 AM, Magnus Hagander wrote: > On Tue, Nov 1, 2011 at 13:19, Simon Riggs wrote: >> On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas wrote: >>> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander >>> wrote: Actually, for the future, it might be useful to have a "state" column, that holds the idle/in transaction/running status, instead of the tools having to parse the query text to get that information... >>> >>> +1 for doing it this way. Splitting "current_query" into "query" and >>> "state" would be more elegant and easier to use all around. >> >> Why not leave it exactly as it is, and add a previous_query column? >> >> That gives you exactly what you need without breaking anything. > > That would be the backwards compatible way I suggested. > > That said, I think there's still value in exposing a "state" column, > and to encourage people not to rely on the text in the query column. > Then you can add it to your list of things to remove in 10.0 :-) Personally, I think we're getting a bit overexcited about backward compatibility here. We make backward-incompatible changes in every release. Things that change the behavior of user queries (like reserving keywords, or other changes in syntax, or having the same syntax now mean something different) cause a fair amount of user pain, and we're justifiably cautious about making them. But changes that have to do with server administration, as far as I can see, result in much less pain. Splitting the current_query field into query and state is going to require only the most minimal adjustment to user code, and anyone for whom it's really a problem can easily create their own view that mimics the old behavior. On the flip side, keeping both fields around is going to make the pg_stat_activity, which is already extremely wide, even more difficult to read in a window of any reasonable width, especially because the field we're proposing to duplicate is the widest one by far. I also doubt very much that it creates a meaningful migration path; most people will just keep doing it the old way until it breaks, and even new users may not realize that one column is nominally deprecated. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 12:41 PM, Magnus Hagander wrote: > On Tue, Nov 1, 2011 at 13:19, Simon Riggs wrote: >> On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas wrote: >>> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander >>> wrote: Actually, for the future, it might be useful to have a "state" column, that holds the idle/in transaction/running status, instead of the tools having to parse the query text to get that information... >>> >>> +1 for doing it this way. Splitting "current_query" into "query" and >>> "state" would be more elegant and easier to use all around. >> >> Why not leave it exactly as it is, and add a previous_query column? >> >> That gives you exactly what you need without breaking anything. > > That would be the backwards compatible way I suggested. Sorry Magnus, I hadn't read the whole thread. +1 to your suggestion. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 13:19, Simon Riggs wrote: > On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas wrote: >> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander wrote: >>> Actually, for the future, it might be useful to have a "state" column, >>> that holds the idle/in transaction/running status, instead of the >>> tools having to parse the query text to get that information... >> >> +1 for doing it this way. Splitting "current_query" into "query" and >> "state" would be more elegant and easier to use all around. > > Why not leave it exactly as it is, and add a previous_query column? > > That gives you exactly what you need without breaking anything. That would be the backwards compatible way I suggested. That said, I think there's still value in exposing a "state" column, and to encourage people not to rely on the text in the query column. Then you can add it to your list of things to remove in 10.0 :-) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas wrote: > On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander wrote: >> Actually, for the future, it might be useful to have a "state" column, >> that holds the idle/in transaction/running status, instead of the >> tools having to parse the query text to get that information... > > +1 for doing it this way. Splitting "current_query" into "query" and > "state" would be more elegant and easier to use all around. Why not leave it exactly as it is, and add a previous_query column? That gives you exactly what you need without breaking anything. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 7:38 AM, Magnus Hagander wrote: > If we are doing it, it might be useful to just call it "query", so > that it is dead obvious to people that the definition changed.. Yeah. Otherwise, people who are parsing the hard-coded strings and are likely to get confused. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Tue, Nov 1, 2011 at 00:18, Scott Mead wrote: > > > On Mon, Oct 31, 2011 at 6:13 PM, Robert Haas wrote: >> >> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander >> wrote: >> > Actually, for the future, it might be useful to have a "state" column, >> > that holds the idle/in transaction/running status, instead of the >> > tools having to parse the query text to get that information... >> >> +1 for doing it this way. Splitting "current_query" into "query" and >> "state" would be more elegant and easier to use all around. > > I'm all for splitting it out actually. My concern was that I would break > the 'ba-gillion' monitoring tools that already have support for > pg_stat_activity if I dropped a column. What if we had: > 'state' : idle | in transaction | running ( per Robert ) If we're going with breaking compatibility, "waiting" should be a state as well, I think. Actually, it should be even if we're not breaking compatibilty, but just adding a state field. > 'current_query' : the most recent query (either last / currently > running) > That may be a bit tougher to get across to people though (especially in > the case where state=''). > I'll rework this when I don't have trick-or-treaters coming to the front > door :) I think the question is how Ok it is to break compatibility. We could always leave current_query in there and create a new field for last_query *and* introduce a state... And then advertise a change in the future. But that might be too much... If we are doing it, it might be useful to just call it "query", so that it is dead obvious to people that the definition changed.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 7:18 PM, Scott Mead wrote: > > > On Mon, Oct 31, 2011 at 6:13 PM, Robert Haas wrote: > >> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander >> wrote: >> > Actually, for the future, it might be useful to have a "state" column, >> > that holds the idle/in transaction/running status, instead of the >> > tools having to parse the query text to get that information... >> >> +1 for doing it this way. Splitting "current_query" into "query" and >> "state" would be more elegant and easier to use all around. >> > > I'm all for splitting it out actually. My concern was that I would break > the 'ba-gillion' monitoring tools that already have support for > pg_stat_activity if I dropped a column. What if we had: > >'state' : idle | in transaction | running ( per Robert ) > Sorry per Robert and Jaime >'current_query' : the most recent query (either last / currently > running) > >That may be a bit tougher to get across to people though (especially in > the case where state=''). > > I'll rework this when I don't have trick-or-treaters coming to the front > door :) > > -- > Scott Mead > OpenSCG http://www.openscg.com > > >> -- >> Robert Haas >> EnterpriseDB: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > >
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 6:13 PM, Robert Haas wrote: > On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander > wrote: > > Actually, for the future, it might be useful to have a "state" column, > > that holds the idle/in transaction/running status, instead of the > > tools having to parse the query text to get that information... > > +1 for doing it this way. Splitting "current_query" into "query" and > "state" would be more elegant and easier to use all around. > I'm all for splitting it out actually. My concern was that I would break the 'ba-gillion' monitoring tools that already have support for pg_stat_activity if I dropped a column. What if we had: 'state' : idle | in transaction | running ( per Robert ) 'current_query' : the most recent query (either last / currently running) That may be a bit tougher to get across to people though (especially in the case where state=''). I'll rework this when I don't have trick-or-treaters coming to the front door :) -- Scott Mead OpenSCG http://www.openscg.com > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander wrote: > Actually, for the future, it might be useful to have a "state" column, > that holds the idle/in transaction/running status, instead of the > tools having to parse the query text to get that information... +1 for doing it this way. Splitting "current_query" into "query" and "state" would be more elegant and easier to use all around. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 4:45 PM, Magnus Hagander wrote: > > Actually, for the future, it might be useful to have a "state" column, > that holds the idle/in transaction/running status, instead of the > tools having to parse the query text to get that information... > if we are going to create the "state" column let's do it now and change current_query for last_query (so last query can be running, or it was the last before enter in idle state) -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 22:37, Scott Mead wrote: > Hey all, > > So, I'm dealing with a a big ol' java app that has multiple roads on the > way to in transaction. We can reproduce the problem in a test > environment, but the lead dev always asks "can you just tell me the last > query that it ran?" > So I wrote the attached patch, it just turns in transaction into: > " in transaction\n: Previous: ". After seeing > how quickly our dev's fixed the issue once they saw prepared statement XYZ, > I'm thinking that I'd like to be able to have this in prod, and... maybe > (with the frequency of IIT questions posted here) someone else would find > this useful. > > Just wondering what ya'll thought. Any feedback (including a more > efficient approach) is welcome. (Patch against release 9.1.1 tarball). > Thanks! I think the idea in general is pretty useful, but I'd like to extend on it. It would be even better to have a last query executed in the general IDLE state as well, not just idle in transaction. However, doing it the way you did it by adding it to the current query is going to break a lot of tools. I think it's a better idea to create a separate column called "last query" or something like that. Actually, for the future, it might be useful to have a "state" column, that holds the idle/in transaction/running status, instead of the tools having to parse the query text to get that information... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] IDLE in transaction introspection
Hey all, So, I'm dealing with a a big ol' java app that has multiple roads on the way to in transaction. We can reproduce the problem in a test environment, but the lead dev always asks "can you just tell me the last query that it ran?" So I wrote the attached patch, it just turns in transaction into: " in transaction\n: Previous: ". After seeing how quickly our dev's fixed the issue once they saw prepared statement XYZ, I'm thinking that I'd like to be able to have this in prod, and... maybe (with the frequency of IIT questions posted here) someone else would find this useful. Just wondering what ya'll thought. Any feedback (including a more efficient approach) is welcome. (Patch against release 9.1.1 tarball). Thanks! -- Scott Mead OpenSCG idleInTrans.911.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers