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
