Subject: Re: [PATCH] Add features to pg_stat_statements
To: Yuki Seino <seinoyu@oss.nttdata.com>, pgsql-hackers@lists.postgresql.org
Cc: yuta katsuragi <btkatsuragiyu@oss.nttdata.com>
References: <0d9f1107772cf5c3f954e985464c7298@oss.nttdata.com>
 <dcb63417e592d5966c19c04f90db0aef@oss.nttdata.com>
 <160250511275.1171.7692218521107860164.pgcf@coridan.postgresql.org>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <369018a4-746c-6914-4983-1b79aa9fdd37@oss.nttdata.com>
Date: Thu, 22 Oct 2020 01:31:11 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.12.1
MIME-Version: 1.0
In-Reply-To: 
 <160250511275.1171.7692218521107860164.pgcf@coridan.postgresql.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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