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

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.

I just want to check one thing: will the log output be unnecessary this 
time?
Quotes from v2.patch
> 
> +    {
>         entry_dealloc();
> +        /* Update pgss_info */
> +        {
> +            volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
> +            SpinLockAcquire(&s->mutex);
> +            s->pgss_info.dealloc += 1;    /* increment dealloc count */
> +            SpinLockRelease(&s->mutex);
> +        }
> +        ereport(LOG,
> +            (errmsg("The information in pg_stat_statements has been 
> deallocated.")));
> +    }

Regards.