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
