Re: Add session statistics to pg_stat_database

[Magnus Hagander]

On Mon, Jan 18, 2021 at 5:11 PM Laurenz Albe  wrote:
>
> On Sun, 2021-01-17 at 14:07 +0100, Magnus Hagander wrote:
> > I have applied this version, with some minor changes:
> >
> > * I renamed the n__time members in the struct to just
> > total__time. The n_ indicates "number of" and is thus wrong for
> > time parameters.
>
> Right.
>
> > * Some very minor wording changes.
> >
> > * catversion bump (for once I didn't forget it!)
>
> Thank you!
>
> You included the catversion bump, but shouldn't PGSTAT_FILE_FORMAT_ID
> in "include/pgstat.h" be updated as well?

Yup, you are absolutely correct. Will fix.

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

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Sun, 2021-01-17 at 14:07 +0100, Magnus Hagander wrote:
> I have applied this version, with some minor changes:
> 
> * I renamed the n__time members in the struct to just
> total__time. The n_ indicates "number of" and is thus wrong for
> time parameters.

Right.

> * Some very minor wording changes.
>
> * catversion bump (for once I didn't forget it!)

Thank you!

You included the catversion bump, but shouldn't PGSTAT_FILE_FORMAT_ID
in "include/pgstat.h" be updated as well?

Yours,
Laurenz Albe

Re: Add session statistics to pg_stat_database

[Magnus Hagander]

On Fri, Jan 8, 2021 at 10:34 AM Laurenz Albe  wrote:
>
> On Fri, 2021-01-08 at 12:00 +0900, Masahiro Ikeda wrote:
> > 2. monitoring.sgml
> >
> > > > IIUC, "active_time" includes the time executes a fast-path function
> > > > and
> > > > "idle in transaction" includes "idle in transaction(aborted)" time.
> > > > Why don't you reference pg_stat_activity's "state" column and
> > > > "active_time" is the total time when the state is "active" and "fast
> > > > path"?
> > > > "idle in transaction" is as same too.
> > >
> > > Good idea; I have expanded the documentation like that.
> >
> > BTW, is there any reason to merge the above statistics?
> > IIUC, to separate statistics' cons is that two columns increase, and
> > there is no performance penalty. So, I wonder that there is a way to
> > separate them
> > corresponding to the state column of pg_stat_activity.
>
> Sure, that could be done.
>
> I decided to do it like this because I thought that few people would
> be interested in "time spend doing fast-path function calls"; my guess
> was that the more interesting value is "time where the database was
> busy calculating results".
>
> I tried to keep the balance between providing reasonable detail
> while not creating more additional columns to "pg_stat_database"
> than necessary.
>
> This is of course a matter of taste, and it is good to hear different
> opinions.  If more people share your opinion, I'll change the code.
>
> > There are some following codes in pgstatfuncs.c.
> > int64 result = 0.0;
> >
> > But, I think the following is better.
> > int64 result = 0;
>
> You are right.  That was a silly copy-and-paste error.  Fixed.
>
> > Although now pg_stat_get_db_session_time is initialize "result" to zero
> > when it is declared,
> > another pg_stat_XXX function didn't initialize. Is it better to change
> > it?
>
> I looked at other similar functions, and the ones I saw returned
> NULL if there were no data.  In that case, it makes sense to write
>
> char *result;
>
> if ((result = get_stats_data()) == NULL)
> PG_RETURN_NULL();
>
> PG_RETURN_TEXT_P(cstring_to_text(result));
>
> But I want to return 0 for the session time if there are no data yet,
> so I think initializing the result to 0 in the declaration makes sense.
>
> There are some functions that do it like this:
>
> int32   result;
>
> result = 0;
> for (...)
> {
> if (...)
> result++;
> }
>
> PG_RETURN_INT32(result);
>
> Again, it is a matter of taste, and I didn't detect a clear pattern
> in the existing code that I feel I should follow in this question.
>
> Version 12 of the patch is attached.

Thanks! I have applied this version, with some minor changes:

* I renamed the n__time members in the struct to just
total__time. The n_ indicates "number of" and is thus wrong for
time parameters.

* Some very minor wording changes.

* catversion bump (for once I didn't forget it!)


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

Re: Add session statistics to pg_stat_database

[Masahiro Ikeda]

On 2021-01-08 18:34, Laurenz Albe wrote:

On Fri, 2021-01-08 at 12:00 +0900, Masahiro Ikeda wrote:

2. monitoring.sgml

> > IIUC, "active_time" includes the time executes a fast-path function
> > and
> > "idle in transaction" includes "idle in transaction(aborted)" time.
> > Why don't you reference pg_stat_activity's "state" column and
> > "active_time" is the total time when the state is "active" and "fast
> > path"?
> > "idle in transaction" is as same too.
>
> Good idea; I have expanded the documentation like that.

BTW, is there any reason to merge the above statistics?
IIUC, to separate statistics' cons is that two columns increase, and
there is no performance penalty. So, I wonder that there is a way to
separate them
corresponding to the state column of pg_stat_activity.


Sure, that could be done.

I decided to do it like this because I thought that few people would
be interested in "time spend doing fast-path function calls"; my guess
was that the more interesting value is "time where the database was
busy calculating results".

I tried to keep the balance between providing reasonable detail
while not creating more additional columns to "pg_stat_database"
than necessary.

This is of course a matter of taste, and it is good to hear different
opinions.  If more people share your opinion, I'll change the code.


OK, I understood.
I don't have any strong opinions to add them.


There are some following codes in pgstatfuncs.c.
int64 result = 0.0;

But, I think the following is better.
int64 result = 0;


You are right.  That was a silly copy-and-paste error.  Fixed.


Thanks.

Although now pg_stat_get_db_session_time is initialize "result" to 
zero

when it is declared,
another pg_stat_XXX function didn't initialize. Is it better to change
it?


I looked at other similar functions, and the ones I saw returned
NULL if there were no data.  In that case, it makes sense to write

char *result;

if ((result = get_stats_data()) == NULL)
PG_RETURN_NULL();

PG_RETURN_TEXT_P(cstring_to_text(result));

But I want to return 0 for the session time if there are no data yet,
so I think initializing the result to 0 in the declaration makes sense.

There are some functions that do it like this:

int32   result;

result = 0;
for (...)
{
if (...)
result++;
}

PG_RETURN_INT32(result);

Again, it is a matter of taste, and I didn't detect a clear pattern
in the existing code that I feel I should follow in this question.


Thanks, I understood.

I checked my comments are fixed.
This patch looks good to me for monitoring session statistics.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Fri, 2021-01-08 at 12:00 +0900, Masahiro Ikeda wrote:
> 2. monitoring.sgml
> 
> > > IIUC, "active_time" includes the time executes a fast-path function 
> > > and
> > > "idle in transaction" includes "idle in transaction(aborted)" time.
> > > Why don't you reference pg_stat_activity's "state" column and
> > > "active_time" is the total time when the state is "active" and "fast
> > > path"?
> > > "idle in transaction" is as same too.
> >
> > Good idea; I have expanded the documentation like that.
> 
> BTW, is there any reason to merge the above statistics?
> IIUC, to separate statistics' cons is that two columns increase, and
> there is no performance penalty. So, I wonder that there is a way to 
> separate them
> corresponding to the state column of pg_stat_activity.

Sure, that could be done.

I decided to do it like this because I thought that few people would
be interested in "time spend doing fast-path function calls"; my guess
was that the more interesting value is "time where the database was
busy calculating results".

I tried to keep the balance between providing reasonable detail
while not creating more additional columns to "pg_stat_database"
than necessary.

This is of course a matter of taste, and it is good to hear different
opinions.  If more people share your opinion, I'll change the code.

> There are some following codes in pgstatfuncs.c.
> int64 result = 0.0;
> 
> But, I think the following is better.
> int64 result = 0;

You are right.  That was a silly copy-and-paste error.  Fixed.

> Although now pg_stat_get_db_session_time is initialize "result" to zero 
> when it is declared,
> another pg_stat_XXX function didn't initialize. Is it better to change 
> it?

I looked at other similar functions, and the ones I saw returned
NULL if there were no data.  In that case, it makes sense to write

char *result;

if ((result = get_stats_data()) == NULL)
PG_RETURN_NULL();

PG_RETURN_TEXT_P(cstring_to_text(result));

But I want to return 0 for the session time if there are no data yet,
so I think initializing the result to 0 in the declaration makes sense.

There are some functions that do it like this:

int32   result;

result = 0;
for (...)
{
if (...)
result++;
}

PG_RETURN_INT32(result);

Again, it is a matter of taste, and I didn't detect a clear pattern
in the existing code that I feel I should follow in this question.

Version 12 of the patch is attached.

Yours,
Laurenz Albe
From 324847353f5d9e5b2899dd93d43fb345df1dcdb8 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Thu, 7 Jan 2021 16:33:45 +0100
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- total number of connections
- number of sessions that ended by loss of network connection,
  fatal errors and operator intervention
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Discussion: https://postgr.es/m/b07e1f9953701b90c66ed368656f2aef40cac4fb.ca...@cybertec.at
Reviewed-By: Soumyadeep Chakraborty, Justin Pryzby, Masahiro Ikeda, Magnus Hagander

(This requires a catversion bump, as well as an update to PGSTAT_FILE_FORMAT_ID)
---
 doc/src/sgml/monitoring.sgml |  77 +++
 src/backend/catalog/system_views.sql |   7 ++
 src/backend/postmaster/pgstat.c  | 134 ++-
 src/backend/tcop/postgres.c  |  11 ++-
 src/backend/utils/adt/pgstatfuncs.c  |  94 +++
 src/backend/utils/error/elog.c   |   8 ++
 src/include/catalog/pg_proc.dat  |  32 +++
 src/include/pgstat.h |  39 
 src/test/regress/expected/rules.out  |   7 ++
 9 files changed, 404 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 43fe8ae383..59622173da 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3737,6 +3737,83 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent by database sessions in this database, in milliseconds
+   (note that statistics are only updated when the state of a session
+   changes, so if sessions have been idle for a long time, this idle time
+   won't be included)
+  
+ 
+
+ 
+

Re: Add session statistics to pg_stat_database

[Masahiro Ikeda]

On 2021-01-08 00:47, Laurenz Albe wrote:

On Fri, 2020-12-25 at 20:28 +0900, Masahiro Ikeda wrote:

As a user, I want this feature to know whether
clients' session activities are as expected.

I have some comments about the patch.


Thanks you for the thorough review!


Thanks for updating the patch!


1. pg_proc.dat

The unit of "session time" and so on says "in seconds".
But, is "in milliseconds" right?


You are right.  Fixed.


2. monitoring.sgml

IIUC, "active_time" includes the time executes a fast-path function 
and

"idle in transaction" includes "idle in transaction(aborted)" time.

Why don't you reference pg_stat_activity's "state" column and
"active_time" is the total time when the state is "active" and "fast
path"?
"idle in transaction" is as same too.


Good idea; I have expanded the documentation like that.


BTW, is there any reason to merge the above statistics?
IIUC, to separate statistics' cons is that two columns increase, and
there is no performance penalty. So, I wonder that there is a way to 
separate them

corresponding to the state column of pg_stat_activity.


3. pgstat.h

The comment of PgStat_MsgConn says "Sent by pgstat_connection".
I thought "pgstat_connection" is a function, but it doesn't exist.

Is "Sent by the backend" right?


The function was renamed and is now called "pgstat_send_connstats".

But you are right, I might as well match the surrounding code and
write "Sent by the backend".


Although this is a trivial thing, the following row has too many tabs.

Other structs have only one space.

// }Pgstat_MsgConn;


Yes, I messed that up during the pgindent run.  Fixed.

Patch version 11 is attached.


There are some following codes in pgstatfuncs.c.
int64 result = 0.0;

But, I think the following is better.
int64 result = 0;

Although now pg_stat_get_db_session_time is initialize "result" to zero 
when it is declared,
another pg_stat_XXX function didn't initialize. Is it better to change 
it?


Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Fri, 2020-12-25 at 20:28 +0900, Masahiro Ikeda wrote:
> As a user, I want this feature to know whether
> clients' session activities are as expected.
> 
> I have some comments about the patch.

Thanks you for the thorough review!

> 1. pg_proc.dat
> 
> The unit of "session time" and so on says "in seconds".
> But, is "in milliseconds" right?

You are right.  Fixed.

> 2. monitoring.sgml
> 
> IIUC, "active_time" includes the time executes a fast-path function and
> "idle in transaction" includes "idle in transaction(aborted)" time.
>
> Why don't you reference pg_stat_activity's "state" column and
> "active_time" is the total time when the state is "active" and "fast 
> path"?
> "idle in transaction" is as same too.

Good idea; I have expanded the documentation like that.

> 3. pgstat.h
> 
> The comment of PgStat_MsgConn says "Sent by pgstat_connection".
> I thought "pgstat_connection" is a function, but it doesn't exist.
>
> Is "Sent by the backend" right?

The function was renamed and is now called "pgstat_send_connstats".

But you are right, I might as well match the surrounding code and
write "Sent by the backend".

> Although this is a trivial thing, the following row has too many tabs.
> 
> Other structs have only one space.
> 
> // }Pgstat_MsgConn;

Yes, I messed that up during the pgindent run.  Fixed.

Patch version 11 is attached.

Yours,
Laurenz Albe
From 324847353f5d9e5b2899dd93d43fb345df1dcdb8 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Thu, 7 Jan 2021 16:33:45 +0100
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- total number of connections
- number of sessions that ended by loss of network connection,
  fatal errors and operator intervention
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Discussion: https://postgr.es/m/b07e1f9953701b90c66ed368656f2aef40cac4fb.ca...@cybertec.at
Reviewed-By: Soumyadeep Chakraborty, Justin Pryzby, Masahiro Ikeda, Magnus Hagander

(This requires a catversion bump, as well as an update to PGSTAT_FILE_FORMAT_ID)
---
 doc/src/sgml/monitoring.sgml |  77 +++
 src/backend/catalog/system_views.sql |   7 ++
 src/backend/postmaster/pgstat.c  | 134 ++-
 src/backend/tcop/postgres.c  |  11 ++-
 src/backend/utils/adt/pgstatfuncs.c  |  94 +++
 src/backend/utils/error/elog.c   |   8 ++
 src/include/catalog/pg_proc.dat  |  32 +++
 src/include/pgstat.h |  39 
 src/test/regress/expected/rules.out  |   7 ++
 9 files changed, 404 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 43fe8ae383..59622173da 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3737,6 +3737,83 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent by database sessions in this database, in milliseconds
+   (note that statistics are only updated when the state of a session
+   changes, so if sessions have been idle for a long time, this idle time
+   won't be included)
+  
+ 
+
+ 
+  
+   active_time double precision
+  
+  
+   Time spent executing SQL statements in this database, in milliseconds
+   (this corresponds to the states active and
+   fastpath function call in
+   
+   pg_stat_activity)
+  
+ 
+
+ 
+  
+   idle_in_transaction_time double precision
+  
+  
+   Time spent idling while in a transaction in this database, in milliseconds
+   (this corresponds to the states idle in transaction and
+   idle in transaction (aborted) in
+   
+   pg_stat_activity)
+  
+ 
+
+ 
+  
+   sessions bigint
+  
+  
+   Total number of sessions established to this database
+  
+ 
+
+ 
+  
+   sessions_abandoned bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   because connection to the client was lost
+  
+ 
+
+ 
+  
+   sessions_fatal bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   by fatal errors
+  
+ 
+
+ 
+  
+   sessions_killed bigint
+  
+  
+

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Fri, 2020-12-25 at 20:28 +0900, Masahiro Ikeda wrote:
> As a user, I want this feature to know whether
> clients' session activities are as expected.
> 
> I have some comments about the patch.
> 
> 1. pg_proc.dat
> 
> The unit of "session time" and so on says "in seconds".
> But, is "in milliseconds" right?
> 
> 2. monitoring.sgml
> 
> IIUC, "active_time" includes the time executes a fast-path function and
> "idle in transaction" includes "idle in transaction(aborted)" time.
> 
> Why don't you reference pg_stat_activity's "state" column and
> "active_time" is the total time when the state is "active" and "fast 
> path"?
> "idle in transaction" is as same too.
> 
> 3. pgstat.h
> 
> The comment of PgStat_MsgConn says "Sent by pgstat_connection".
> I thought "pgstat_connection" is a function, but it doesn't exist.
> 
> Is "Sent by the backend" right?
> 
> Although this is a trivial thing, the following row has too many tabs.
> Other structs have only one space.
> // }Pgstat_MsgConn;

Thanks for the feedback.

I am currently on vacations and will take a look after January 7.

Yours,
Laurenz Albe

Re: Add session statistics to pg_stat_database

[Masahiro Ikeda]

Hi,

As a user, I want this feature to know whether
clients' session activities are as expected.

I have some comments about the patch.


1. pg_proc.dat

The unit of "session time" and so on says "in seconds".
But, is "in milliseconds" right?


2. monitoring.sgml

IIUC, "active_time" includes the time executes a fast-path function and
"idle in transaction" includes "idle in transaction(aborted)" time.

Why don't you reference pg_stat_activity's "state" column and
"active_time" is the total time when the state is "active" and "fast 
path"?

"idle in transaction" is as same too.


3. pgstat.h

The comment of PgStat_MsgConn says "Sent by pgstat_connection".
I thought "pgstat_connection" is a function, but it doesn't exist.

Is "Sent by the backend" right?

Although this is a trivial thing, the following row has too many tabs.
Other structs have only one space.
// }Pgstat_MsgConn;



Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Tue, 2020-12-15 at 13:53 +0100, Laurenz Albe wrote:
> Attached is patch version 9.

Aah, I forgot the ++.
Version 10 attached.

Yours,
Laurenz Albe
From b40e34141c80ff59c0005f430bd8c273918eb7bb Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Tue, 15 Dec 2020 13:46:44 +0100
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- total number of connections
- number of sessions that ended by loss of network connection,
  fatal errors and operator intervention
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Discussion: https://postgr.es/m/b07e1f9953701b90c66ed368656f2aef40cac4fb.ca...@cybertec.at
Reviewed-By: Soumyadeep Chakraborty, Justin Pryzby, Masahiro Ikeda, Magnus Hagander

(This requires a catversion bump, as well as an update to PGSTAT_FILE_FORMAT_ID)
---
 doc/src/sgml/monitoring.sgml |  69 ++
 src/backend/catalog/system_views.sql |   7 ++
 src/backend/postmaster/pgstat.c  | 134 ++-
 src/backend/tcop/postgres.c  |  10 +-
 src/backend/utils/adt/pgstatfuncs.c  |  94 +++
 src/backend/utils/error/elog.c   |   8 ++
 src/include/catalog/pg_proc.dat  |  28 ++
 src/include/pgstat.h |  39 
 src/test/regress/expected/rules.out  |   7 ++
 9 files changed, 391 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 52a69a5366..6206fefec0 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3731,6 +3731,75 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent by database sessions in this database, in milliseconds
+   (note that statistics are only updated when the state of a session
+   changes, so if sessions have been idle for a long time, this idle time
+   won't be included)
+  
+ 
+
+ 
+  
+   active_time double precision
+  
+  
+   Time spent executing SQL statements in this database, in milliseconds
+  
+ 
+
+ 
+  
+   idle_in_transaction_time double precision
+  
+  
+   Time spent idling while in a transaction in this database, in milliseconds
+  
+ 
+
+ 
+  
+   sessions bigint
+  
+  
+   Total number of sessions established to this database
+  
+ 
+
+ 
+  
+   sessions_abandoned bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   because connection to the client was lost
+  
+ 
+
+ 
+  
+   sessions_fatal bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   by fatal errors
+  
+ 
+
+ 
+  
+   sessions_killed bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   by operator intervention
+  
+ 
+
  
   
stats_reset timestamp with time zone
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index b140c210bc..3a2569b135 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -924,6 +924,13 @@ CREATE VIEW pg_stat_database AS
 pg_stat_get_db_checksum_last_failure(D.oid) AS checksum_last_failure,
 pg_stat_get_db_blk_read_time(D.oid) AS blk_read_time,
 pg_stat_get_db_blk_write_time(D.oid) AS blk_write_time,
+pg_stat_get_db_session_time(D.oid) AS session_time,
+pg_stat_get_db_active_time(D.oid) AS active_time,
+pg_stat_get_db_idle_in_transaction_time(D.oid) AS idle_in_transaction_time,
+pg_stat_get_db_sessions(D.oid) AS sessions,
+pg_stat_get_db_sessions_abandoned(D.oid) AS sessions_abandoned,
+pg_stat_get_db_sessions_fatal(D.oid) AS sessions_fatal,
+pg_stat_get_db_sessions_killed(D.oid) AS sessions_killed,
 pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset
 FROM (
 SELECT 0 AS oid, NULL::name AS datname
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 7c75a25d21..a2337b78f1 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -258,6 +258,9 @@ static int	pgStatXactCommit = 0;
 static int	pgStatXactRollback = 0;
 PgStat_Counter pgStatBlockReadTime = 0;
 PgStat_Counter pgStatBlockWriteTime = 0;
+static PgStat_Counter pgStatActiveTime = 0;
+static PgStat_Counter pgStatTransactio

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Sun, 2020-12-13 at 17:49 +0100, Magnus Hagander wrote:
> > > 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 named the three counters "sessions_client_eof", "sessions_fatal" and
> > "sessions_killed", but I am not wedded to these bike shed colors.
> 
> 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?

I slept over it, and came up with "sessions_abandoned".

> 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"?

Yes.  I named it DISCONNECT_NOT_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++;

Ok, changed.

> + typedef enum sessionEndType {
> 
> To be consistent with the other enums in the same place, seems this should be 
> SessionEndType.

True.  I have renamed the type.

Attached is patch version 9.
Added goodie: I ran pgindent on it.

Yours,
Laurenz Albe
From b40e34141c80ff59c0005f430bd8c273918eb7bb Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Tue, 15 Dec 2020 13:46:44 +0100
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- total number of connections
- number of sessions that ended by loss of network connection,
  fatal errors and operator intervention
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Discussion: https://postgr.es/m/b07e1f9953701b90c66ed368656f2aef40cac4fb.ca...@cybertec.at
Reviewed-By: Soumyadeep Chakraborty, Justin Pryzby, Masahiro Ikeda, Magnus Hagander

(This requires a catversion bump, as well as an update to PGSTAT_FILE_FORMAT_ID)
---
 doc/src/sgml/monitoring.sgml |  69 ++
 src/backend/catalog/system_views.sql |   7 ++
 src/backend/postmaster/pgstat.c  | 134 ++-
 src/backend/tcop/postgres.c  |  10 +-
 src/backend/utils/adt/pgstatfuncs.c  |  94 +++
 src/backend/utils/error/elog.c   |   8 ++
 src/include/catalog/pg_proc.dat  |  28 ++
 src/include/pgstat.h |  39 
 src/test/regress/expected/rules.out  |   7 ++
 9 files changed, 391 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 52a69a5366..6206fefec0 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3731,6 +3731,75 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent by database sessions in this database, in milliseconds
+   (note that statistics are only updated when the state of a session
+   changes, so if sessions have been idle for a long time, this idle time
+   won't be included)
+  
+ 
+
+ 
+  
+   active_time double precision
+  
+  
+   Time spent executing SQL statements in this database, in milliseconds
+  
+ 
+
+ 
+  
+   idle_in_transaction_time double precision
+  
+  
+   Time spent idling while in a transaction in this database, in milliseconds
+  
+ 
+
+ 
+  
+   sessions bigint
+  
+  
+   Total number of sessions established to this database
+  
+ 
+
+ 
+  
+   sessions_abandoned bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   because connection to the client was lost
+  
+ 
+
+ 
+  
+   sessions_fatal bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   by fatal errors
+  
+ 
+
+ 
+  
+

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

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.

Yours,
Laurenz Albe
From 96c4aaa71ceff3ef83004780ac6dc3f5059302bb Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Sat, 5 Dec 2020 12:58:46 +0100
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- total number of connections
- number of sessions that ended by loss of network connection,
  fatal errors and operator intervention
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Discussion: https://postgr.es/m/b07e1f9953701b90c66ed368656f2aef40cac4fb.ca...@cybertec.at
Reviewed-By: Soumyadeep Chakraborty, Justin Pryzby, Masahiro Ikeda, Magnus Hagander

(This requires a catversion bump, as well as an update to
PGSTAT_FILE_FORMAT_ID)
---
 doc/src/sgml/monitoring.sgml |  69 ++
 src/backend/catalog/system_views.sql |   7 ++
 src/backend/postmaster/pgstat.c  | 134 ++-
 src/backend/tcop/postgres.c  |  20 ++--
 src/backend/utils/adt/pgstatfuncs.c  |  94 +++
 src/backend/utils/error/elog.c   |   9 ++
 src/include/catalog/pg_proc.dat  |  28 ++
 src/include/pgstat.h |  37 
 src/test/regress/expected/rules.out  |   7 ++
 9 files changed, 395 insertions(+), 10 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 52a69a5366..9bc1acf841 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3731,6 +3731,75 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent by database sessions in this database, in milliseconds
+   (note that statistics are only updated when the state of a session
+   changes, so if sessions have been idle for a long time, this idle time
+   won't be included)
+  
+ 
+
+ 
+  
+   active_time double precision
+  
+  
+   Time spent executing SQL statements in this database, in milliseconds
+  
+ 
+
+ 
+  
+   idle_in_transaction_time double precision
+  
+  
+   Time spent idling while in a transaction in this database, in milliseconds
+  
+ 
+
+ 
+  
+   sessions bigint
+  
+  
+   Total number of sessions established to this database
+  
+ 
+
+ 
+  
+   sessions_client_eof bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   because connection to the client was lost
+  
+ 
+
+ 
+  
+   sessions_fatal bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   by fatal errors
+  
+ 
+
+ 
+  
+   sessions_killed bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   by operator intervention
+  
+ 
+
  
   
stats_reset timestamp with time zone
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index b140c210bc..1b022a114f 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -924,6 +924,13 @@ CREATE VIEW pg_stat_database AS
 pg_stat_get_db_checksum_last_failure(D.oid) AS checksum_last_failure,
 pg_stat_get_db_blk_read_time(D.

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Thu, 2020-12-03 at 13:22 +0100, Laurenz Albe 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).
> 
> > (Let me know if you think the idea could work and would prefer it if I 
> > worked up a
> > complete suggestion based on it rather than just spitting ideas)
> 
> Thanks for the offer, and I'll get back to it if I get stuck.

Ok, I could use a pointer.

I am considering the cases

1) client just went away (currently "aborted")
2) death by FATAL error
3) killed by the administrator (or shutdown)

What is a good place in the code to tell 2) or 3)
so that I can set the state accordingly?

Yours,
Laurenz Albe

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Tue, 2020-12-01 at 17:32 +0100, Magnus Hagander wrote:
> > I have changed "connections" to "sessions" and renamed the new
> > column "connections" to "session_count".
> > 
> > I think that most people will understand a session as started after a 
> > successful
> > connection.
> 
> Yeah, I agree, and as long as it's consistent we don't need more explanations 
> than that.
> 
> Further int he views, it's a bit strange to have session_count and 
> aborted_session, but I'm not
>  sure what to suggest. "aborted_session_count" seems too long. Maybe just 
> "sessions" instead
>  of "session_count" -- no other counters actually have the "_count" suffix.

"sessions" is fine, I think; I changed the name.

> > > I wonder if there would also be a way to count "sessions that crashed" as 
> > > well.
> > > That is,the ones that failed in a way that caused the postmaster to 
> > > restart the system.
> >
> > Sure, a crash count would be useful.  I don't know if it is easy for the 
> > stats collector
> > to tell the difference between a start after a backend crash and - say - 
> > starting from
> > a base backup.
> > 
> > I think that that would be material for another patch, and I don't think it 
> > should go
> > to "pg_stat_database", because a) it might be hard to tell to which 
> > database the crashed
> > backend was attached, b) it might be a background process that doesn't 
> > belong to a database
> > and c) if the crash were caused by - say - corruption in a shared catalog, 
> > it would be
> > misleading
> 
> I'm not sure it is outside the scope of this patch, because I think it might 
> be easier to
>  do than I (and I think you) first thought. We don't need to track which 
> database crashed --
>  if we track all *other* ways a database exits, then crashes are all that 
> remains.
> 
> So in fact, we *almost* have all the data we need already. We have the number 
> of sessions
>  started. We have the number of sessions "aborted". if we also had the number 
> of sessions
>  that were closed normally, then whatever is "left" would be the number of 
> sessions crashed.
>  And we do already, in your patch, send the message in the case of both 
> aborted and
>  non-aborted sessions. So we just need to keep track of both in the statsfile
>  (which we don't now), and we'd more or less have it, wouldn't we?

There is one problem with that: the statistics collector is not guaranteed to 
get all
messages, right?  If a disconnection statistics UDP datagram doesn't reach the 
statistics
collector, that connection
would end up being reported as crashed.
That would alarm people unnecessarily and make the crash statistics misleading.

> However, some thinking around that also leads me to another question which is 
> very much
>  in scope for this patch regardless, which is what about shutdown and admin 
> termination.
>  Right now, when you do a "pg_ctl stop" on the database, all sessions count 
> as aborted.
>  Same thing for a pg_terminate_backend(). I wonder if this is also a case 
> that would be
>  useful to track as a separate thing? One could argue that the docs in your 
> patch say
>  aborted means "terminated by something else than a regular client 
> disconnection".
> But that's true for a "shutdown", but not for a crash, so whichever way we go 
> with crashes
>  it's slightly incorrect.

> But thinking from a usability perspective, wouldn't what we want more be 
> something
>  like , , 
> ,
>  ?
> 
> What do you think of adapting it to that?
> 
> 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).

> (Let me know if you think the idea could work and would prefer it if I worked 
> up a
>  complete suggestion based on it rather than just spitting ideas)

Thanks for the offer, and I'll get back to it if I get stuck.
But I'm ready to do the grunt work, so that you can spend your precious
committer cycles elsewhe

Re: Add session statistics to pg_stat_database

[Magnus Hagander]

On Fri, Nov 20, 2020 at 3:41 PM Laurenz Albe 
wrote:

> On Tue, 2020-11-17 at 17:33 +0100, Magnus Hagander wrote:
> > I've taken a look as well, and here are a few short notes:
>
> Much appreciated!
>

Sorry about the delay in getting back to you on this one. FYI, while the
patch has been bumped to the next CF by now, I do intend to continue
working on it before that starts.


> * It talks about "number of connections" but "number of aborted
> sessions". We should probably
> >   be consistent about talking either about connections or sessions? In
> particular, connections
> >   seems wrong in this case, because it only starts counting after
> authentication is complete
> >   (since otherwise we send no stats)? (This goes for both docs and
> actual function names)
>
> Yes, that is true.  I have changed "connections" to "sessions" and renamed
> the new
> column "connections" to "session_count".
>
> I think that most people will understand a session as started after a
> successful
> connection.
>

Yeah, I agree, and as long as it's consistent we don't need more
explanations than that.

Further int he views, it's a bit strange to have session_count and
aborted_session, but I'm not sure what to suggest. "aborted_session_count"
seems too long. Maybe just "sessions" instead of "session_count" -- no
other counters actually have the "_count" suffix.


> * Is this actually a fix that's independent of the new stats? It seems in
> general to be
> >   changing the behaviour of "force", which is more generic?
> > -   !have_function_stats)
> > +   !have_function_stats && !force)
>
> The comment right above that reads:
> /* Don't expend a clock check if nothing to do */
> So it is just a quick exit if there is nothing to do.
>
> But with that patch we have something to do if "force" (see below) is true:
> Report the remaining session duration and if the session was closed
> normally.
>
> Thus the additional check.
>

