Re: Add session statistics to pg_stat_database

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;

>
>
> > *
> > > static TimestampTz pgStatActiveStart = DT_NOBEGIN;
> > > static PgStat_Counter pgStatActiveTime = 0;
> > > static TimestampTz pgStatTransactionIdleStart = DT_NOBEGIN;
> > > static PgStat_Counter pgStatTransactionIdleTime = 0;
> > > static bool pgStatSessionReported = false;
> > > bool pgStatSessionDisconnected = false;
> >
> > I think we can house all of these globals inside PgBackendStatus and can
> > follow the protocol for reading/writing fields in PgBackendStatus.
> > Refer: PGSTAT_{BEGIN|END}_WRITE_ACTIVITY
>
> Are you sure that is the right way to go?
>
> Correct me if I am wrong, but isn't PgBackendStatus for relevant status
> information that other processes can access?
> I'd assume that it is not the correct place to store backend-private data
> that are not relevant to others.
> Besides, if data is written to this structure more often, readers would
> have deal with more contention, which could affect performance.

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;

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

>
> > * PGSTAT_FILE_FORMAT_ID needs to be updated when a stats collector data
> > structure changes and we had a change in PgStat_StatDBEntry.
>
> I think that should be left to the committer.

Fair.

> > * We would need to bump the catalog version since we have made
> > changes to system views. Refer: #define CATALOG_VERSION_NO
>
> Again, I think that's up to the committer.

Fair.

Regards,
Soumyadeep (VMware)