Re: Add session statistics to pg_stat_database

From: Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
To: Soumyadeep Chakraborty <soumyadeep2007(at)gmail(dot)com>
Cc: Ahsan Hadi <ahsan(dot)hadi(at)gmail(dot)com>, ikedamsh(at)oss(dot)nttdata(dot)com, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Add session statistics to pg_stat_database
Date: 2020-10-13 11:44:41
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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
> > > 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.

Laurenz Albe

Attachment Content-Type Size
0001-Add-session-statistics-to-pg_stat_database.v3.patch text/x-patch 16.8 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Banck 2020-10-13 11:53:49 Re: Two fsync related performance issues?
Previous Message Heikki Linnakangas 2020-10-13 10:12:59 Re: partition routing layering in nodeModifyTable.c