From: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Seino Yuki <seinoyu(at)oss(dot)nttdata(dot)com> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org, horikyota(dot)ntt(at)gmail(dot)com |
Subject: | Re: Feature improvement for pg_stat_statements |
Date: | 2020-11-30 06:02:03 |
Message-ID: | b94b0279-9112-68ce-bc1b-fb722ce72ace@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2020/11/30 12:08, Seino Yuki wrote:
> 2020-11-27 22:39 に Fujii Masao さんは書きました:
>> On 2020/11/27 21:39, Seino Yuki wrote:
>>> 2020-11-27 21:37 に Seino Yuki さんは書きました:
>>>> 2020-11-16 12:28 に Seino Yuki さんは書きました:
>>>>> Due to similar fixed, we have also merged the patches discussed in the
>>>>> following thread.
>>>>> https://commitfest.postgresql.org/30/2736/
>>>>> The patch is posted on the 2736 side of the thread.
>>>>>
>>>>> Regards.
>>>>
>>>
>>> I forgot the attachment and will resend it.
>>>
>>> The following patches have been committed and we have created a
>>> compliant patch for them.
>>> https://commitfest.postgresql.org/30/2736/
>>>
>>> Please confirm.
>>
>> Thanks for updating the patch! Here are review comments from me.
>>
>> + OUT reset_exec_time TIMESTAMP WITH TIME ZONE
>>
>> I prefer "stats_reset" as the name of this column for the sake of
>> consistency with the similar column in other stats views like
>> pg_stat_database.
>>
>> Isn't it better to use lower cases for "TIMESTAMP WITH TIME ZONE"
>> because other DDLs in pg_stat_statements do that for the declaraion
>> of data type?
>>
>> @@ -565,6 +568,8 @@ pgss_shmem_startup(void)
>> pgss->n_writers = 0;
>> pgss->gc_count = 0;
>> pgss->stats.dealloc = 0;
>> + pgss->stats.reset_exec_time = 0;
>> + pgss->stats.reset_exec_time_isnull = true;
>>
>> The reset time should be initialized with GetCurrentTimestamp() instead
>> of 0? If so, I think that we can completely get rid of reset_exec_time_isnull.
>>
>> + memset(nulls, 0, sizeof(nulls));
>>
>> If some entries in "values[]" may not be set, "values[]" also needs to
>> be initialized with 0.
>>
>> MemSet() should be used, instead?
>>
>> + /* Read dealloc */
>> + values[0] = stats.dealloc;
>>
>> Int64GetDatum() should be used here?
>>
>> + reset_ts = GetCurrentTimestamp();
>>
>> GetCurrentTimestamp() needs to be called only when all the entries
>> are removed. But it's called even in other cases.
>>
>> Regards,
>
> Thanks for the review!
> Fixed the patch.
Thanks for updating the patch! Here are another review comments.
+ <structfield>reset_exec_time</structfield> <type>timestamp with time zone</type>
You forgot to update the column name in the doc?
+ Shows the time at which <function>pg_stat_statements_reset(0,0,0)</function> was last called.
What about updating this to something like "Time at which all statistics
in the pg_stat_statements view were last reset." for the sale of
onsistency with the description about stats_reset column in other
tats views?
+ /* Read stats_reset */
+ values[1] = stats.stats_reset;
TimestampTzGetDatum() seems necessary.
+ reset_ts = GetCurrentTimestamp();
/* Reset global statistics for pg_stat_statements */
Isn't it better to call GetCurrentTimestamp() before taking
an exclusive lwlock, in entry_reset()?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-11-30 06:08:27 | Re: Improving spin-lock implementation on ARM. |
Previous Message | Thomas Munro | 2020-11-30 05:59:40 | Re: BUG #16663: DROP INDEX did not free up disk space: idle connection hold file marked as deleted |