Thread: Add session statistics to pg_stat_database

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

On Thu, 2020-07-23 at 18:16 +0500, Ahsan Hadi wrote:
> On Wed, Jul 8, 2020 at 4:17 PM Laurenz Albe <laurenz.albe@cybertec.at> 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
itwas 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

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 <laurenz.albe@cybertec.at> 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
whileit 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

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)

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

On Tue, Sep 29, 2020 at 2:44 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:


> > * Are we trying to capture ONLY client initiated disconnects in
> > m_aborted (we are not handling other disconnects by not accounting for
> > EOF..like if psql was killed)? If yes, why?
>
> I thought it was interesting to know how many database sessions are
> ended regularly as opposed to ones that get killed or end by unexpected
> client death.

It may very well be. It would also be interesting to find out how many
connections are still open on the database (something we could easily
glean if we had the number of all disconnects, client-initiated or
unnatural). Maybe we could have both?

m_sessions_disconnected;
m_sessions_killed;

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

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

/*
 * Updated by pgstat_count_buffer_*_time macros
 */
extern PgStat_Counter pgStatBlockReadTime;
extern PgStat_Counter pgStatBlockWriteTime;

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

See my comment above about client-initiated and unnatural disconnects.

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

Fair.

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

Fair.


Regards,
Soumyadeep (VMware)

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

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

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

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

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

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

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

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


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

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

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

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

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

Yes, the patch has bit-rotted.

Attached is v3 with improvements.

Yours,
Laurenz Albe

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

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, November 12, 2020 9:31 AM, Laurenz Albe <laurenz.albe@cybertec.at> wrote:

> I wrote:
>
> > On Tue, 2020-11-10 at 15:03 +0000, 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 "<!ENTITY version \"14devel\">"; \
>
>     echo "<!ENTITY majorversion \\"14\\">"; \\
>
>
> } > 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
> <book id="postgres">
>
>                      ^
>
>
> postgres.sgml:24: element title: validity error : No declaration for element title
> <title>PostgreSQL &version; Documentation</title>
>
> 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

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

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

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 <closed by correct disconnect>, <closed by abnormal disconnect>, <closed by admin>,
>  <crash>?
> 
> 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 elsewhere :^)

I'll have a go at "closed by admin", meanwhile here is patch v7 with the renaming
"session_count -> sessions".

Yours,
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

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

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

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

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.
// }<tab><tab><tab>Pgstat_MsgConn;



Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

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.
> // }<tab><tab><tab>Pgstat_MsgConn;

Thanks for the feedback.

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

Yours,
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.
> 
> // }<tab><tab><tab>Pgstat_MsgConn;

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

Patch version 11 is attached.

Yours,
Laurenz Albe

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.
>> 
>> // }<tab><tab><tab>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

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

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

On Fri, Jan 8, 2021 at 10:34 AM Laurenz Albe <laurenz.albe@cybertec.at> 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_<x>_time members in the struct to just
total_<x>_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/

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_<x>_time members in the struct to just
> total_<x>_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

On Mon, Jan 18, 2021 at 5:11 PM Laurenz Albe <laurenz.albe@cybertec.at> 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_<x>_time members in the struct to just
> > total_<x>_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/