Ah yeah, makes sense. It becomes more clear with the rename.


> * in pgstat_send_connstat() you pass the parameter "force" in as
> "disconnect".
> >   That behaviour at least requires a comment saying why, I think. My
> understanding is
> >   it relies on that "force" means this is a "backend is shutting down",
> but that is not
> >   actually documented anywhere. Maybe the "force" parameter should
> actually be renamed
> >   to indicate this is really what it means, to avoid a future mistake in
> the area?
> >   But even with that, how does that turn into disconnect?
>
> "pgstat_report_stat(true)" is only called from "pgstat_beshutdown_hook()",
> so
> it is currently only called when the backend is about to exit.
>
> According the the comments the flag means that "caller wants to force
> stats out".
> I guess that the author thought that there may arise other reasons to
> force sending
> statistics in the future (commit 641912b4d from 2007).
>
> However, since that has not happened, I have renamed the flag to
> "disconnect" and
> adapted the documentation.  This doesn't change the current behavior, but
> establishes
> a new rule.
>

That makes it a lot more clear. And I agree, if nobody came up with a
reason since 2007, then we are free to repurpose it :)



> * Maybe rename pgStatSessionDisconnected to
> pgStatSessionNormalDisconnected?
> >   To avoid having to go back to the setting point and look it up in a
> comment.
>
> Long, descriptive names are a good thing.
> I have decided to use "pgStatSessionDisconnectedNormally", since that is
> even longer
> and seems to fit the "yes or no" category better.
>

