Re: IDLE in transaction introspection

On Tue, Dec 6, 2011 at 6:38 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:

> On Sat, Nov 19, 2011 at 02:55, Scott Mead <scottm(at)openscg(dot)com> wrote:
> >
> > On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead <scottm(at)openscg(dot)com> wrote:
> >>
> >> On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead <scottm(at)openscg(dot)com> wrote:
> >>>
> >>>
> >>>
> >>> On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat <rob(at)xzilla(dot)net> wrote:
> >>>>
> >>>> On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <greg(at)2ndquadrant(dot)com>
> >>>> wrote:
> >>>> > On 11/15/2011 09:44 AM, Scott Mead wrote:
> >>>> >>
> >>>> >> Fell off the map last week, here's v2 that:
> >>>> >> * RUNNING => active
> >>>> >> * all states from CAPS to lower case
> >>>> >>
> >>>> >
> >>>> > This looks like what I was hoping someone would add here now. Patch
> >>>> > looks
> >>>> > good, only issue I noticed was a spaces instead of a tab goof where
> >>>> > you set
> >>>> > beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c
> >>>> >
> >>>> > Missing pieces:
> >>>> >
> >>>> > -There is one regression test that uses pg_stat_activity that is
> >>>> > broken now.
> >>>> > -The documentation should list the new field and all of the states
> it
> >>>> > might
> >>>> > include. That's a serious doc update from the minimal info
> available
> >>>> > right
> >>>> > now.
> >
> >
> > V3 Attached:
> >
> > * Updates Documentation
> > -- Adds a separate table to cover each column of pg_stat_activity
>
> I like that a lot - we should've done taht years ago :-)
>
> For consistency, either both datname and usename should refer to their
> respective oid, or none of them.
>

> application_name should probably have a link to the libpq
> documentation for said parameter.
>
> "field is not set" should probably be "field is NULL"
>
>
Thanks for pointing these out.

> "Boolean, if the query is waiting because of a block / lock" reads
> really strange to me. "Boolean indicating if" would be a good start,
> but I'm not sure what you're trying to say with "block / lock" at all?
>

Yeah, me neither. What about:
"Boolean indicating if a query is in a wait state due to a conflict with
another backend."

>
> The way the list of states is built up looks really strange. I think
> it should be built out of <variablelist> like other such places
> instead - but I admit to not having checked what that actually looks
> like in the output.
>

Agreed. I'll look at <variablelist>

>
> The use of single quotes in the descriptions, things like "This is the
> 'state' of" seems wrong. If we should use anything, it should be
> double quotes, but why do we need double quotes around that? And the
> reference to "think time" is just strange, IMO.
>
> Yeah, that's a 'Scottism' (see, I used it again :-). I'll clean that up

> In some other cases, the single quotes should probably be replaced
> with <literal>.
>
> NB: should probably be <note>.
>
>
I am actually looking for some helping in describing the "<FASTPATH>
function call" state. Any takers?

>
>
> > * Adds 'state_change' (timestamp) column
> > -- Tracks when the 'state' column is updated independently
> > of when the 'query' column is updated
>
> Very useful.
>
>
> > * renames procpid => pid
>
> Shouldn't we do this consistently if we do it? It's change in
> pg_stat_get_activity() but not pg_stat_get_wal_senders()? I think we
> either change in both functions, or none of them
>

Agreed

>
> > * Bug Fixes
> > -- If a backend had never before issued a query, another
> > session looking at pg_stat_activity would print <command string
> not
> > enabled>
> > in the query column.
> > -- query_start was being updated on a 'state' change, now only
> updated
> > on query
> > change
> >
> > * Fixed 'rules' regression failure
> > -- Added new columns for pg_stat_activity
>
> Also, I think state=-1 needs a symbolic value. STATE_UNDEFINED or
> something like that.
>

Agreed.

>
> There's also a lot of random trailing whitespace in the patch - "git
> diff" (which you seem to be using already, that's why I mention it)
> will happily alert you about that - they should be removed. Or the
> committer can clean that up of course, but it makes for less work ;)
>
> Thanks for pointing that out.

>
> The code is starting to look really good, but I think the docs
> definitely need another round of work.
>
>
Yeah, I figured they would. Thanks for going through them!

--
Scott Mead
OpenSCG, http://www.openscg.com

--
> Magnus Hagander
> Me: http://www.hagander.net/
> Work: http://www.redpill-linpro.com/
>