Horton

Thread

Commits

  1. pg_stat_statements: Track number of times pgss entries were deallocated.

  1. [PATCH] Add features to pg_stat_statements

    btkatsuragiyu <btkatsuragiyu@oss.nttdata.com> — 2020-09-18T08:53:44Z

    This is a proposal to add some features to pg_stat_statements.
Attached is the patch of this.

pg_stat_statements uses a hash table to hold statistics,
and the maximum number of its entries can be configured through 
pg_stat_statements.max.
When the number of entries exceeds the pg_stat_statements.max,
pg_stat_statements deallocate existing entries.

Currently, it is impossible to know how many times/when this 
deallocation happened.
But, with this information, more detailed performance analysis would be 
possible.
So, this patch provides access to this information.

This patch provides a view (pg_stat_statements_ctl) and
a function (pg_stat_statements_ctl).
The pg_stat_statements_ctl view is defined
in terms of a function also named pg_stat_statements_ctl.

The entry of pg_stat_statements_ctl view is removed
when pg_stat_statements_reset(0,0,0) is called.
Maybe, it is convenient to provide a function named 
pg_stat_statements_ctl_reset
that removes only the entry of pg_stat_statements_ctl.

The following is an example of this feature.
Here, a deallocation is shown with the INFO message.
pg_stat_statements_ctl tracks the number of deallocations
and timestamp of last deallocation.


testdb=# select * from pg_stat_statements_ctl;
  dealloc |         last_dealloc
---------+-------------------------------
        2 | 2020-09-18 16:55:31.128256+09
(1 row)

testdb=# select count(*) from test1;
2020-09-18 16:59:20.745 JST [3920] INFO:  deallocate
2020-09-18 16:59:20.745 JST [3920] STATEMENT:  select count(*) from 
test1;
INFO:  deallocate
  count
-------
     90
(1 row)

testdb=# select * from pg_stat_statements_ctl;
  dealloc |         last_dealloc
---------+-------------------------------
        3 | 2020-09-18 16:59:20.745652+09
(1 row)


Regards,
Katsuragi Yuta

  2. Re: [PATCH] Add features to pg_stat_statements

    Julien Rouhaud <rjuju123@gmail.com> — 2020-09-18T09:49:44Z

    Hi,

On Fri, Sep 18, 2020 at 10:54 AM Katsuragi Yuta
<btkatsuragiyu@oss.nttdata.com> wrote:
>
> This is a proposal to add some features to pg_stat_statements.
> Attached is the patch of this.
>
> pg_stat_statements uses a hash table to hold statistics,
> and the maximum number of its entries can be configured through
> pg_stat_statements.max.
> When the number of entries exceeds the pg_stat_statements.max,
> pg_stat_statements deallocate existing entries.
>
> Currently, it is impossible to know how many times/when this
> deallocation happened.
> But, with this information, more detailed performance analysis would be
> possible.
> So, this patch provides access to this information.
>
> This patch provides a view (pg_stat_statements_ctl) and
> a function (pg_stat_statements_ctl).
> The pg_stat_statements_ctl view is defined
> in terms of a function also named pg_stat_statements_ctl.
>
> The entry of pg_stat_statements_ctl view is removed
> when pg_stat_statements_reset(0,0,0) is called.
> Maybe, it is convenient to provide a function named
> pg_stat_statements_ctl_reset
> that removes only the entry of pg_stat_statements_ctl.
>
> The following is an example of this feature.
> Here, a deallocation is shown with the INFO message.
> pg_stat_statements_ctl tracks the number of deallocations
> and timestamp of last deallocation.
>
>
> testdb=# select * from pg_stat_statements_ctl;
>   dealloc |         last_dealloc
> ---------+-------------------------------
>         2 | 2020-09-18 16:55:31.128256+09
> (1 row)
>
> testdb=# select count(*) from test1;
> 2020-09-18 16:59:20.745 JST [3920] INFO:  deallocate
> 2020-09-18 16:59:20.745 JST [3920] STATEMENT:  select count(*) from
> test1;
> INFO:  deallocate
>   count
> -------
>      90
> (1 row)
>
> testdb=# select * from pg_stat_statements_ctl;
>   dealloc |         last_dealloc
> ---------+-------------------------------
>         3 | 2020-09-18 16:59:20.745652+09
> (1 row)

