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