On 2021-01-08 00:47, Laurenz Albe wrote:
> On Fri, 2020-12-25 at 20:28 +0900, Masahiro Ikeda wrote:
>> As a user, I want this feature to know whether
>> clients' session activities are as expected.
>>
>> I have some comments about the patch.
>
> Thanks you for the thorough review!
Thanks for updating the patch!
>> 1. pg_proc.dat
>>
>> The unit of "session time" and so on says "in seconds".
>> But, is "in milliseconds" right?
>
> You are right. Fixed.
>
>> 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.
>> 3. pgstat.h
>>
>> The comment of PgStat_MsgConn says "Sent by pgstat_connection".
>> I thought "pgstat_connection" is a function, but it doesn't exist.
>>
>> Is "Sent by the backend" right?
>
> The function was renamed and is now called "pgstat_send_connstats".
>
> But you are right, I might as well match the surrounding code and
> write "Sent by the backend".
>
>> Although this is a trivial thing, the following row has too many tabs.
>>
>> Other structs have only one space.
>>
>> // }<tab><tab><tab>Pgstat_MsgConn;
>
> Yes, I messed that up during the pgindent run. Fixed.
>
> Patch version 11 is attached.
There are some following codes in pgstatfuncs.c.
int64 result = 0.0;
But, I think the following is better.
int64 result = 0;
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?
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