I like it, this is especially important since this can lead to quite
huge overhead.  Did you consider also adding the cumulated number of
evicted entries?  This could be useful to know how to configure
pg_stat_statements.max.

  3. Re: [PATCH] Add features to pg_stat_statements

    legrand legrand <legrand_legrand@hotmail.com> — 2020-09-19T07:35:42Z

    +1 !

An other way is to log evictions, it provides informations about time and
amount :

	for (i = 0; i < nvictims; i++)
	{
		hash_search(pgssp_hash, &entries[i]->key, HASH_REMOVE, NULL);
	}

	pfree(entries);

	/* trace when evicting entries, if appening too often this can slow queries
...
	 * increasing pg_stat_sql_plans.max value could help */
	 ereport(LOG,
		(errmsg("pg_stat_sql_plans evicting %d entries", nvictims),
		errhidecontext(true), errhidestmt(true)));



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

  4. Re: [PATCH] Add features to pg_stat_statements

    btkatsuragiyu <btkatsuragiyu@oss.nttdata.com> — 2020-09-23T06:48:15Z

    On 2020-09-18 18:49, Julien Rouhaud wrote:
> Did you consider also adding the cumulated number of
> evicted entries?  This could be useful to know how to configure
> pg_stat_statements.max.

Thank you for your comments!
I overlooked the cumulated number of evicted entries.
This statistic looks important. But, I am not sure
if I should add this statistic to a view.
This is because I am not sure how to utilize the cumulated
number of evicted entries for configuring pg_stat_statements.max.

Not only providing a view but also logging evictions
along with the number of evicted entries might be a choice.
This idea is from legrand legrand [1].

[1] 
https://www.postgresql.org/message-id/1600500942767-0.post%40n3.nabble.com

Regards,
Katsuragi Yuta

  5. Re: [PATCH] Add features to pg_stat_statements

    Julien Rouhaud <rjuju123@gmail.com> — 2020-09-23T07:01:12Z

    On Wed, Sep 23, 2020 at 2:48 PM Katsuragi Yuta
<btkatsuragiyu@oss.nttdata.com> wrote:
>
> On 2020-09-18 18:49, Julien Rouhaud wrote:
> > Did you consider also adding the cumulated number of
> > evicted entries?  This could be useful to know how to configure
> > pg_stat_statements.max.
>
> Thank you for your comments!
> I overlooked the cumulated number of evicted entries.
> This statistic looks important. But, I am not sure
> if I should add this statistic to a view.
> This is because I am not sure how to utilize the cumulated
> number of evicted entries for configuring pg_stat_statements.max.

You're right, as the number of evicted entries isn't depending on the
number of entries that wouls been needed to contain the entirely
workload.

> Not only providing a view but also logging evictions
> along with the number of evicted entries might be a choice.
> This idea is from legrand legrand [1].

+1.  I'm wondering if logging each evicted entry, with its queryid,
would help to estimate the actual size of the normalised queries set.

  6. Re: [PATCH] Add features to pg_stat_statements

    btkatsuragiyu <btkatsuragiyu@oss.nttdata.com> — 2020-09-23T08:13:36Z

    On 2020-09-19 16:35, legrand legrand wrote:
> +1 !
> 
> An other way is to log evictions, it provides informations about time 
> and
> amount :
> 
> 	for (i = 0; i < nvictims; i++)
> 	{
> 		hash_search(pgssp_hash, &entries[i]->key, HASH_REMOVE, NULL);
> 	}
> 
> 	pfree(entries);
> 
> 	/* trace when evicting entries, if appening too often this can slow 
> queries
> ...
> 	 * increasing pg_stat_sql_plans.max value could help */
> 	 ereport(LOG,
> 		(errmsg("pg_stat_sql_plans evicting %d entries", nvictims),
> 		errhidecontext(true), errhidestmt(true)));
> 
> 
> 
> --
> Sent from: 
> https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Thank you for your comments!
In addition to providing a view that is like a summary of history,
logging might be a good choice.

