Re: [PATCH] Add features to pg_stat_statements - Mailing list pgsql-hackers

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,

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