Thread: 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
Attachment
On Mon, Oct 31, 2011 at 22:37, Scott Mead <scottm@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/
On Mon, Oct 31, 2011 at 4:45 PM, Magnus Hagander <magnus@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
On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander <magnus@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
On Mon, Oct 31, 2011 at 6:13 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander <magnus@hagander.net> wrote:> Actually, for the future, it might be useful to have a "state" column,+1 for doing it this way. Splitting "current_query" into "query" and
> that holds the idle/in transaction/running status, instead of the
> tools having to parse the query text to get that information...
"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
On Mon, Oct 31, 2011 at 7:18 PM, Scott Mead <scottm@openscg.com> wrote:
On Mon, Oct 31, 2011 at 6:13 PM, Robert Haas <robertmhaas@gmail.com> wrote:On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander <magnus@hagander.net> wrote:> Actually, for the future, it might be useful to have a "state" column,+1 for doing it this way. Splitting "current_query" into "query" and
> that holds the idle/in transaction/running status, instead of the
> tools having to parse the query text to get that information...
"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 MeadOpenSCG http://www.openscg.com
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Nov 1, 2011 at 00:18, Scott Mead <scottm@openscg.com> wrote: > > > On Mon, Oct 31, 2011 at 6:13 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> >> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander <magnus@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/
On Tue, Nov 1, 2011 at 7:38 AM, Magnus Hagander <magnus@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
On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander <magnus@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
On Tue, Nov 1, 2011 at 13:19, Simon Riggs <simon@2ndquadrant.com> wrote: > On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander <magnus@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/
On Tue, Nov 1, 2011 at 12:41 PM, Magnus Hagander <magnus@hagander.net> wrote: > On Tue, Nov 1, 2011 at 13:19, Simon Riggs <simon@2ndquadrant.com> wrote: >> On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander <magnus@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
On Tue, Nov 1, 2011 at 8:41 AM, Magnus Hagander <magnus@hagander.net> wrote: > On Tue, Nov 1, 2011 at 13:19, Simon Riggs <simon@2ndquadrant.com> wrote: >> On Mon, Oct 31, 2011 at 10:13 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Mon, Oct 31, 2011 at 5:45 PM, Magnus Hagander <magnus@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
Simon Riggs <simon@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
On Tue, Nov 1, 2011 at 1:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@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
On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@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
On Mon, Oct 31, 2011 at 23:37, Scott Mead <scottm@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
On 11/01/2011 09:52 AM, Tom Lane wrote: > Simon Riggs<simon@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
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
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane <tgl@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
2011/11/1 Marti Raudsepp <marti@juffo.org>: > On Mon, Oct 31, 2011 at 23:37, Scott Mead <scottm@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
On Tue, Nov 1, 2011 at 18:11, Scott Mead <scottm@openscg.com> wrote: > > On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> Robert Haas <robertmhaas@gmail.com> writes: >> > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane <tgl@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/
On Tue, Nov 1, 2011 at 1:20 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Tue, Nov 1, 2011 at 18:11, Scott Mead <scottm@openscg.com> wrote:Well, by appending it in that field, you require the end
>
> On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Robert Haas <robertmhaas@gmail.com> writes:
>> > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane <tgl@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.
user/monitoring app to parse out the string anyway, so you're not
exactly making life easier on the consumer of the information..This would be great, but as you say, it's a different project.
>> 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".
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
Really? I'd assume every single monitoring tool that graphs how many
>> 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.
active connections you have (vs idle vs idle in transaction) would do
just this.
On Tue, Nov 1, 2011 at 18:40, Scott Mead <scottm@openscg.com> wrote: > > > On Tue, Nov 1, 2011 at 1:20 PM, Magnus Hagander <magnus@hagander.net> wrote: >> >> On Tue, Nov 1, 2011 at 18:11, Scott Mead <scottm@openscg.com> wrote: >> > >> > On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> >> >> Robert Haas <robertmhaas@gmail.com> writes: >> >> > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane <tgl@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/
On Tue, Nov 1, 2011 at 1:20 PM, Magnus Hagander <magnus@hagander.net> wrote: > On Tue, Nov 1, 2011 at 18:11, Scott Mead <scottm@openscg.com> wrote: >> >> On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> >>> Robert Haas <robertmhaas@gmail.com> writes: >>> > On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane <tgl@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
On Tue, Nov 1, 2011 at 10:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Nov 1, 2011 at 9:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:>> That would cost twice as much shared memory for query strings, and twiceI understood the proposal to be "store the previous query in addition
>> 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.
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
On Tue, Nov 01, 2011 at 10:13:52AM -0400, Andrew Dunstan wrote: > > > On 11/01/2011 09:52 AM, Tom Lane wrote: > >Simon Riggs<simon@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. reedstrm@rice.edu Systems Engineer & Admin, Research Scientist phone: 713-348-6166 Connexions http://cnx.org fax: 713-348-3665 Rice University MS-375, Houston, TX 77005 GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE
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
On Wed, Nov 2, 2011 at 4:12 AM, Albe Laurenz <laurenz.albe@wien.gv.at> wrote:
Andrew Dunstan wrote:
> On 11/01/2011 09:52 AM, Tom Lane wrote:>> I'm for just redefining the query field as "current or last+1 for renaming, +1 for a state column.
>> 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.
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
Attachment
On Thu, Nov 3, 2011 at 3:18 AM, Scott Mead <scottm@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
On Fri, Nov 4, 2011 at 03:27, Fujii Masao <masao.fujii@gmail.com> wrote: > On Thu, Nov 3, 2011 at 3:18 AM, Scott Mead <scottm@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/
On Wed, Nov 2, 2011 at 20:18, Scott Mead <scottm@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
Marti Raudsepp <marti@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
On Fri, Nov 4, 2011 at 14:42, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marti Raudsepp <marti@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/
On Fri, Nov 4, 2011 at 9:48 AM, Magnus Hagander <magnus@hagander.net> wrote: > On Fri, Nov 4, 2011 at 14:42, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Marti Raudsepp <marti@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
On Fri, Nov 4, 2011 at 9:48 AM, Magnus Hagander <magnus@hagander.net> wrote:
On Fri, Nov 4, 2011 at 14:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:I guess with the changes that showed different thing like fastpath,
> Marti Raudsepp <marti@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.
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'.
Yeha, that seems inconsistent. Using a single character might work -
>> 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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Magnus Hagander <magnus@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
On Fri, Nov 4, 2011 at 10:34 AM, Tom Lane <tgl@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
On Fri, Nov 4, 2011 at 15:42, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marti Raudsepp <marti@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
On Fri, Nov 4, 2011 at 12:46 PM, Marti Raudsepp <marti@juffo.org> wrote: > On Fri, Nov 4, 2011 at 15:42, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Marti Raudsepp <marti@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
Robert Haas <robertmhaas@gmail.com> writes: > Maybe there's a better term than "running", like "in progress" or > something of that sort. "active"? regards, tom lane
On Fri, Nov 4, 2011 at 2:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@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
On Fri, Nov 4, 2011 at 2:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Nov 4, 2011 at 2:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:+1.
> Robert Haas <robertmhaas@gmail.com> writes:
>> Maybe there's a better term than "running", like "in progress" or
>> something of that sort.
>
> "active"?
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
Scott Mead <scottm@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
On Fri, Nov 4, 2011 at 2:46 PM, Scott Mead <scottm@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
On Fri, Nov 4, 2011 at 3:28 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Nov 4, 2011 at 2:46 PM, Scott Mead <scottm@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
On Fri, Nov 4, 2011 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@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
On 11/04/2011 05:01 PM, Tom Lane wrote: > Scott Mead<scottm@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 US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
<p><br /> On Nov 5, 2011 9:02 AM, "Greg Smith" <<a href="mailto:greg@2ndquadrant.com">greg@2ndquadrant.com</a>> wrote:<br/> ><br /> > On 11/04/2011 05:01 PM, Tom Lane wrote:<br /> >><br /> >> Scott Mead<<a href="mailto:scottm@openscg.com">scottm@openscg.com</a>> writes:<br /> >> <br /> >>><br /> >>> I leave the waiting flag in place for posterity. With this in mind, is<br /> >>> the consensus:<br/> >>> RUNNING<br /> >>> or<br /> >>> ACTIVE<br /> >>> <br/> >><br /> >> Personally, I'd go for lower case.<br /> >> <br /> ><br /> ><br /> > I wasthinking it would be nice if this state looked like the WAL sender state values in pg_stat_replication, which are alllower case. For comparison those states are:<br /> ><br /> > startup<br /> > backup<br /> > catchup<br />> streaming<p> +1, it'll be easier to query against. <br /> ><br /> > -- <br /> > Greg Smith 2ndQuadrantUS greg@2ndQuadrant.com Baltimore, MD<br /> > PostgreSQL Training, Services, and 24x7 Support <a href="http://www.2ndQuadrant.us">www.2ndQuadrant.us</a><br/> ><br /> ><br /> ><br /> > -- <br /> > Sent viapgsql-hackers mailing list (<a href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br /> >To make changes to your subscription:<br /> > <a href="http://www.postgresql.org/mailpref/pgsql-hackers">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/>
Scott Mead wrote: > On Wed, Nov 2, 2011 at 4:12 AM, Albe Laurenz <laurenz.albe@wien.gv.at>wrote: > > > Andrew Dunstan wrote: > > > On 11/01/2011 09:52 AM, Tom Lane wrote: > > >> I'm for just redefining the query field as "current or last > > >> query". > > > > > > +1 > > > > > >> I could go either way on whether to rename it. > > > > > > Rename it please. "current_query" will just be wrong. I'd be inclined > > > just to call it "query" or "query_string" and leave it to the docs to > > > define the exact semantics. > > > > +1 for renaming, +1 for a state column. > > I think it is overkill to keep a query history beyond that -- if you > > want that, > > you can resort to the log files. > > > > > ISTM that we're all for: > > creating a new column: state > renaming current_query => query > > State will display <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/fin transaction: t/f or maybe instead of 'active': idle: t/fin 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@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
Tom Lane wrote: > Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@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
On Thu, Nov 10, 2011 at 2:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Bruce Momjian <bruce@momjian.us> writes:No, we're not using magic strings, we're using an enum --- maybe not an
> 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.
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
Attachment
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 US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <greg@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
On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat <rob@xzilla.net> wrote:
+1On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <greg@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 for me as well.
Anybody else?
lmk if you need help, we'll be doing this with some of our tools /
> 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.
>
projects anyway, it probably wouldn't hurt to coordinate.--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead <scottm@openscg.com> wrote:
On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat <rob@xzilla.net> wrote:On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <greg@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
+1>
> 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 for me as well.Anybody else?lmk if you need help, we'll be doing this with some of our tools /
> 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.
>
projects anyway, it probably wouldn't hurt to coordinate.--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead <scottm@openscg.com> wrote:
On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead <scottm@openscg.com> wrote:On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat <rob@xzilla.net> wrote:On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <greg@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
* Adds 'state_change' (timestamp) column
-- Tracks when the 'state' column is updated independently
of when the 'query' column is updated
* renames procpid => pid
* 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
--
Scott Mead
OpenSCG, http://www.openscg.com
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 MeadOpenSCG, http://www.openscg.com+1>
> 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 for me as well.Anybody else?lmk if you need help, we'll be doing this with some of our tools /
> 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.
>
projects anyway, it probably wouldn't hurt to coordinate.--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Sat, Nov 19, 2011 at 02:55, Scott Mead <scottm@openscg.com> wrote: > > On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead <scottm@openscg.com> wrote: >> >> On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead <scottm@openscg.com> wrote: >>> >>> >>> >>> On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat <rob@xzilla.net> wrote: >>>> >>>> On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <greg@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/
On Tue, Dec 6, 2011 at 6:38 AM, Magnus Hagander <magnus@hagander.net> wrote:
I like that a lot - we should've done taht years ago :-)On Sat, Nov 19, 2011 at 02:55, Scott Mead <scottm@openscg.com> wrote:
>
> On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead <scottm@openscg.com> wrote:
>>
>> On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead <scottm@openscg.com> wrote:
>>>
>>>
>>>
>>> On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat <rob@xzilla.net> wrote:
>>>>
>>>> On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <greg@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
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?
Very useful.
> * Adds 'state_change' (timestamp) column
> -- Tracks when the 'state' column is updated independently
> of when the 'query' column is updatedShouldn't we do this consistently if we do it? It's change in
> * renames procpid => pid
pg_stat_get_activity() but not pg_stat_get_wal_senders()? I think we
either change in both functions, or none of them
Agreed
Also, I think state=-1 needs a symbolic value. STATE_UNDEFINED or
> * 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
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
On Wed, Dec 7, 2011 at 17:45, Scott Mead <scottm@openscg.com> wrote: > > On Tue, Dec 6, 2011 at 6:38 AM, Magnus Hagander <magnus@hagander.net> wrote: >> >> On Sat, Nov 19, 2011 at 02:55, Scott Mead <scottm@openscg.com> wrote: >> > >> > On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead <scottm@openscg.com> wrote: >> >> >> >> On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead <scottm@openscg.com> wrote: >> >>> >> >>> On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat <rob@xzilla.net> wrote: >> >>>> >> >>>> On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <greg@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/
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 US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
On Thu, Dec 15, 2011 at 13:18, Greg Smith <greg@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/
On Thu, Dec 15, 2011 at 12:10 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Thu, Dec 15, 2011 at 13:18, Greg Smith <greg@2ndquadrant.com> wrote:My hope was to get a quick update from Scott and then apply it. But
> 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.
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
On Thu, Dec 15, 2011 at 2:23 PM, Scott Mead <scottm@openscg.com> wrote:
On Thu, Dec 15, 2011 at 12:10 PM, Magnus Hagander <magnus@hagander.net> wrote:On Thu, Dec 15, 2011 at 13:18, Greg Smith <greg@2ndquadrant.com> wrote:My hope was to get a quick update from Scott and then apply it. But
> 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.
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.
Pretty delayed, but please find the attached patch that addresses all the issues discussed.
--Scott
OpenSCG http://www.openscg.com
Thanks guys, I should have something by this weekend. Sorry for the delay.--Scott
Attachment
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 US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com
On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith <greg@2ndquadrant.com> wrote:
On 01/12/2012 11:57 AM, Scott Mead wrote: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.Pretty delayed, but please find the attached patch that addresses all the issues discussed.
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
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com
--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead <scottm@openscg.com> wrote:
On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith <greg@2ndquadrant.com> wrote:On 01/12/2012 11:57 AM, Scott Mead wrote: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.Pretty delayed, but please find the attached patch that addresses all the issues discussed.
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.
--Scott
Okay, I'll fix that and the rules.out regression that I missed--ScottOpenSCG, http://www.openscg.comPostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com
--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
Attachment
On Tue, Jan 17, 2012 at 01:43, Scott Mead <scottm@openscg.com> wrote: > > > On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead <scottm@openscg.com> wrote: >> >> >> On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith <greg@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/
On Wed, Jan 18, 2012 at 8:27 AM, Magnus Hagander <magnus@hagander.net> wrote:
On Tue, Jan 17, 2012 at 01:43, Scott Mead <scottm@openscg.com> wrote:I'm reviewing this again, and have changed a few things around. I came
>
>
> On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead <scottm@openscg.com> wrote:
>>
>>
>> On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith <greg@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.
>
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
On Wed, Jan 18, 2012 at 16:35, Scott Mead <scottm@openscg.com> wrote: > > On Wed, Jan 18, 2012 at 8:27 AM, Magnus Hagander <magnus@hagander.net> > wrote: >> >> On Tue, Jan 17, 2012 at 01:43, Scott Mead <scottm@openscg.com> wrote: >> > >> > >> > On Mon, Jan 16, 2012 at 6:10 PM, Scott Mead <scottm@openscg.com> wrote: >> >> >> >> >> >> On Sun, Jan 15, 2012 at 4:28 AM, Greg Smith <greg@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/
On Thu, Jan 19, 2012 at 10:31 PM, Magnus Hagander <magnus@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
Attachment
On Thu, Jan 19, 2012 at 15:42, Fujii Masao <masao.fujii@gmail.com> wrote: > On Thu, Jan 19, 2012 at 10:31 PM, Magnus Hagander <magnus@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/