Regards,
Katsuragi Yuta

  7. Re: [PATCH] Add features to pg_stat_statements

    btkatsuragiyu <btkatsuragiyu@oss.nttdata.com> — 2020-09-23T08:17:04Z

    On 2020-09-23 16:01, Julien Rouhaud wrote:
>> Not only providing a view but also logging evictions
>> along with the number of evicted entries might be a choice.
>> This idea is from legrand legrand [1].
> 
> +1.  I'm wondering if logging each evicted entry, with its queryid,
> would help to estimate the actual size of the normalised queries set.

I agree with the estimation of the actual size of the
query set is important.
It looks difficult to estimate the actual size of the
query set from queryid because queryid is a 64bit hash value.

Regards,
Katsuragi Yuta

  8. Re: [PATCH] Add features to pg_stat_statements

    legrand legrand <legrand_legrand@hotmail.com> — 2020-09-24T05:15:23Z

    Hi,
Both are probably not needed.
I would prefer it accessible in a view live

Event | date | victims 
Eviction...
Reset...
Part reset ...

As there are other needs to track reset times.
Regards
PAscal




--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

  9. Re: [PATCH] Add features to pg_stat_statements

    btkatsuragiyu <btkatsuragiyu@oss.nttdata.com> — 2020-09-24T07:32:51Z

    On 2020-09-24 14:15, legrand legrand wrote:
> Hi,
> Both are probably not needed.
> I would prefer it accessible in a view live
> 
> Event | date | victims
> Eviction...
> Reset...
> Part reset ...
> 
> As there are other needs to track reset times.

Let me confirm one thing.
Is the number of records of this view fixed to three?
Or, will a new record be appended every time
an event (Eviction or Reset or Part Reset) happens?

Regards,
Katsuragi Yuta

  10. Re: [PATCH] Add features to pg_stat_statements

    legrand legrand <legrand_legrand@hotmail.com> — 2020-09-24T09:55:06Z

    Not limited to 3, Like an history table.

Will have to think if data is saved at shutdown.



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

  11. Re: [PATCH] Add features to pg_stat_statements

    btkatsuragiyu <btkatsuragiyu@oss.nttdata.com> — 2020-09-25T05:46:00Z

    On 2020-09-24 18:55, legrand legrand wrote:
> Not limited to 3, Like an history table.
> 
> Will have to think if data is saved at shutdown.

Thanks.

This design might introduce some additional complexity
and things to be considered.
For example, the maximum size of "history table",
how to evict entries from the history table and how to manage
the information maintained by evicted entries.

Also, providing a history table looks similar to logging.

