Re: Add session statistics to pg_stat_database - Mailing list pgsql-hackers
From | Masahiro Ikeda |
---|---|
Subject | Re: Add session statistics to pg_stat_database |
Date | |
Msg-id | d1e95fe03f7bb3a0f449aa9fcd0fca5b@oss.nttdata.com Whole thread Raw |
In response to | Re: Add session statistics to pg_stat_database (Laurenz Albe <laurenz.albe@cybertec.at>) |
List | pgsql-hackers |
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
pgsql-hackers by date: