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

> 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/

> Why is this assertion check necessary? It seems not necessary.
As indicated, it is unnecessary and will be removed.

> 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?
Fix pgssSharedState to include pgssInfoCounters . The related parts were 
also corrected accordingly.

> PGSS_FILE_HEADER needs to be changed since the patch changes
> the format of pgss file?
The value of PGSS_FILE_HEADER has been updated.

> Why does feof(file) need to be called here?
As indicated, it is unnecessary and will be removed.

> Why is the second "{" necessary? It seems redundant.
As indicated, it is unnecessary and will be removed.
But I left the {} in pg_stat_statements_info() to make the shared memory 
edit part explicit.

> Why does Int64GetDatum() need to be called here? It seems not 
> necessary.
As indicated, it is unnecessary and will be removed.

> 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?
As indicated, it is unnecessary and will be removed.

Regards.