Subject: Re: [PATCH] Add features to pg_stat_statements
From: Fujii Masao <masao.fujii@oss.nttdata.com>
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>
 <e77813f9-e0f8-9fe0-aeed-749c7d4bd3e8@oss.nttdata.com>
Message-ID: <334b9b54-dcdb-5fc3-654c-b319da584e0b@oss.nttdata.com>
Date: Thu, 26 Nov 2020 21:22:11 +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: <e77813f9-e0f8-9fe0-aeed-749c7d4bd3e8@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 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