Re: Add session statistics to pg_stat_database

On Fri, 2020-10-02 at 15:10 -0700, Soumyadeep Chakraborty wrote:
> On Tue, Sep 29, 2020 at 2:44 AM Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> wrote:
> > > * Are we trying to capture ONLY client initiated disconnects in
> > > m_aborted (we are not handling other disconnects by not accounting for
> > > EOF..like if psql was killed)? If yes, why?
> >
> > I thought it was interesting to know how many database sessions are
> > ended regularly as opposed to ones that get killed or end by unexpected
> > client death.
>
> It may very well be. It would also be interesting to find out how many
> connections are still open on the database (something we could easily
> glean if we had the number of all disconnects, client-initiated or
> unnatural). Maybe we could have both?
>
> m_sessions_disconnected;
> m_sessions_killed;

We already have "numbackends" in "pg_stat_database", so we know the number
of active connections, right?

> You are absolutely right! PgBackendStatus is not the place for any of
> these fields. We could place them in LocalPgBackendStatus perhaps. But
> I don't feel too strongly about that now, having looked at similar fields
> such as pgStatXactCommit, pgStatXactRollback etc. If we decide to stick
> with the globals, let's isolate and decorate them with a comment such as
> this example from the source:
>
> /*
> * Updated by pgstat_count_buffer_*_time macros
> */
> extern PgStat_Counter pgStatBlockReadTime;
> extern PgStat_Counter pgStatBlockWriteTime;

I have reduced the number of variables with my latest patch; I think
the rewrite based on your review is definitely an improvement.

The comment you quote is from "pgstat.h", and my only global variable
has a comment there.

> > > pgStatSessionDisconnected is not required as it can be determined if a
> > > session has been disconnected by looking at the force argument to
> > > pgstat_report_stat() [unless we would want to distinguish between
> > > client-initiated disconnects, which I am not sure why, as I have
> > > brought up above].
> >
> > But wouldn't that mean that we count *every* end of a session as regular
> > disconnection, even if the backend was killed?
>
> See my comment above about client-initiated and unnatural disconnects.

I decided to leave the functionality as it is; I think it is interesting
information to know if your clients disconnect cleanly or not.

Masahiro Ikeda wrote:
> I think to add the number of login failures is good for security.
> Although we can see the event from log files, it's useful to know the
> overview if the database may be attached or not.

I don't think login failures can be reasonably reported in
"pg_stat_database", since authentication happens before the session is
attached to a database.

What if somebody attempts to connect to a non-existing database?

I agree that this is interesting information, but I don't think it
belongs into this patch.

> By the way, could you rebase the patch since the latest patches
> failed to be applied to the master branch?

Yes, the patch has bit-rotted.

Attached is v3 with improvements.

Yours,
Laurenz Albe