Re: [HACKERS] IDLE in transaction introspection
On Thu, Jan 19, 2012 at 15:42, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Jan 19, 2012 at 10:31 PM, Magnus Hagander mag...@hagander.net 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 Wed, Jan 18, 2012 at 16:35, Scott Mead sco...@openscg.com wrote: On Wed, Jan 18, 2012 at 8:27 AM, Magnus Hagander mag...@hagander.net wrote: On Tue, Jan 17, 2012 at 01:43, Scott Mead sco...@openscg.com wrote: On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead sco...@openscg.com wrote: On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith g...@2ndquadrant.com 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 command string 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 Thu, Jan 19, 2012 at 10:31 PM, Magnus Hagander mag...@hagander.net 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 Tue, Jan 17, 2012 at 01:43, Scott Mead sco...@openscg.com wrote: On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead sco...@openscg.com wrote: On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith g...@2ndquadrant.com 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 command string not enabled 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 Wed, Jan 18, 2012 at 8:27 AM, Magnus Hagander mag...@hagander.netwrote: On Tue, Jan 17, 2012 at 01:43, Scott Mead sco...@openscg.com wrote: On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead sco...@openscg.com wrote: On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith g...@2ndquadrant.com 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 command string 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 Sun, Jan 15, 2012 at 4:28 AM, Greg Smith g...@2ndquadrant.com 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
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 Thu, Dec 15, 2011 at 13:18, Greg Smith g...@2ndquadrant.com 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
On Thu, Dec 15, 2011 at 12:10 PM, Magnus Hagander mag...@hagander.netwrote: On Thu, Dec 15, 2011 at 13:18, Greg Smith g...@2ndquadrant.com 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 Wed, Dec 7, 2011 at 17:45, Scott Mead sco...@openscg.com wrote: On Tue, Dec 6, 2011 at 6:38 AM, Magnus Hagander mag...@hagander.net wrote: On Sat, Nov 19, 2011 at 02:55, Scott Mead sco...@openscg.com wrote: On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead sco...@openscg.com wrote: On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead sco...@openscg.com wrote: On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat r...@xzilla.net wrote: On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith g...@2ndquadrant.com 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 literal. NB: should probably be note. I am actually looking for some helping in describing the FASTPATH 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 mag...@hagander.net wrote: On Sat, Nov 19, 2011 at 02:55, Scott Mead sco...@openscg.com wrote: On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead sco...@openscg.com wrote: On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead sco...@openscg.com wrote: On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat r...@xzilla.net wrote: On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith g...@2ndquadrant.com 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 variablelist 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 variablelist 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 literal. NB: should probably be note. I am actually looking for some helping in describing the FASTPATH 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 command string 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 sco...@openscg.com wrote: On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead sco...@openscg.com wrote: On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead sco...@openscg.com wrote: On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat r...@xzilla.net wrote: On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith g...@2ndquadrant.com 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 variablelist 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 literal. NB: should probably be note. * 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 command string 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. 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 sco...@openscg.com wrote: On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat r...@xzilla.net wrote: On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith g...@2ndquadrant.com 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 r...@xzilla.net wrote: On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith g...@2ndquadrant.com 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 Thu, Nov 10, 2011 at 2:49 PM, Tom Lane t...@sss.pgh.pa.us wrote: Bruce Momjian br...@momjian.us 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
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 Tue, Nov 15, 2011 at 12:00 PM, Greg Smith g...@2ndquadrant.com 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 Nov 5, 2011 9:02 AM, Greg Smith g...@2ndquadrant.com wrote: On 11/04/2011 05:01 PM, Tom Lane wrote: Scott Meadsco...@openscg.com 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
Scott Mead wrote: On Wed, Nov 2, 2011 at 4:12 AM, Albe Laurenz laurenz.a...@wien.gv.atwrote: 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 RUNNING, IDLE, IDLE 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 br...@momjian.ushttp://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 br...@momjian.us 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
Tom Lane wrote: Bruce Momjian br...@momjian.us 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 br...@momjian.ushttp://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 br...@momjian.us 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
On 11/04/2011 05:01 PM, Tom Lane wrote: Scott Meadsco...@openscg.com 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 03:27, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Nov 3, 2011 at 3:18 AM, Scott Mead sco...@openscg.com wrote: ISTM that we're all for: creating a new column: state renaming current_query = query State will display RUNNING, IDLE, IDLE 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 Wed, Nov 2, 2011 at 20:18, Scott Mead sco...@openscg.com wrote: State will display RUNNING, IDLE, IDLE 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
Marti Raudsepp ma...@juffo.org 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 Fri, Nov 4, 2011 at 14:42, Tom Lane t...@sss.pgh.pa.us wrote: Marti Raudsepp ma...@juffo.org 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
On Fri, Nov 4, 2011 at 9:48 AM, Magnus Hagander mag...@hagander.net wrote: On Fri, Nov 4, 2011 at 14:42, Tom Lane t...@sss.pgh.pa.us wrote: Marti Raudsepp ma...@juffo.org 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 9:48 AM, Magnus Hagander mag...@hagander.net wrote: On Fri, Nov 4, 2011 at 14:42, Tom Lane t...@sss.pgh.pa.us wrote: Marti Raudsepp ma...@juffo.org 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
Magnus Hagander mag...@hagander.net 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(IDLE in transaction (aborted)); src/backend/tcop/postgres.c: 3797: pgstat_report_activity(IDLE in transaction); src/backend/tcop/postgres.c: 3805: pgstat_report_activity(IDLE); src/backend/tcop/postgres.c: 3925: pgstat_report_activity(FASTPATH 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 10:34 AM, Tom Lane t...@sss.pgh.pa.us 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
On Fri, Nov 4, 2011 at 15:42, Tom Lane t...@sss.pgh.pa.us wrote: Marti Raudsepp ma...@juffo.org 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 12:46 PM, Marti Raudsepp ma...@juffo.org wrote: On Fri, Nov 4, 2011 at 15:42, Tom Lane t...@sss.pgh.pa.us wrote: Marti Raudsepp ma...@juffo.org 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
Robert Haas robertmh...@gmail.com 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 2:28 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com 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
On Fri, Nov 4, 2011 at 2:31 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Nov 4, 2011 at 2:28 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com 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
Scott Mead sco...@openscg.com 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:46 PM, Scott Mead sco...@openscg.com 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
On Fri, Nov 4, 2011 at 3:28 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Nov 4, 2011 at 2:46 PM, Scott Mead sco...@openscg.com 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 10:34 AM, Tom Lane t...@sss.pgh.pa.us wrote: Magnus Hagander mag...@hagander.net 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(IDLE in transaction (aborted)); src/backend/tcop/postgres.c: 3797: pgstat_report_activity(IDLE in transaction); src/backend/tcop/postgres.c: 3805: pgstat_report_activity(IDLE); src/backend/tcop/postgres.c: 3925: pgstat_report_activity(FASTPATH 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 Thu, Nov 3, 2011 at 3:18 AM, Scott Mead sco...@openscg.com wrote: ISTM that we're all for: creating a new column: state renaming current_query = query State will display RUNNING, IDLE, IDLE 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
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 Wed, Nov 2, 2011 at 4:12 AM, Albe Laurenz laurenz.a...@wien.gv.atwrote: 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 RUNNING, IDLE, IDLE 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
On Tue, Nov 1, 2011 at 00:18, Scott Mead sco...@openscg.com wrote: On Mon, Oct 31, 2011 at 6:13 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander mag...@hagander.net 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='IDLE'). 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 Tue, Nov 1, 2011 at 7:38 AM, Magnus Hagander mag...@hagander.net 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 idle and idle in transaction 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 Mon, Oct 31, 2011 at 10:13 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander mag...@hagander.net 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 13:19, Simon Riggs si...@2ndquadrant.com wrote: On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander mag...@hagander.net 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 Tue, Nov 1, 2011 at 12:41 PM, Magnus Hagander mag...@hagander.net wrote: On Tue, Nov 1, 2011 at 13:19, Simon Riggs si...@2ndquadrant.com wrote: On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander mag...@hagander.net 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 8:41 AM, Magnus Hagander mag...@hagander.net wrote: On Tue, Nov 1, 2011 at 13:19, Simon Riggs si...@2ndquadrant.com wrote: On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander mag...@hagander.net 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
Simon Riggs si...@2ndquadrant.com 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 1:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com 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
On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com 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 Mon, Oct 31, 2011 at 23:37, Scott Mead sco...@openscg.com wrote: So I wrote the attached patch, it just turns IDLE in transaction into: IDLE in transaction\n: Previous: last query executed. 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 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
Robert Haas robertmh...@gmail.com writes: On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane t...@sss.pgh.pa.us 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
2011/11/1 Marti Raudsepp ma...@juffo.org: On Mon, Oct 31, 2011 at 23:37, Scott Mead sco...@openscg.com wrote: So I wrote the attached patch, it just turns IDLE in transaction into: IDLE in transaction\n: Previous: last query executed. 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
On Tue, Nov 1, 2011 at 18:11, Scott Mead sco...@openscg.com wrote: On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane t...@sss.pgh.pa.us 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 IDLE 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 idle and idle in transaction 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
On Tue, Nov 1, 2011 at 1:20 PM, Magnus Hagander mag...@hagander.net wrote: On Tue, Nov 1, 2011 at 18:11, Scott Mead sco...@openscg.com wrote: On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane t...@sss.pgh.pa.us 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 IDLE 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. thinking-aloud 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). /thinking-aloud 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 idle and idle in transaction 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:40, Scott Mead sco...@openscg.com wrote: On Tue, Nov 1, 2011 at 1:20 PM, Magnus Hagander mag...@hagander.net wrote: On Tue, Nov 1, 2011 at 18:11, Scott Mead sco...@openscg.com wrote: On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane t...@sss.pgh.pa.us 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 IDLE 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 mag...@hagander.net wrote:
On Tue, Nov 1, 2011 at 18:11, Scott Mead sco...@openscg.com wrote:
On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Robert Haas robertmh...@gmail.com writes:
On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane t...@sss.pgh.pa.us 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 IDLE 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 idle
and idle in transaction 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 IDLE 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 11/01/2011 09:52 AM, Tom Lane wrote: Simon Riggssi...@2ndquadrant.com 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 10:40 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane t...@sss.pgh.pa.us 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 IDLE 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 idle and idle in transaction 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 Tue, Nov 01, 2011 at 10:13:52AM -0400, Andrew Dunstan wrote: On 11/01/2011 09:52 AM, Tom Lane wrote: Simon Riggssi...@2ndquadrant.com 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
[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 IDLE 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 IDLE in transaction into: IDLE in transaction\n: Previous: last query executed. 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
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 22:37, Scott Mead sco...@openscg.com wrote: Hey all, So, I'm dealing with a a big ol' java app that has multiple roads on the way to IDLE 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 IDLE in transaction into: IDLE in transaction\n: Previous: last query executed. 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
Re: [HACKERS] IDLE in transaction introspection
On Mon, Oct 31, 2011 at 4:45 PM, Magnus Hagander mag...@hagander.net 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 5:45 PM, Magnus Hagander mag...@hagander.net 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 6:13 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander mag...@hagander.net 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='IDLE'). 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 7:18 PM, Scott Mead sco...@openscg.com wrote: On Mon, Oct 31, 2011 at 6:13 PM, Robert Haas robertmh...@gmail.comwrote: On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander mag...@hagander.net 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='IDLE'). 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
