On Tue, Dec 6, 2011 at 6:38 AM, Magnus Hagander <mag...@hagander.net> wrote:
> On Sat, Nov 19, 2011 at 02:55, Scott Mead <sco...@openscg.com> wrote: > > > > On Thu, Nov 17, 2011 at 11:58 AM, Scott Mead <sco...@openscg.com> wrote: > >> > >> On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead <sco...@openscg.com> wrote: > >>> > >>> > >>> > >>> On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat <r...@xzilla.net> wrote: > >>>> > >>>> On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith <g...@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." > > 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/ >