WFM.


> I wonder if there would also be a way to count "sessions that crashed" as
> well.
> >  That is,the ones that failed in a way that caused the postmaster to
> restart the system.
> >  But that's information we'd have to send from the postmaster, but I'm
> actually unsure
> >  if we're "allowed" to send things to the stats collector from the
> postmaster.
> >  But I think it could be quite useful information to have. Maybe we can
> find some way
> >  to piggyback on the fact that we're restarting the stats collector as a
> result?
>
> Sure, a crash count would be useful.  I don't know if it is easy for the
> stats collector
> to tell the difference between a start after a backend crash and - say -
> starting from
> a base backup.
>
> Patch v6 attached.
>
> I think that that would be material for another patch, and I don't think
> it should go
> to "pg_stat_database", because a) it might be hard to tell to which
> database the crashed
> backend was attached, b) it might be a background process that doesn't
> belong to a database
> and c) if the crash were caused by - say - corruption in a shared catalog,
> it would be
> misleading


I'm not sure it is outside the scope of this patch, because I think it
might be easier to do than I (and I think you) first thought. We don't need
to track which database crashed -- if we track all *other* ways a database
exits, then crashes are 

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Tue, 2020-11-17 at 17:33 +0100, Magnus Hagander wrote:
> I've taken a look as well, and here are a few short notes:

