Re: Add session statistics to pg_stat_database

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