Re: [PATCH] Add features to pg_stat_statements

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.