Re: Feature improvement for pg_stat_statements

2020-11-27 22:39 に Fujii Masao さんは書きました:
> 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,

Thanks for the review!
Fixed the patch.

I learned a lot, especially since I didn't know about MemSet().

Regards.