Re: Feature improvement for pg_stat_statements

On 2020/12/17 22:59, Seino Yuki wrote:
> 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(at)oss(dot)nttdata(dot)com <mailto:seinoyu(at)oss(dot)nttdata(dot)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.

So I pushed the patch. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION