Re: Feature improvement for pg_stat_statements

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,

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