Re: Add connection active, idle time to pg_stat_activity - Mailing list pgsql-hackers

Hi,

For the most part this patch looks like it's in pretty good shape to
me, although I am less sold on the desirability of it than it sounds
like most people are. I do think it can be useful, and it seems to
mostly piggyback on existing time measurements, so it should be pretty
cheap. But I'm just not quite sure about the design.

While the number of times that we enter each state does seem to have
some utility, it's also a little weird. If I'm not mistaken, the total
number of times you've become idle (any flavor) has got to add up to
the number of times that you've become active, or it can be 1 less if
you're still active. So that seems a bit redundant: why count four
things where three of them basically sum up to the fourth? Basically,
I think you end up with the times-active counter counting how many
statements you executed, the times-idle counter counting how many of
those statements were not in explicit transactions, and the other two
idle flavors counting how many statements you ran within explicit
transactions, divided between statements that errored out and
statements that didn't. I certainly don't think that's useless, but
there are other choices that seem equally valid; e.g. most obviously,
we could count how many statements committed and and how many
statements aborted, which seems like it might be more generally useful
than this, even though this also has some use. The thing that concerns
me is that I don't think we ever want both. If we add this, we're more
or less saying we're never going to do that, or at least I think so.

I'm also not sure that I like the idea of breaking this out into a
separate pg_stat_session view, as Andres proposed. It might be the
right idea, but if you want to see everything, you'll now need to join
the two views -- and there's no guarantee that you'll get a consistent
read across both of them, so the join might do weird things unless
you're very careful.

If we do decide to keep it separate, I think we should consider
omitting rows entirely when the HAS_PGSTAT_PERMISSIONS check fails. In
the case of pg_stat_activity, we still display some useful information
even to unprivileged users, but here all you get without privileges is
the pid. Maybe there is some point to that if we think we might add
unprivileged columns later or if we think it's important to be
consistent with pg_stat_activity, but I'm sort of inclined to think
it's just clutter.

Cuddled braces are not project style. You will find that code like "}
else if (PGSTAT_IS_IDLE(beentry)){" gats reindented by pgindent; but
it's best to run pgindent before submitting.

I'd probably write the increments as ++ rather than += 1 but I'm not
sure if everyone would agree.

--
Robert Haas
EDB: http://www.enterprisedb.com