Much appreciated!

> * It talks about "number of connections" but "number of aborted sessions". We 
> should probably
>   be consistent about talking either about connections or sessions? In 
> particular, connections
>   seems wrong in this case, because it only starts counting after 
> authentication is complete
>   (since otherwise we send no stats)? (This goes for both docs and actual 
> function names)

Yes, that is true.  I have changed "connections" to "sessions" and renamed the 
new
column "connections" to "session_count".

I think that most people will understand a session as started after a successful
connection.

> * Is there a reason we're counting active and idle in transaction (including 
> aborted),
>   but not fastpath? In particular, we seem to ignore fastpath -- if we don't 
> want to single
>   it out specifically, it should probably be included in active?

The only reason is that I didn't think of it.  Fixed.

> * pgstat_send_connstat() but pgstat_recv_connection(). Let's call both 
> connstat or both
>   connection (I'd vote connstat)?

Agreed, done.

> * Is this actually a fix that's independent of the new stats? It seems in 
> general to be
>   changing the behaviour of "force", which is more generic?
> -   !have_function_stats)
> +   !have_function_stats && !force)

The comment right above that reads:
/* Don't expend a clock check if nothing to do */
So it is just a quick exit if there is nothing to do.

But with that patch we have something to do if "force" (see below) is true:
Report the remaining session duration and if the session was closed normally.

Thus the additional check.

> * in pgstat_send_connstat() you pass the parameter "force" in as "disconnect".
>   That behaviour at least requires a comment saying why, I think. My 
> understanding is
>   it relies on that "force" means this is a "backend is shutting down", but 
> that is not
>   actually documented anywhere. Maybe the "force" parameter should actually 
> be renamed
>   to indicate this is really what it means, to avoid a future mistake in the 
> area?
>   But even with that, how does that turn into disconnect?

