Re: Add session statistics to pg_stat_database

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/