Subject: Re: [PATCH] Add features to pg_stat_statements
To: Seino Yuki <seinoyu@oss.nttdata.com>
Cc: pgsql-hackers@lists.postgresql.org
References: <0d9f1107772cf5c3f954e985464c7298@oss.nttdata.com>
 <dcb63417e592d5966c19c04f90db0aef@oss.nttdata.com>
 <160250511275.1171.7692218521107860164.pgcf@coridan.postgresql.org>
 <369018a4-746c-6914-4983-1b79aa9fdd37@oss.nttdata.com>
 <4ab1bf5ab895d44ff0a3b3d06d1f85b2@oss.nttdata.com>
 <5d13be75-ed32-c500-f029-5838ba77d6c9@oss.nttdata.com>
 <2f749a3bd1b49f1d282abf59619fe1f5@oss.nttdata.com>
 <050fd52dcddac965ad24a636c25a5611@oss.nttdata.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <2e9d596e-5b33-3368-682b-87431b65ce8f@oss.nttdata.com>
Date: Wed, 11 Nov 2020 02:12:44 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.4.1
MIME-Version: 1.0
In-Reply-To: <050fd52dcddac965ad24a636c25a5611@oss.nttdata.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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