"pgstat_report_stat(true)" is only called from "pgstat_beshutdown_hook()", so
it is currently only called when the backend is about to exit.

According the the comments the flag means that "caller wants to force stats 
out".
I guess that the author thought that there may arise other reasons to force 
sending
statistics in the future (commit 641912b4d from 2007).

However, since that has not happened, I have renamed the flag to "disconnect" 
and
adapted the documentation.  This doesn't change the current behavior, but 
establishes
a new rule.

> * Maybe rename pgStatSessionDisconnected to pgStatSessionNormalDisconnected?
>   To avoid having to go back to the setting point and look it up in a comment.

Long, descriptive names are a good thing.
I have decided to use "pgStatSessionDisconnectedNormally", since that is even 
longer
and seems to fit the "yes or no" category better.
 
> I wonder if there would also be a way to count "sessions that crashed" as 
> well.
>  That is,the ones that failed in a way that caused the postmaster to restart 
> the system.
>  But that's information we'd have to send from the postmaster, but I'm 
> actually unsure
>  if we're "allowed" to send things to the stats collector from the postmaster.
>  But I think it could be quite useful information to have. Maybe we can find 
> some way
>  to piggyback on the fact that we're restarting the stats collector as a 
> result?

Sure, a crash count would be useful.  I don't know if it is easy for the stats 
collector
to tell the difference between a start after a backend crash and - say - 
starting from
a base backup.

Patch v6 attached.

I think that that would be material for another patch, and I don't think it 
should go
to "pg_stat_database", because a) it might be hard to tell to which database 
the crashed
backend was attached, b) it might be a background process that doesn't belong 
to a database
and c) if the crash were caused by - say - corruption in a shared catalog, it 
would be
misleading.

Yours,
Laurenz Albe
From 8feed416f91a5de9011616c1545156b9c8f28943 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Fri, 20 Nov 2020 15:11:57 +0100
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- total number of connections
- number of sessions that e

Re: Add session statistics to pg_stat_database

[Magnus Hagander]

On Tue, Nov 17, 2020 at 4:22 PM Laurenz Albe 
wrote:

> On Fri, 2020-10-16 at 16:24 +0500, Ahsan Hadi wrote:
> > I have applied the latest patch on master, all the regression test cases
> are passing
> >  and the implemented functionality is also looking fine. The point that
> I raised about
> >  idle connection not included is also addressed.
>
> If you think that the patch is ready to go, you could mark it as
> "ready for committer" in the commitfest app.
>

I've taken a look as well, and here are a few short notes:

* It talks about "number of connections" but "number of aborted sessions".
We should probably be consistent about talking either about connections or
sessions? In particular, connections seems wrong in this case, because it
only starts counting after authentication is complete (since otherwise we
send no stats)? (This goes for both docs and actual function names)

* Is there a reason we're counting active and idle in transaction
(including aborted), but not fastpath? In particular, we seem to ignore
fastpath -- if we don't want to single it out specifically, it should
probably be included in active?

* pgstat_send_connstat() but pgstat_recv_connection(). Let's call both
connstat or both connection (I'd vote connstat)?

* Is this actually a fix that's independent of the new stats? It seems in
general to be changing the behaviour of "force", which is more generic?
-   !have_function_stats)
+   !have_function_stats && !force)

* in pgstat_send_connstat() you pass the parameter "force" in as
"disconnect". That behaviour at least requires a comment saying why, I
think. My understanding is it relies on that "force" means this is
a "backend is shutting down", but that is not actually documented anywhere.
Maybe the "force" parameter should actually be renamed to indicate this is
really what it means, to avoid a future mistake in the area? But even with
that, how does that turn into disconnect?

* Maybe rename pgStatSessionDisconnected
to pgStatSessionNormalDisconnected? To avoid having to go back to the
setting point and look it up in a comment.

I wonder if there would also be a way to count "sessions that crashed" as
well. That is,the ones that failed in a way that caused the postmaster to
restart the system. But that's information we'd have to send from the
postmaster, but I'm actually unsure if we're "allowed" to send things to
the stats collector from the postmaster. But I think it could be quite
useful information to have. Maybe we can find some way to piggyback on the
fact that we're restarting the stats collector as a result?

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

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Fri, 2020-10-16 at 16:24 +0500, Ahsan Hadi wrote:
> I have applied the latest patch on master, all the regression test cases are 
> passing
>  and the implemented functionality is also looking fine. The point that I 
> raised about
>  idle connection not included is also addressed.

If you think that the patch is ready to go, you could mark it as
"ready for committer" in the commitfest app.

Yours,
Laurenz Albe

Re: Add session statistics to pg_stat_database

[Georgios]

‐‐‐ Original Message ‐‐‐
On Thursday, November 12, 2020 9:31 AM, Laurenz Albe  
wrote:

> I wrote:
>
> > On Tue, 2020-11-10 at 15:03 +, Georgios Kokolatos wrote:
> >
> > > I noticed that the cfbot fails for this patch.
> > > For this, I am setting the status to: 'Waiting on Author'.
> >
> > Thanks for noticing, it was only the documentation build.
> > Version 5 attached, status changed back to "waiting for review".
>
> The patch is still failing, so I looked again:
>
> make[3]: Entering directory 
> '/home/travis/build/postgresql-cfbot/postgresql/doc/src/sgml'
> { \
> echo ""; \
>
> echo ""; \\
>
>
> } > version.sgml
> '/usr/bin/perl' ./mk_feature_tables.pl YES 
> ../../../src/backend/catalog/sql_feature_packages.txt 
> ../../../src/backend/catalog/sql_features.txt > features-supported.sgml
> '/usr/bin/perl' ./mk_feature_tables.pl NO 
> ../../../src/backend/catalog/sql_feature_packages.txt 
> ../../../src/backend/catalog/sql_features.txt > features-unsupported.sgml
> '/usr/bin/perl' ./generate-errcodes-table.pl 
> ../../../src/backend/utils/errcodes.txt > errcodes-table.sgml
> '/usr/bin/perl' ./generate-keywords-table.pl . > keywords-table.sgml
> /usr/bin/xmllint --path . --noout --valid postgres.sgml
> error : Unknown IO error
> postgres.sgml:21: /usr/bin/bison -Wno-deprecated -d -o gram.c gram.y
> warning: failed to load external entity 
> "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd;
> ]>
>
> ^
>
>
> postgres.sgml:23: element book: validity error : No declaration for attribute 
> id of element book
> 
>
>  ^
>
>
> postgres.sgml:24: element title: validity error : No declaration for element 
> title
> PostgreSQL  Documentation
>
> I have the impression that this is not the fault of my patch, something seems 
> to be
> wrong with the cfbot.
>
> I see that other patches are failing with the same error.

You are indeed correct. Unfortunately the cfbot is a bit unstable due
to some issues related to the documentation. I alerted a contributor
and he was quick to try to address the issue in pgsql-www [1].

Thank you very much for looking and apologies for the chatter.

>
> Yours,
> Laurenz Albe

[1] 
https://www.postgresql.org/message-id/E2EE6B76-2D96-408A-B961-CAE47D1A86F0%40yesql.se

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

I wrote:
> On Tue, 2020-11-10 at 15:03 +, Georgios Kokolatos wrote:
> > I noticed that the cfbot fails for this patch.
> > 
> > For this, I am setting the status to: 'Waiting on Author'.
> 
> Thanks for noticing, it was only the documentation build.
> 
> Version 5 attached, status changed back to "waiting for review".

The patch is still failing, so I looked again:

  make[3]: Entering directory 
'/home/travis/build/postgresql-cfbot/postgresql/doc/src/sgml'
  { \
echo ""; \
echo ""; \
  } > version.sgml
  '/usr/bin/perl' ./mk_feature_tables.pl YES 
../../../src/backend/catalog/sql_feature_packages.txt 
../../../src/backend/catalog/sql_features.txt > features-supported.sgml
  '/usr/bin/perl' ./mk_feature_tables.pl NO 
../../../src/backend/catalog/sql_feature_packages.txt 
../../../src/backend/catalog/sql_features.txt > features-unsupported.sgml
  '/usr/bin/perl' ./generate-errcodes-table.pl 
../../../src/backend/utils/errcodes.txt > errcodes-table.sgml
  '/usr/bin/perl' ./generate-keywords-table.pl . > keywords-table.sgml
  /usr/bin/xmllint --path . --noout --valid postgres.sgml
  error : Unknown IO error
  postgres.sgml:21: /usr/bin/bison -Wno-deprecated  -d -o gram.c gram.y
  warning: failed to load external entity 
"http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd;
  ]>
^
  postgres.sgml:23: element book: validity error : No declaration for attribute 
id of element book
  
 ^
  postgres.sgml:24: element title: validity error : No declaration for element 
title
   PostgreSQL  Documentation

I have the impression that this is not the fault of my patch, something seems 
to be
wrong with the cfbot.

I see that other patches are failing with the same error.

