Обсуждение: IDLE in transaction introspection

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 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 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 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 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

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

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 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 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 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 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 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 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

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/