MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Wed, 25 Nov 2020 15:40:36 +0900
From: Seino Yuki <seinoyu@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Add features to pg_stat_statements
In-Reply-To: <98cc08b5-3fd7-718a-cb71-4d7a2fbd7d10@oss.nttdata.com>
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>
Message-ID: <31f3c20c034ade4435629afc8fd3fb73@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.1.9
Precedence: bulk

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.

> 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.
However, most users may not be that aware of it.
Let's not add a log this time.

Regards.