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