Providing the original view (# of dealloc and last_dealloc ts)
and optional logging looks a simple and better way.

Regards,
Katsuragi Yuta

  12. Re: [PATCH] Add features to pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-10-12T12:18:32Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

The patch applies cleanly and looks fine to me. It's a small detail, However wouldn't it be better if the following changes were made.

1.There are unnecessary difference lines 707 and 863 in "pg_stat_statements.c".
2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in "pg_stat_statements.c".
3."update_ctl" and "reset_ctl" are generic and illegible name.You might want to rename it something like "pgss_ctl_update".
4.To improve the readability of the source, why not change the function declaration option in "pg_stat_statements_ctl" from 
"AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME', 'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".

The new status of this patch is: Waiting on Author

  13. Re: [PATCH] Add features to pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-10-21T16:31:11Z

    
On 2020/10/12 21:18, Yuki Seino wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
> 
> The patch applies cleanly and looks fine to me. It's a small detail, However wouldn't it be better if the following changes were made.
> 
> 1.There are unnecessary difference lines 707 and 863 in "pg_stat_statements.c".
> 2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in "pg_stat_statements.c".
> 3."update_ctl" and "reset_ctl" are generic and illegible name.You might want to rename it something like "pgss_ctl_update".
> 4.To improve the readability of the source, why not change the function declaration option in "pg_stat_statements_ctl" from
> "AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME', 'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".
> 
> The new status of this patch is: Waiting on Author

Here are other comments from me.

-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql\

One space character needs to be added just before the character "\".

+--- Define pg_stat_statements_ctl

ISTM that this is not good name.
What about pg_stat_statements_info, _stats, _activity or something?

+	OUT dealloc integer,

The type of "dealloc" should be bigint, instead?

+	OUT last_dealloc TIMESTAMP WITH TIME ZONE

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

+LANGUAGE  C STRICT VOLATILE PARALLEL SAFE;

There are two space characters just after "LANGUAGE".
One space character should be removed from there.

+CREATE VIEW pg_stat_statements_ctl AS
+	SELECT * FROM pg_stat_statements_ctl();

If we rename the function, this view name also should be changed.

+GRANT SELECT ON pg_stat_statements TO PUBLIC;

"pg_stat_statements" should be "pg_stat_statement_xxx"?

Regards,

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

  14. Re: [PATCH] Add features to pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-10-28T08:31:14Z

    2020-10-22 01:31 に Fujii Masao さんは書きました:
> On 2020/10/12 21:18, Yuki Seino wrote:
>> The following review has been posted through the commitfest 
>> application:
>> make installcheck-world:  tested, passed
>> Implements feature:       tested, passed
>> Spec compliant:           tested, passed
>> Documentation:            tested, passed
>> 
>> The patch applies cleanly and looks fine to me. It's a small detail, 
>> However wouldn't it be better if the following changes were made.
>> 
>> 1.There are unnecessary difference lines 707 and 863 in 
>> "pg_stat_statements.c".
>> 2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in 
>> "pg_stat_statements.c".
>> 3."update_ctl" and "reset_ctl" are generic and illegible name.You 
>> might want to rename it something like "pgss_ctl_update".
>> 4.To improve the readability of the source, why not change the 
>> function declaration option in "pg_stat_statements_ctl" from
>> "AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME', 
>> 'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".
>> 
>> The new status of this patch is: Waiting on Author
> 
> Here are other comments from me.
> 
> -DATA = pg_stat_statements--1.4.sql \
> +DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql\
> 
> One space character needs to be added just before the character "\".
> 
> +--- Define pg_stat_statements_ctl
> 
> ISTM that this is not good name.
> What about pg_stat_statements_info, _stats, _activity or something?
> 
> +	OUT dealloc integer,
> 
> The type of "dealloc" should be bigint, instead?
> 
> +	OUT last_dealloc TIMESTAMP WITH TIME ZONE
> 
> Is this information really useful?
> If there is no valid use case for this, I'd like to drop it.
> Thought?
> 
> +LANGUAGE  C STRICT VOLATILE PARALLEL SAFE;
> 
> There are two space characters just after "LANGUAGE".
> One space character should be removed from there.
> 
> +CREATE VIEW pg_stat_statements_ctl AS
> +	SELECT * FROM pg_stat_statements_ctl();
> 
> If we rename the function, this view name also should be changed.
> 
> +GRANT SELECT ON pg_stat_statements TO PUBLIC;
> 
> "pg_stat_statements" should be "pg_stat_statement_xxx"?
> 
> Regards,


Thanks for the comment, Fujii-san.

I will post the patch again in the future to reflect your and my points.

However, let me confirm the following.
> Is this information really useful?
> If there is no valid use case for this, I'd like to drop it.
> Thought?

I thought it would be easy for users to see at a glance that if there is 
a case I assumed,
if the last modified date and time is old, there is no need to adjust at 
all, and if the
last modified date and time is recent, it would be easy for users to 
understand that the
parameters need to be adjusted.
What do you think?

Regards,
Seino Yuki

  15. Re: [PATCH] Add features to pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-10-28T12:14:33Z

    
On 2020/10/28 17:31, seinoyu wrote:
> 2020-10-22 01:31 に Fujii Masao さんは書きました:
>> On 2020/10/12 21:18, Yuki Seino wrote:
>>> The following review has been posted through the commitfest application:
>>> make installcheck-world:  tested, passed
>>> Implements feature:       tested, passed
>>> Spec compliant:           tested, passed
>>> Documentation:            tested, passed
>>>
>>> The patch applies cleanly and looks fine to me. It's a small detail, However wouldn't it be better if the following changes were made.
>>>
>>> 1.There are unnecessary difference lines 707 and 863 in "pg_stat_statements.c".
>>> 2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in "pg_stat_statements.c".
>>> 3."update_ctl" and "reset_ctl" are generic and illegible name.You might want to rename it something like "pgss_ctl_update".
>>> 4.To improve the readability of the source, why not change the function declaration option in "pg_stat_statements_ctl" from
>>> "AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME', 'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".
>>>
>>> The new status of this patch is: Waiting on Author
>>
>> Here are other comments from me.
>>
>> -DATA = pg_stat_statements--1.4.sql \
>> +DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql\
>>
>> One space character needs to be added just before the character "\".
>>
>> +--- Define pg_stat_statements_ctl
>>
>> ISTM that this is not good name.
>> What about pg_stat_statements_info, _stats, _activity or something?
>>
>> +    OUT dealloc integer,
>>
>> The type of "dealloc" should be bigint, instead?
>>
>> +    OUT last_dealloc TIMESTAMP WITH TIME ZONE
>>
>> Is this information really useful?
>> If there is no valid use case for this, I'd like to drop it.
>> Thought?
>>
>> +LANGUAGE  C STRICT VOLATILE PARALLEL SAFE;
>>
>> There are two space characters just after "LANGUAGE".
>> One space character should be removed from there.
>>
>> +CREATE VIEW pg_stat_statements_ctl AS
>> +    SELECT * FROM pg_stat_statements_ctl();
>>
>> If we rename the function, this view name also should be changed.
>>
>> +GRANT SELECT ON pg_stat_statements TO PUBLIC;
>>
>> "pg_stat_statements" should be "pg_stat_statement_xxx"?
>>
>> Regards,
> 
> 
> Thanks for the comment, Fujii-san.
> 
> I will post the patch again in the future to reflect your and my points.

Thanks!

> 
> However, let me confirm the following.
>> Is this information really useful?
>> If there is no valid use case for this, I'd like to drop it.
>> Thought?
> 
> I thought it would be easy for users to see at a glance that if there is a case I assumed,
> if the last modified date and time is old, there is no need to adjust at all, and if the
> last modified date and time is recent, it would be easy for users to understand that the
> parameters need to be adjusted.
> What do you think?

Even without the last deallocation timestamp, you can presume
when the deallocation of entries happened by periodically monitoring
the total number of deallocations and seeing those history. Or IMO
it's better to log whenever the deallocation happens as proposed upthread.
Then you can easily check the history of occurrences of deallocations
from the log file.

Regards,

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

  16. Re: [PATCH] Add features to pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-09T06:39:05Z

    >> 
>> However, let me confirm the following.
>>> Is this information really useful?
>>> If there is no valid use case for this, I'd like to drop it.
>>> Thought?
>> 
>> I thought it would be easy for users to see at a glance that if there 
>> is a case I assumed,
>> if the last modified date and time is old, there is no need to adjust 
>> at all, and if the
>> last modified date and time is recent, it would be easy for users to 
>> understand that the
>> parameters need to be adjusted.
>> What do you think?
> 
> Even without the last deallocation timestamp, you can presume
> when the deallocation of entries happened by periodically monitoring
> the total number of deallocations and seeing those history. Or IMO
> it's better to log whenever the deallocation happens as proposed 
> upthread.
> Then you can easily check the history of occurrences of deallocations
> from the log file.
> 
> Regards,

+1.I think you should output the deallocation history to the log as 
well.

In light of the above, I've posted a patch that reflects the points 
made.

Regards,

  17. Re: [PATCH] Add features to pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-09T09:02:53Z

    2020-11-09 15:39 に Seino Yuki さんは書きました:
>>> 
>>> However, let me confirm the following.
>>>> Is this information really useful?
>>>> If there is no valid use case for this, I'd like to drop it.
>>>> Thought?
>>> 
>>> I thought it would be easy for users to see at a glance that if there 
>>> is a case I assumed,
>>> if the last modified date and time is old, there is no need to adjust 
>>> at all, and if the
>>> last modified date and time is recent, it would be easy for users to 
>>> understand that the
>>> parameters need to be adjusted.
>>> What do you think?
>> 
>> Even without the last deallocation timestamp, you can presume
>> when the deallocation of entries happened by periodically monitoring
>> the total number of deallocations and seeing those history. Or IMO
>> it's better to log whenever the deallocation happens as proposed 
>> upthread.
>> Then you can easily check the history of occurrences of deallocations
>> from the log file.
>> 
>> Regards,
> 
> +1.I think you should output the deallocation history to the log as 
> well.
> 
> In light of the above, I've posted a patch that reflects the points 
> made.
> 
> Regards,

Sorry. There was a typo in the documentation correction.
I'll post a patch to fix it.

Regards,

  18. Re: [PATCH] Add features to pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-11-10T17:12:44Z

    
On 2020/11/09 18:02, Seino Yuki wrote:
> 2020-11-09 15:39 に Seino Yuki さんは書きました:
>>>>
>>>> However, let me confirm the following.
>>>>> Is this information really useful?
>>>>> If there is no valid use case for this, I'd like to drop it.
>>>>> Thought?
>>>>
>>>> I thought it would be easy for users to see at a glance that if there is a case I assumed,
>>>> if the last modified date and time is old, there is no need to adjust at all, and if the
>>>> last modified date and time is recent, it would be easy for users to understand that the
>>>> parameters need to be adjusted.
>>>> What do you think?
>>>
>>> Even without the last deallocation timestamp, you can presume
>>> when the deallocation of entries happened by periodically monitoring
>>> the total number of deallocations and seeing those history. Or IMO
>>> it's better to log whenever the deallocation happens as proposed upthread.
>>> Then you can easily check the history of occurrences of deallocations
>>> from the log file.
>>>
>>> Regards,
>>
>> +1.I think you should output the deallocation history to the log as well.
>>
>> In light of the above, I've posted a patch that reflects the points made.
>>
>> Regards,
> 
> Sorry. There was a typo in the documentation correction.
> I'll post a patch to fix it.

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,

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

  19. Re: [PATCH] Add features to pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-16T03:22:44Z

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

  20. Re: [PATCH] Add features to pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-11-16T16:46:52Z

    
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

  21. Re: [PATCH] Add features to pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-25T03:02:38Z

    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.

  22. Re: [PATCH] Add features to pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-11-25T04:13:56Z

    
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

  23. Re: [PATCH] Add features to pg_stat_statements

    Yuki Seino <seinoyu@oss.nttdata.com> — 2020-11-25T06:40:36Z

    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.

  24. Re: [PATCH] Add features to pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-11-25T08:07:48Z

    
On 2020/11/25 15:40, Seino Yuki wrote:
> 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.

Thanks!


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

Yes, but I guess that there are some users who cannot increase pgss.max
for some reasons (e.g., lack of memory). It seems annoying for them to
*always* log the deallocation of pgss entries.

Regards,

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

  25. Re: [PATCH] Add features to pg_stat_statements

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-11-26T12:22:11Z

    
On 2020/11/25 17:07, Fujii Masao wrote:
> 
> 
> On 2020/11/25 15:40, Seino Yuki wrote:
>> 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.
> 
> Thanks!

I modified the docs a bit and pushed the patch. Thanks!

Regards,

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