Subject: Re: [PATCH] Add features to pg_stat_statements
To: Seino Yuki <seinoyu@oss.nttdata.com>
Cc: pgsql-hackers@lists.postgresql.org
References: <0d9f1107772cf5c3f954e985464c7298@oss.nttdata.com>
 <dcb63417e592d5966c19c04f90db0aef@oss.nttdata.com>
 <160250511275.1171.7692218521107860164.pgcf@coridan.postgresql.org>
 <369018a4-746c-6914-4983-1b79aa9fdd37@oss.nttdata.com>
 <4ab1bf5ab895d44ff0a3b3d06d1f85b2@oss.nttdata.com>
 <5d13be75-ed32-c500-f029-5838ba77d6c9@oss.nttdata.com>
 <2f749a3bd1b49f1d282abf59619fe1f5@oss.nttdata.com>
 <050fd52dcddac965ad24a636c25a5611@oss.nttdata.com>
 <2e9d596e-5b33-3368-682b-87431b65ce8f@oss.nttdata.com>
 <f34470e5e1d409a99eb1617197a16102@oss.nttdata.com>
 <d456d878-e7ec-02f6-b5ad-45630207b5d8@oss.nttdata.com>
 <4ea5ffd114ede2f4f1702932f73a402c@oss.nttdata.com>
 <98cc08b5-3fd7-718a-cb71-4d7a2fbd7d10@oss.nttdata.com>
 <31f3c20c034ade4435629afc8fd3fb73@oss.nttdata.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <e77813f9-e0f8-9fe0-aeed-749c7d4bd3e8@oss.nttdata.com>
Date: Wed, 25 Nov 2020 17:07:48 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.5.0
MIME-Version: 1.0
In-Reply-To: <31f3c20c034ade4435629afc8fd3fb73@oss.nttdata.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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