Yours,
Laurenz Albe

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Tue, 2020-11-10 at 15:03 +, Georgios Kokolatos wrote:
> I noticed that the cfbot fails for this patch.
> 
> For this, I am setting the status to: 'Waiting on Author'.

Thanks for noticing, it was only the documentation build.

Version 5 attached, status changed back to "waiting for review".

Yours,
Laurenz Albe
From afc37856c12fd0a85587c638fca291a0b5652d9b Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Wed, 11 Nov 2020 20:14:28 +0100
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- total number of connections
- number of sessions that ended other than with a client disconnect
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Discussion: https://postgr.es/m/b07e1f9953701b90c66ed368656f2aef40cac4fb.ca...@cybertec.at
Reviewed-By: Soumyadeep Chakraborty, Justin Pryzby, Masahiro Ikeda

(This requires a catversion bump, as well as an update to
PGSTAT_FILE_FORMAT_ID)
---
 doc/src/sgml/monitoring.sgml |  49 +
 src/backend/catalog/system_views.sql |   5 ++
 src/backend/postmaster/pgstat.c  | 105 ++-
 src/backend/tcop/postgres.c  |   5 ++
 src/backend/utils/adt/pgstatfuncs.c  |  68 +
 src/include/catalog/pg_proc.dat  |  20 +
 src/include/pgstat.h |  27 +++
 src/test/regress/expected/rules.out  |   5 ++
 8 files changed, 283 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 98e1995453..89610d1010 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3704,6 +3704,55 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent by database sessions in this database, in milliseconds
+   (note that statistics are only updated when the state of a session
+   changes, so if sessions have been idle for a long time, this idle time
+   won't be included)
+  
+ 
+
+ 
+  
+   active_time double precision
+  
+  
+   Time spent executing SQL statements in this database, in milliseconds
+  
+ 
+
+ 
+  
+   idle_in_transaction_time double precision
+  
+  
+   Time spent idling while in a transaction in this database, in milliseconds
+  
+ 
+
+ 
+  
+   connections bigint
+  
+  
+   Total number of connections established to this database
+  
+ 
+
+ 
+  
+   aborted_sessions bigint
+  
+  
+   Number of database sessions to this database that were terminated
+   by something else than a regular client disconnection
+  
+ 
+
  
   
stats_reset timestamp with time zone
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 2e4aa1c4b6..998b4d542a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -924,6 +924,11 @@ CREATE VIEW pg_stat_database AS
 pg_stat_get_db_checksum_last_failure(D.oid) AS checksum_last_failure,
 pg_stat_get_db_blk_read_time(D.oid) AS blk_read_time,
 pg_stat_get_db_blk_write_time(D.oid) AS blk_write_time,
+pg_stat_get_db_session_time(D.oid) AS session_time,
+pg_stat_get_db_active_time(D.oid) AS active_time,
+pg_stat_get_db_idle_in_transaction_time(D.oid) AS idle_in_transaction_time,
+pg_stat_get_db_connections(D.oid) AS connections,
+pg_stat_get_db_aborted_sessions(D.oid) AS aborted_sessions,
 pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset
 FROM (
 SELECT 0 AS oid, NULL::name AS datname
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index e76e627c6b..9978aab60a 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -249,6 +249,9 @@ static int	pgStatXactCommit = 0;
 static int	pgStatXactRollback = 0;
 PgStat_Counter pgStatBlockReadTime = 0;
 PgStat_Counter pgStatBlockWriteTime = 0;
+static PgStat_Counter pgStatActiveTime = 0;
+static PgStat_Counter pgStatTransactionIdleTime = 0;
+bool pgStatSessionDisconnected = false;
 
 /* Record that's written to 2PC state file when pgstat state is persisted */
 typedef struct TwoPhasePgStatRecord
@@ -334,6 +337,7 @@ static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg);
 static void pgstat_send_funcstats(void);
 static void pgstat_send_slru(void);
 static HTAB *pgstat_collect_oids(Oid catalogid, AttrNumber anum_oid);
+static vo

Re: Add session statistics to pg_stat_database

[Georgios Kokolatos]

Hi,

I noticed that the cfbot fails for this patch.
For this, I am setting the status to: 'Waiting on Author'.

Cheers,
//Georgios

The new status of this patch is: Waiting on Author

Re: Add session statistics to pg_stat_database

[Ahsan Hadi]

Hi Laurenz,

I have applied the latest patch on master, all the regression test cases
are passing and the implemented functionality is also looking fine. The
point that I raised about idle connection not included is also addressed.

thanks,
Ahsan

On Wed, Oct 14, 2020 at 2:28 PM Laurenz Albe 
wrote:

> Thanks for the --- as always --- valuable review!
>
> On Tue, 2020-10-13 at 17:55 -0500, Justin Pryzby wrote:
> > On Tue, Oct 13, 2020 at 01:44:41PM +0200, Laurenz Albe wrote:
> > > Attached is v3 with improvements.
> >
> > +  
> > +   Time spent in database sessions in this database, in
> milliseconds.
> > +  
> >
> > Should say "Total time spent *by* DB sessions..." ?
>
> That is indeed better.  Fixed.
>
> > I think these counters are only accurate as of the last state change,
> right?
> > So a session which has been idle for 1hr, that 1hr is not included.  I
> think
> > the documentation should explain that, or (ideally) the implementation
> would be
> > more precise.  Maybe the timestamps should only be updated after a
> session
> > terminates (and the docs should say so).
>
> I agree, and I have added an explanation that the value doesn't include
> the duration of the current state.
>
> Of course it would be nice to have totally accurate values, but I think
> that the statistics are by nature inaccurate (datagrams can get lost),
> and more frequent statistics updates increase the work load.
> I don't think that is worth the effort.
>
> > +  
> > +   connections bigint
> > +  
> > +  
> > +   Number of connections established to this database.
> >
> > *Total* number of connections established, otherwise it sounds like it
> might
> > mean "the number of sessions [currently] established".
>
> Fixed like that.
>
> > +   Number of database sessions to this database that did not end
> > +   with a regular client disconnection.
> >
> > Does that mean "sessions which ended irregularly" ?  Or does it also
> include
> > "sessions which have not ended" ?
>
> I have added an explanation for that.
>
> > +   msg.m_aborted = (!disconnect || pgStatSessionDisconnected) ? 0 :
> 1;
> >
> > I think this can be just:
> > msg.m_aborted = (bool) (disconnect && !pgStatSessionDisconnected);
>
> I mulled over this and finally decided to leave it as it is.
>
> Since "m_aborted" gets added to the total counter, I'd prefer to
> have it be an "int".
>
> Your proposed code works (the cast is actually not necessary, right?).
> But I think that my version is more readable if you think of
> "m_aborted" as a counter rather than a flag.
>
> > +   if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL)
> > +   result = 0;
> > +   else
> > +   result = ((double) dbentry->n_session_time) / 1000.0;
> >
> > I think these can say:
> > > double result = 0;
> > > if ((dbentry=..) != NULL)
> > >  result = (double) ..;
> >
> > That not only uses fewer LOC, but also the assignment to zero is (known
> to be)
> > done at compile time (BSS) rather than runtime.
>
> I didn't know about the performance difference.
> Concise code (if readable) is good, so I changed the code like you propose.
>
> The code pattern is actually copied from neighboring functions,
> which then should also be changed like this, but that is outside
> the scope of this patch.
>
> Attached is v4 of the patch.
>
> Yours,
> Laurenz Albe
>


-- 
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.h...@highgo.ca

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

Thanks for the --- as always --- valuable review!

On Tue, 2020-10-13 at 17:55 -0500, Justin Pryzby wrote:
> On Tue, Oct 13, 2020 at 01:44:41PM +0200, Laurenz Albe wrote:
> > Attached is v3 with improvements.
> 
> +  
> +   Time spent in database sessions in this database, in milliseconds.
> +  
> 
> Should say "Total time spent *by* DB sessions..." ?

That is indeed better.  Fixed.

> I think these counters are only accurate as of the last state change, right?
> So a session which has been idle for 1hr, that 1hr is not included.  I think
> the documentation should explain that, or (ideally) the implementation would 
> be
> more precise.  Maybe the timestamps should only be updated after a session
> terminates (and the docs should say so).

I agree, and I have added an explanation that the value doesn't include
the duration of the current state.

Of course it would be nice to have totally accurate values, but I think
that the statistics are by nature inaccurate (datagrams can get lost),
and more frequent statistics updates increase the work load.
I don't think that is worth the effort.

> +  
> +   connections bigint
> +  
> +  
> +   Number of connections established to this database.
> 
> *Total* number of connections established, otherwise it sounds like it might
> mean "the number of sessions [currently] established".

Fixed like that.

> +   Number of database sessions to this database that did not end
> +   with a regular client disconnection.
> 
> Does that mean "sessions which ended irregularly" ?  Or does it also include
> "sessions which have not ended" ?

I have added an explanation for that.

> +   msg.m_aborted = (!disconnect || pgStatSessionDisconnected) ? 0 : 1;
> 
> I think this can be just:
> msg.m_aborted = (bool) (disconnect && !pgStatSessionDisconnected);

I mulled over this and finally decided to leave it as it is.

Since "m_aborted" gets added to the total counter, I'd prefer to
have it be an "int".

