Re: IDLE in transaction introspection

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"

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