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

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,
that holds the idle/in transaction/running status, instead of the
tools having to parse the query text to get that information...

+1 for doing it this way. Splitting "current_query" into "query" and
"state" would be more elegant and easier to use all around.

I'm all for splitting it out actually. My concern was that I would break
the 'ba-gillion' monitoring tools that already have support for
pg_stat_activity if I dropped a column. What if we had:

'state' : idle | in transaction | running ( per Robert )

Sorry per Robert and Jaime

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

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.

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