Re: Add session statistics to pg_stat_database

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