From: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Seino Yuki <seinoyu(at)oss(dot)nttdata(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Cc: | horikyota(dot)ntt(at)gmail(dot)com |
Subject: | Re: Feature improvement for pg_stat_statements |
Date: | 2020-11-27 13:39:27 |
Message-ID: | 0024895c-c951-c01d-7526-84d285dd26d2@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2020/11/27 21:39, Seino Yuki wrote:
> 2020-11-27 21:37 に Seino Yuki さんは書きました:
>> 2020-11-16 12:28 に Seino Yuki さんは書きました:
>>> Due to similar fixed, we have also merged the patches discussed in the
>>> following thread.
>>> https://commitfest.postgresql.org/30/2736/
>>> The patch is posted on the 2736 side of the thread.
>>>
>>> Regards.
>>
>
> I forgot the attachment and will resend it.
>
> The following patches have been committed and we have created a
> compliant patch for them.
> https://commitfest.postgresql.org/30/2736/
>
> Please confirm.
Thanks for updating the patch! Here are review comments from me.
+ OUT reset_exec_time TIMESTAMP WITH TIME ZONE
I prefer "stats_reset" as the name of this column for the sake of
consistency with the similar column in other stats views like
pg_stat_database.
Isn't it better to use lower cases for "TIMESTAMP WITH TIME ZONE"
because other DDLs in pg_stat_statements do that for the declaraion
of data type?
@@ -565,6 +568,8 @@ pgss_shmem_startup(void)
pgss->n_writers = 0;
pgss->gc_count = 0;
pgss->stats.dealloc = 0;
+ pgss->stats.reset_exec_time = 0;
+ pgss->stats.reset_exec_time_isnull = true;
The reset time should be initialized with GetCurrentTimestamp() instead
of 0? If so, I think that we can completely get rid of reset_exec_time_isnull.
+ memset(nulls, 0, sizeof(nulls));
If some entries in "values[]" may not be set, "values[]" also needs to
be initialized with 0.
MemSet() should be used, instead?
+ /* Read dealloc */
+ values[0] = stats.dealloc;
Int64GetDatum() should be used here?
+ reset_ts = GetCurrentTimestamp();
GetCurrentTimestamp() needs to be called only when all the entries
are removed. But it's called even in other cases.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2020-11-27 13:40:57 | Re: Add Information during standby recovery conflicts |
Previous Message | osumi.takamichi@fujitsu.com | 2020-11-27 13:34:49 | RE: Disable WAL logging to speed up data loading |