Your proposed code works (the cast is actually not necessary, right?).
But I think that my version is more readable if you think of
"m_aborted" as a counter rather than a flag.

> +   if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL)
> +   result = 0;
> +   else
> +   result = ((double) dbentry->n_session_time) / 1000.0;
> 
> I think these can say:
> > double result = 0;
> > if ((dbentry=..) != NULL)
> >  result = (double) ..;
> 
> That not only uses fewer LOC, but also the assignment to zero is (known to be)
> done at compile time (BSS) rather than runtime.

I didn't know about the performance difference.
Concise code (if readable) is good, so I changed the code like you propose.

The code pattern is actually copied from neighboring functions,
which then should also be changed like this, but that is outside
the scope of this patch.

Attached is v4 of the patch.

Yours,
Laurenz Albe
From 9e8bf3efd984306c73243736d0b4a4023cdd5f3a Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Wed, 14 Oct 2020 11:08:20 +0200
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- total number of connections
- number of sessions that ended other than with a client disconnect
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Discussion: https://postgr.es/m/b07e1f9953701b90c66ed368656f2aef40cac4fb.ca...@cybertec.at
Reviewed-By: Soumyadeep Chakraborty, Justin Pryzby, Masahiro Ikeda

(This requires a catversion bump, as well as an update to
PGSTAT_FILE_FORMAT_ID)
---
 doc/src/sgml/monitoring.sgml |  49 +
 src/backend/catalog/system_views.sql |   5 ++
 src/backend/postmaster/pgstat.c  | 105 ++-
 src/backend/tcop/postgres.c  |   5 ++
 src/backend/utils/adt/pgstatfuncs.c  |  68 +
 src/include/catalog/pg_proc.dat  |  20 +
 src/include/pgstat.h |  27 +++
 src/test/regress/expected/rules.out  |   5 ++
 8 files changed, 283 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 66566765f0..a50fc025d5 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3663,6 +3663,55 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent by

Re: Add session statistics to pg_stat_database

[Justin Pryzby]

On Tue, Oct 13, 2020 at 01:44:41PM +0200, Laurenz Albe wrote:
> Attached is v3 with improvements.

+  
+   Time spent in database sessions in this database, in milliseconds.
+  

Should say "Total time spent *by* DB sessions..." ?

I think these counters are only accurate as of the last state change, right?
So a session which has been idle for 1hr, that 1hr is not included.  I think
the documentation should explain that, or (ideally) the implementation would be
more precise.  Maybe the timestamps should only be updated after a session
terminates (and the docs should say so).

+  
+   connections bigint
+  
+  
+   Number of connections established to this database.

*Total* number of connections established, otherwise it sounds like it might
mean "the number of sessions [currently] established".

+   Number of database sessions to this database that did not end
+   with a regular client disconnection.

Does that mean "sessions which ended irregularly" ?  Or does it also include
"sessions which have not ended" ?

+   msg.m_aborted = (!disconnect || pgStatSessionDisconnected) ? 0 : 1;

I think this can be just:
msg.m_aborted = (bool) (disconnect && !pgStatSessionDisconnected);

+   if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL)
+   result = 0;
+   else
+   result = ((double) dbentry->n_session_time) / 1000.0;

I think these can say:
|double result = 0;
|if ((dbentry=..) != NULL)
|  result = (double) ..;

That not only uses fewer LOC, but also the assignment to zero is (known to be)
done at compile time (BSS) rather than runtime.

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Fri, 2020-10-02 at 15:10 -0700, Soumyadeep Chakraborty wrote:
> On Tue, Sep 29, 2020 at 2:44 AM Laurenz Albe  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
From 0cc86e8a2bf3ffc76358c9022636502779c30910 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Tue, 13 Oct 2020 13:26:48 +0200
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- number of connections
- number of sessions that were not disconnected regularly
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Discussion: https://postgr.es/m/b07e1f9953701b90c66ed368656f2aef40cac4fb.ca...@cybertec.at
Reviewed-By: Soumyadeep Chakraborty, Masahiro Ikeda
---
 doc/src/sgml/monitoring.sgml |  46 
 src/backend/catalog/system_views.sql |   5 ++
 src/backend/postmaster/pgstat.c  | 105 ++-
 src/backend/tcop/postgres.c  |   5 ++
 src/backend/utils/adt/pgstatfuncs.c  |  78 
 src/include/catalog/pg_proc.dat  |  20 +
 src/include/pgstat.h |  27 +++
 src/test/regress/expected/rules.out  |   5 ++
 8 files changed, 290 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 66566765f0..13ef586857 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3415,6 +3415,52 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+

Re: Add session statistics to pg_stat_database

[Masahiro Ikeda]

On 2020-09-05 00:50, Laurenz Albe wrote:

I have changed the code so that connections are counted immediately.
Attached is a new version.


Thanks for making a patch.
I'm interested in this feature.

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.

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

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Add session statistics to pg_stat_database

[Soumyadeep Chakraborty]

On Tue, Sep 29, 2020 at 2:44 AM Laurenz Albe  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)

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Thu, 2020-09-24 at 14:38 -0700, Soumyadeep Chakraborty wrote:
> Thanks for submitting this! Please find my feedback below.

Thanks for the thorough review.

Before I update the patch, I have a few comments and questions.

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

> * pgstat_send_connstats(): How about renaming the "force" argument to
> "disconnected"?

Yes, that might be better.  I'll do that.

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

But I agree with the following:

> Also, some of these fields are not required:
> 
> I don't think we need pgStatActiveStart and pgStatTransactionIdleStart -
> instead of these two we could use
> PgBackendStatus.st_state_start_timestamp which marks the beginning TS of
> the backend's current state (st_state). We can look at that field along
> with the current and to-be-transitioned-to states inside
> pgstat_report_activity() when there is a transition away from
> STATE_RUNNING, STATE_IDLEINTRANSACTION or
> STATE_IDLEINTRANSACTION_ABORTED, in order to update pgStatActiveTime and
> pgStatTransactionIdleTime. We would also need to update those counters
> on disconnect/PGSTAT_STAT_INTERVAL timeout if the backend's current
> state was STATE_RUNNING, STATE_IDLEINTRANSACTION or
> STATE_IDLEINTRANSACTION_ABORTED (in pgstat_send_connstats())

Yes, that would be better.

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

I personally would want all my database connections to be closed by
the client, unless something unexpected happens.

> pgStatSessionReported is not required. We can glean this information by
> checking if the function local static last_report in
> pgstat_report_stat() is 0 and passing this on as another param
> "first_report" to pgstat_send_connstats().

Yes, that is better.

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

> * We can directly use PgBackendStatus.st_proc_start_timestamp for
> calculating m_session_time. We can also choose to report session uptime
> even when the report is for the not-disconnect case
> (PGSTAT_STAT_INTERVAL elapsed). No reason why not. Then we would need to
> pass in the value of last_report to pgstat_send_connstats() -> calculate
> m_session_time to be number of time units from
> PgBackendStatus.st_proc_start_timestamp for the first report and then
> number of time units from the last_report for all subsequent reports.

Yes, that would make for better statistics, since client connections
can last quite long.

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

Thanks again!

Yours,
Laurenz Albe

Re: Add session statistics to pg_stat_database

[Soumyadeep Chakraborty]

Hello Laurenz,

Thanks for submitting this! Please find my feedback below.

* 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?

* pgstat_send_connstats(): How about renaming the "force" argument to
"disconnected"?

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

Also, some of these fields are not required:

I don't think we need pgStatActiveStart and pgStatTransactionIdleStart -
instead of these two we could use
PgBackendStatus.st_state_start_timestamp which marks the beginning TS of
the backend's current state (st_state). We can look at that field along
with the current and to-be-transitioned-to states inside
pgstat_report_activity() when there is a transition away from
STATE_RUNNING, STATE_IDLEINTRANSACTION or
STATE_IDLEINTRANSACTION_ABORTED, in order to update pgStatActiveTime and
pgStatTransactionIdleTime. We would also need to update those counters
on disconnect/PGSTAT_STAT_INTERVAL timeout if the backend's current
state was STATE_RUNNING, STATE_IDLEINTRANSACTION or
STATE_IDLEINTRANSACTION_ABORTED (in pgstat_send_connstats())

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

pgStatSessionReported is not required. We can glean this information by
checking if the function local static last_report in
pgstat_report_stat() is 0 and passing this on as another param
"first_report" to pgstat_send_connstats().


* PGSTAT_FILE_FORMAT_ID needs to be updated when a stats collector data
structure changes and we had a change in PgStat_StatDBEntry.

* We can directly use PgBackendStatus.st_proc_start_timestamp for
calculating m_session_time. We can also choose to report session uptime
even when the report is for the not-disconnect case
(PGSTAT_STAT_INTERVAL elapsed). No reason why not. Then we would need to
pass in the value of last_report to pgstat_send_connstats() -> calculate
m_session_time to be number of time units from
PgBackendStatus.st_proc_start_timestamp for the first report and then
number of time units from the last_report for all subsequent reports.

* We would need to bump the catalog version since we have made
changes to system views. Refer: #define CATALOG_VERSION_NO


