Add session statistics to pg_stat_database

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

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

0001-Add-session-statistics-to-pg_stat_database.patch (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add session statistics to pg_stat_database

Ahsan Hadi-2

On Wed, Jul 8, 2020 at 4:17 PM Laurenz Albe <[hidden email]> 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: [hidden email]
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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

0001-Add-session-statistics-to-pg_stat_database.v2.patch (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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)


Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

Re: Add session statistics to pg_stat_database

Soumyadeep Chakraborty
On Tue, Sep 29, 2020 at 2:44 AM Laurenz Albe <[hidden email]> 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)


Reply | Threaded
Open this post in threaded view
|

Re: Add session statistics to pg_stat_database

Masahiro Ikeda
In reply to this post by Laurenz Albe
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


Reply | Threaded
Open this post in threaded view
|

Re: Add session statistics to pg_stat_database

Laurenz Albe
In reply to this post by Soumyadeep Chakraborty
On Fri, 2020-10-02 at 15:10 -0700, Soumyadeep Chakraborty wrote:

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

0001-Add-session-statistics-to-pg_stat_database.v3.patch (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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.

+      <para>
+       Time spent in database sessions in this database, in milliseconds.
+      </para></entry>

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

+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>connections</structfield> <type>bigint</type>
+      </para>
+      <para>
+       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.


Reply | Threaded
Open this post in threaded view
|

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.
>
> +      <para>
> +       Time spent in database sessions in this database, in milliseconds.
> +      </para></entry>
>
> 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.

> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>connections</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       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

0001-Add-session-statistics-to-pg_stat_database.v4.patch (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add session statistics to pg_stat_database

Ahsan Hadi-2
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 <[hidden email]> 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.
>
> +      <para>
> +       Time spent in database sessions in this database, in milliseconds.
> +      </para></entry>
>
> 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.

> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>connections</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       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: [hidden email]