Re: Feature improvement for pg_stat_statements

2020-12-14 23:01 に Fujii Masao さんは書きました:
> On 2020/12/14 18:17, Seino Yuki wrote:
>> 2020-12-09 21:14 に Fujii Masao さんは書きました:
>>> On 2020/12/09 20:42, Li Japin wrote:
>>>> Hi,
>>>> 
>>>>> On Dec 9, 2020, at 6:37 PM, Seino Yuki <seinoyu@oss.nttdata.com 
>>>>> <mailto:seinoyu@oss.nttdata.com>> wrote:
>>>>> 
>>>>> 2020-12-01 01:04 に Fujii Masao さんは書きました:
>>>>>> On 2020/11/30 23:05, Seino Yuki wrote:
>>>>>>> 2020-11-30 15:02 に Fujii Masao さんは書きました:
>>>>>>>> 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/ 
>>>>>>>>>>>>> <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/ 
>>>>>>>>>>> <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,
>>>>>>> Thanks for the new comment.
>>>>>>> I got the following pointers earlier.
>>>>>>>> +    reset_ts = GetCurrentTimestamp();
>>>>>>>> GetCurrentTimestamp() needs to be called only when all the 
>>>>>>>> entries
>>>>>>>> are removed. But it's called even in other cases.
>>>>>>> Which do you think is better? I think the new pointing out is 
>>>>>>> better,
>>>>>>> because entry_reset is not likely to be called often.
>>>>>> I was thinking that GetCurrentTimestamp() should be called before
>>>>>> pgss->lock lwlock is taken, only when all three arguments userid, 
>>>>>> dbid
>>>>>> and queryid are zero. But on second thought, we should call
>>>>>> GetCurrentTimestamp() and reset the stats, after the following 
>>>>>> codes?
>>>>>> /* All entries are removed? */
>>>>>> if (num_entries != num_remove)
>>>>>> goto release_lock;
>>>>>> That is, IMO that even when pg_stat_statements_reset() with 
>>>>>> non-zero
>>>>>> arguments is executed, if it removes all the entries, we should 
>>>>>> reset
>>>>>> the stats. Thought?
>>>>>> Regards,
>>>>> 
>>>>> +1.Fixed the patch.
>>>>> We tested various reset patterns and they seemed to be fine.
>>>>> 
>>>> 
>>>> Since BlessTupleDesc() is for SRFs according to the comments, I 
>>>> think, we can
>>>> remove it here.  Correct?
>>>> 
>>>> +       if (get_call_result_type(fcinfo, NULL, &tupdesc) != 
>>>> TYPEFUNC_COMPOSITE)
>>>> +               elog(ERROR, "return type must be a row type");
>>>> +
>>>> +       tupdesc = BlessTupleDesc(tupdesc);
>>> 
>>> Here are other comments from me:
>>> 
>>> The patch seems to contain whitespace errors.
>>> You can see them by "git diff --check".
>>> 
>>> +      <para>
>>> +        Time at which all statistics in the pg_stat_statements view
>>> were last reset.
>>> +      </para></entry>
>>> 
>>> "pg_stat_statements" in the above should be enclosed with
>>> <structname> and </structname>.
>>> 
>>> Regards,
>> 
>> Thank you for your comments, Mr. Fujii and Mr. Li.
>> I've posted a patch reflecting your comments.
>> Please check it out.
> 
> Thanks for updating the patch!
> 
> +    reset_ts = GetCurrentTimestamp();
> +    /* Reset global statistics for pg_stat_statements */
> +    {
> +        volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
> +
> +        SpinLockAcquire(&s->mutex);
> +        s->stats.stats_reset = reset_ts;    /* reset execution time */
> +        SpinLockRelease(&s->mutex);
> 
> This code makes me think that "dealloc" field also should be reset at
> the same time together, i.e., whenever all entries are removed.
> Otherwise, even when pg_stat_statements_reset() with non-zero
> arguments discards all statistics, "dealloc" field in
> pg_stat_statements_info is not reset. Only pg_stat_statements_reset(0,
> 0, 0) resets "dealloc" field. That seems confusing. Thought?
> 
> I updated the patch so that "dealloc" field is also reset whenever all
> entries are removed. Attached is the updated version of the patch.
> 
> +    result_tuple = heap_form_tuple(tupdesc, values, nulls);
> +    return HeapTupleGetDatum(result_tuple);
> 
> Isn't it better to use PG_RETURN_DATUM() here, instead? I applied this
> change to the patch.
> 
> I also applied some cosmetic changes to the patch. Could you review
> this version?
> 
> 
> Regards,

Thank you for providing the patch.

I have checked the operation.
There was no problem.

Regards.