Regards,
Soumyadeep (VMware)

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Tue, 2020-08-11 at 13:53 +0200, I wrote:
> On Thu, 2020-07-23 at 18:16 +0500, Ahsan Hadi wrote:
> 
> > On Wed, Jul 8, 2020 at 4:17 PM Laurenz Albe  
> > wrote:
> > > Here is a patch that adds the following to pg_stat_database:
> > > - number of connections
> >
> > Is it expected behaviour to not count idle connections? The connection is 
> > included after it is aborted but not while it was idle.
> 
> Currently, the patch counts connections when they close.
> 
> I could change the behavior that they are counted immediately.

I have changed the code so that connections are counted immediately.

Attached is a new version.

Yours,
Laurenz Albe
From 6d9bfbd682a9f4723f030fdc461f731175f55f44 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Fri, 4 Sep 2020 17:30:24 +0200
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- number of connections
- number of sessions that were not disconnected regularly
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.
---
 doc/src/sgml/monitoring.sgml |  46 +
 src/backend/catalog/system_views.sql |   5 +
 src/backend/postmaster/pgstat.c  | 146 ++-
 src/backend/tcop/postgres.c  |   5 +
 src/backend/utils/adt/pgstatfuncs.c  |  78 ++
 src/include/catalog/pg_proc.dat  |  20 
 src/include/pgstat.h |  29 +-
 src/test/regress/expected/rules.out  |   5 +
 8 files changed, 332 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 673a0e73e4..aa5e22d213 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3514,6 +3514,52 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent in database sessions in this database, in milliseconds.
+  
+ 
+
+ 
+  
+   active_time double precision
+  
+  
+   Time spent executing SQL statements in this database, in milliseconds.
+  
+ 
+
+ 
+  
+   idle_in_transaction_time double precision
+  
+  
+   Time spent idling while in a transaction in this database, in milliseconds.
+  
+ 
+
+ 
+  
+   connections bigint
+  
+  
+   Number of connections established to this database.
+  
+ 
+
+ 
+  
+   aborted_sessions bigint
+  
+  
+   Number of database sessions to this database that did not end
+   with a regular client disconnection.
+  
+ 
+
  
   
stats_reset timestamp with time zone
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index ed4f3f142d..d8b28c7600 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -912,6 +912,11 @@ CREATE VIEW pg_stat_database AS
 pg_stat_get_db_checksum_last_failure(D.oid) AS checksum_last_failure,
 pg_stat_get_db_blk_read_time(D.oid) AS blk_read_time,
 pg_stat_get_db_blk_write_time(D.oid) AS blk_write_time,
+pg_stat_get_db_session_time(D.oid) AS session_time,
+pg_stat_get_db_active_time(D.oid) AS active_time,
+pg_stat_get_db_idle_in_transaction_time(D.oid) AS idle_in_transaction_time,
+pg_stat_get_db_connections(D.oid) AS connections,
+pg_stat_get_db_aborted_sessions(D.oid) AS aborted_sessions,
 pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset
 FROM (
 SELECT 0 AS oid, NULL::name AS datname
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 5f4b168fd1..12a7543554 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -247,6 +247,12 @@ static int	pgStatXactCommit = 0;
 static int	pgStatXactRollback = 0;
 PgStat_Counter pgStatBlockReadTime = 0;
 PgStat_Counter pgStatBlockWriteTime = 0;
+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;
 
 /* Record that's written to 2PC state file when pgstat state is persisted */
 typedef struct TwoPhasePgStatRecord
@@ -326,6 +332,7 @@ static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg);
 static void pgstat_send_funcstats(void);
 static void pgstat_send_slru(void);
 static

Re: Add session statistics to pg_stat_database

[Laurenz Albe]

On Thu, 2020-07-23 at 18:16 +0500, Ahsan Hadi wrote:
> On Wed, Jul 8, 2020 at 4:17 PM Laurenz Albe  wrote:
> > Here is a patch that adds the following to pg_stat_database:
> > - number of connections
> 
> Is it expected behaviour to not count idle connections? The connection is 
> included after it is aborted but not while it was idle.

Thanks for looking.

Currently, the patch counts connections when they close.
I could change the behavior that they are counted immediately.

Yours,
Laurenz Albe

Re: Add session statistics to pg_stat_database

[Ahsan Hadi]

On Wed, Jul 8, 2020 at 4:17 PM Laurenz Albe 
wrote:

> Here is a patch that adds the following to pg_stat_database:
> - number of connections
>

Is it expected behaviour to not count idle connections? The connection is
included after it is aborted but not while it was idle.


> - number of sessions that were not disconnected regularly
> - total time spent in database sessions
> - total time spent executing queries
> - total idle in transaction time
>
> This is useful to check if connection pooling is working.
> It also helps to estimate the size of the connection pool
> required to keep the database busy, which depends on the
> percentage of the transaction time that is spent idling.
>
> Yours,
> Laurenz Albe
>


-- 
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.h...@highgo.ca

Add session statistics to pg_stat_database

[Laurenz Albe]

Here is a patch that adds the following to pg_stat_database:
- number of connections
- number of sessions that were not disconnected regularly
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.

Yours,
Laurenz Albe
From: Laurenz Albe 
Date: Wed, 8 Jul 2020 13:12:42 +0200
Subject: [PATCH] Add session statistics to pg_stat_database

If "track_counts" is active, track the following per database:
- number of connections
- number of sessions that were not disconnected regularly
- total time spent in database sessions
- total time spent executing queries
- total idle in transaction time

This is useful to check if connection pooling is working.
It also helps to estimate the size of the connection pool
required to keep the database busy, which depends on the
percentage of the transaction time that is spent idling.
---
 doc/src/sgml/monitoring.sgml |  46 +
 src/backend/catalog/system_views.sql |   5 +
 src/backend/postmaster/pgstat.c  | 138 ++-
 src/backend/tcop/postgres.c  |   5 +
 src/backend/utils/adt/pgstatfuncs.c  |  78 +++
 src/include/catalog/pg_proc.dat  |  20 
 src/include/pgstat.h |  29 +-
 src/test/regress/expected/rules.out  |   5 +
 8 files changed, 324 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index dfa9d0d641..da66808f02 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3519,6 +3519,52 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
   
  
 
+ 
+  
+   session_time double precision
+  
+  
+   Time spent in database sessions in this database, in milliseconds.
+  
+ 
+
+ 
+  
+   active_time double precision
+  
+  
+   Time spent executing SQL statements in this database, in milliseconds.
+  
+ 
+
+ 
+  
+   idle_in_transaction_time double precision
+  
+  
+   Time spent idling while in a transaction in this database, in milliseconds.
+  
+ 
+
+ 
+  
+   connections bigint
+  
+  
+   Number of connections established to this database.
+  
+ 
+
+ 
+  
+   aborted_sessions bigint
+  
+  
+   Number of database sessions to this database that did not end
+   with a regular client disconnection.
+  
+ 
+
  
   
stats_reset timestamp with time zone
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5314e9348f..64a4e5f0d4 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -909,6 +909,11 @@ CREATE VIEW pg_stat_database AS
 pg_stat_get_db_checksum_last_failure(D.oid) AS checksum_last_failure,
 pg_stat_get_db_blk_read_time(D.oid) AS blk_read_time,
 pg_stat_get_db_blk_write_time(D.oid) AS blk_write_time,
+pg_stat_get_db_session_time(D.oid) AS session_time,
+pg_stat_get_db_active_time(D.oid) AS active_time,
+pg_stat_get_db_idle_in_transaction_time(D.oid) AS idle_in_transaction_time,
+pg_stat_get_db_connections(D.oid) AS connections,
+pg_stat_get_db_aborted_sessions(D.oid) AS aborted_sessions,
 pg_stat_get_db_stat_reset_time(D.oid) AS stats_reset
 FROM (
 SELECT 0 AS oid, NULL::name AS datname
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index c022597bc0..7b62028358 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -247,6 +247,11 @@ static int	pgStatXactCommit = 0;
 static int	pgStatXactRollback = 0;
 PgStat_Counter pgStatBlockReadTime = 0;
 PgStat_Counter pgStatBlockWriteTime = 0;
+static TimestampTz pgStatActiveStart = DT_NOBEGIN;
+static PgStat_Counter pgStatActiveTime = 0;
+static TimestampTz pgStatTransactionIdleStart = DT_NOBEGIN;
+static PgStat_Counter pgStatTransactionIdleTime = 0;
+bool pgStatSessionDisconnected = false;
 
 /* Record that's written to 2PC state file when pgstat state is persisted */
 typedef struct TwoPhasePgStatRecord
@@ -326,6 +331,7 @@ static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg);
 static void pgstat_send_funcstats(void);
 static void pgstat_send_slru(void);
 static HTAB *pgstat_collect_oids(Oid catalogid, AttrNumber anum_oid);
+static void pgstat_send_connstats(bool force);
 
 static PgStat_TableStatus *get_tabstat_entry(Oid rel_id, bool isshared);
 
@@ -359,6 +365,7 @@ static void pgstat_recv_funcpurge(PgStat_MsgFuncpurge *msg, int len);
 static void pgstat_recv_recoveryconflict(PgStat_MsgRecove