Re: [PATCH] Add features to pg_stat_statements

On 2020/11/25 17:07, Fujii Masao wrote:
>
>
> On 2020/11/25 15:40, Seino Yuki wrote:
>> 2020-11-25 13:13 に Fujii Masao さんは書きました:
>>> On 2020/11/25 12:02, Seino Yuki wrote:
>>>> 2020-11-17 01:46 に Fujii Masao さんは書きました:
>>>>> On 2020/11/16 12:22, Seino Yuki wrote:
>>>>>>> Thanks for updating the patch!
>>>>>>>
>>>>>>> +        pgss_info->dealloc = 0;
>>>>>>> +        SpinLockInit(&pgss_info->mutex);
>>>>>>> +        Assert(pgss_info->dealloc == 0);
>>>>>>>
>>>>>>> Why is this assertion check necessary? It seems not necessary.
>>>>>>>
>>>>>>> +    {
>>>>>>> +        Assert(found == found_info);
>>>>>>>
>>>>>>> Having pgssSharedState and pgssInfoCounters separately might make
>>>>>>> the code a bit more complicated like the above? If this is true, what about
>>>>>>> including pgssInfoCounters in pgssSharedState?
>>>>>>>
>>>>>>> PGSS_FILE_HEADER needs to be changed since the patch changes
>>>>>>> the format of pgss file?
>>>>>>>
>>>>>>> +    /* Read pgss_info */
>>>>>>> +    if (feof(file) == 0)
>>>>>>> +        if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
>>>>>>> +            goto read_error;
>>>>>>>
>>>>>>> Why does feof(file) need to be called here?
>>>>>>>
>>>>>>> +pgss_info_update(void)
>>>>>>> +{
>>>>>>> +    {
>>>>>>>
>>>>>>> Why is the second "{" necessary? It seems redundant.
>>>>>>>
>>>>>>> +pgss_info_reset(void)
>>>>>>> +{
>>>>>>> +    {
>>>>>>>
>>>>>>> Same as above.
>>>>>>>
>>>>>>> +pg_stat_statements_info(PG_FUNCTION_ARGS)
>>>>>>> +{
>>>>>>> +    int64        d_count = 0;
>>>>>>> +    {
>>>>>>>
>>>>>>> Same as above.
>>>>>>>
>>>>>>> +        SpinLockAcquire(&c->mutex);
>>>>>>> +        d_count = Int64GetDatum(c->dealloc);
>>>>>>> +        SpinLockRelease(&c->mutex);
>>>>>>>
>>>>>>> Why does Int64GetDatum() need to be called here? It seems not necessary.
>>>>>>>
>>>>>>> +   <varlistentry>
>>>>>>> +    <term>
>>>>>>> +     <function>pg_stat_statements_info() returns bigint</function>
>>>>>>> +     <indexterm>
>>>>>>> +      <primary>pg_stat_statements_info</primary>
>>>>>>> +     </indexterm>
>>>>>>> +    </term>
>>>>>>>
>>>>>>> Isn't it better not to expose pg_stat_statements_info() function in the
>>>>>>> document because pg_stat_statements_info view is enough and there
>>>>>>> seems no use case for the function?
>>>>>>>
>>>>>>> Regards,
>>>>>>
>>>>>> Thanks for the comment.
>>>>>> I'll post a fixed patch.
>>>>>> Due to similar fixed, we have also merged the patches discussed in the following thread.
>>>>>> https://commitfest.postgresql.org/30/2738/
>>>>>
>>>>> I agree that these two patches should use the same infrastructure
>>>>> because they both try to add the global stats for pg_stat_statements.
>>>>> But IMO they should not be merged to one patch. It's better to
>>>>> develop them one by one for ease of review. Thought?
>>>>>
>>>>> So I extracted the "dealloc" part from the merged version of your patch.
>>>>> Also I refactored the code and applied some cosmetic changes into
>>>>> the patch. Attached is the updated version of the patch that implements
>>>>> only "dealloc" part. Could you review this version?
>>>>>
>>>>> Regards,
>>>>
>>>> Thank you for posting the new patch.
>>>>
>>>> I checked "regression test" and "document" and "operation of the view".
>>>> No particular problems were found.
>>>
>>> Thanks for the review and test!
>>> So you think that this patch can be marked as ready for committer?
>>>
>>>
>>>>
>>>> I just want to check one thing: will the log output be unnecessary this time?
>>>> Quotes from v2.patch
>>>
>>> I'm not sure if it's really good idea to add this log message.
>>> If we adopt that logging, in the system where pgss entries are deallocated
>>> very frequently, that message also would be logged very frequently.
>>> Such too many log messages might be noisy to users. To address this issue,
>>> we may want to add new parameter that controls whether log message is
>>> emitted or not when entries are deallocated. But that parameter sounds
>>> too specific... Thought?
>>>
>>> Regards,
>>
>>
>>> Thanks for the review and test!
>>> So you think that this patch can be marked as ready for committer?
>>
>> Updated status to ready for committer.
>
> Thanks!

I modified the docs a bit and pushed the patch. Thanks!

Regards,

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