Re: Add session statistics to pg_stat_database

On Sat, Dec 5, 2020 at 1:04 PM Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
wrote:

> On Fri, 2020-12-04 at 16:55 +0100, I wrote:
> > > > Basically, that would change pgStatSessionDisconnectedNormally into
> instead being an
> > > > enum of reasons, which could be normal disconnect, abnormal
> disconnect and admin.
> > > > And we'd track all those three as separate numbers in the stats
> file, meaning we could
> > > > then calculate the crash by subtracting all three from the total
> number of sessions?
> > >
> > > I think at least "closed by admin" might be interesting; I'll have a
> look.
> > > I don't think we have to specifically count "closed by normal
> disconnect", because
> > > that should be the rule and could be more or less deduced from the
> other numbers
> > > (with the uncertainty mentioned above).
> >
> > I am considering the cases
> >
> > 1) client just went away (currently "aborted")
> > 2) death by FATAL error
> > 3) killed by the administrator (or shutdown)
>
> I think I figured it out. Here is a patch along these lines.
>
> I named the three counters "sessions_client_eof", "sessions_fatal" and
> "sessions_killed", but I am not wedded to these bike shed colors.
>

Maybe we should, in honor of the bikeshed, we should call them
sessions_blue, sessions_green etc :)

In true bikeshedding mode, I'm not entirely happy with sessions_client_eof,
but I'm also not sure I have a better suggestion. Maybe just
"sessions_lost" or "sessions_connlost", which is basically the terminology
that the documentation uses? Maybe it's just me, but I don't really like
the eof terminology here.

What do you think about that? Or does somebody else have an opinion here?

Aside from that bikeshedding, I think this version looks very good!

In today's dept of small things I noticed:

+ if (disconnect)
+ msg.m_disconnect = pgStatSessionEndCause;

in the non-disconnect state, that variable is left uninitialized, isn't
it? It does end up getting ignored later, but to be more future proof the
enum should probably have a value specifically for "not disconnected yet"?

+ case DISCONNECT_CLIENT_EOF:
+ ++(dbentry->n_sessions_client_eof);
+ break;

The normal syntax we'd use for that would be
dbentry->n_sessions_client_eof++;

+ typedef enum sessionEndType {

To be consistent with the other enums in the same place, seems this should
be SessionEndType.

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