Re: [PATCH] Add features to pg_stat_statements

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,

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