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(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.

Regards.