Re: [PATCH] Add features to pg_stat_statements

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'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?
>
> I think it's no problem to add the log after the user requests it.
> Usually I think you should tune pg_stat_statements.max if you have frequent deallocated.

Yes, but I guess that there are some users who cannot increase pgss.max
for some reasons (e.g., lack of memory). It seems annoying for them to
*always* log the deallocation of pgss entries.

Regards,

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