From: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Li Japin <japinli(at)hotmail(dot)com>, Seino Yuki <seinoyu(at)oss(dot)nttdata(dot)com> |
Cc: | "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "horikyota(dot)ntt(at)gmail(dot)com" <horikyota(dot)ntt(at)gmail(dot)com> |
Subject: | Re: Feature improvement for pg_stat_statements |
Date: | 2020-12-09 12:14:50 |
Message-ID: | 808d645e-b20c-c02c-0c39-224ba80426ca@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2020/12/09 20:42, Li Japin wrote:
> Hi,
>
>> On Dec 9, 2020, at 6:37 PM, Seino Yuki <seinoyu(at)oss(dot)nttdata(dot)com <mailto:seinoyu(at)oss(dot)nttdata(dot)com>> wrote:
>>
>> 2020-12-01 01:04 に Fujii Masao さんは書きました:
>>> On 2020/11/30 23:05, Seino Yuki wrote:
>>>> 2020-11-30 15:02 に Fujii Masao さんは書きました:
>>>>> On 2020/11/30 12:08, Seino Yuki wrote:
>>>>>> 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/ <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/ <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.
>>>>> Thanks for updating the patch! Here are another review comments.
>>>>> + <structfield>reset_exec_time</structfield> <type>timestamp
>>>>> with time zone</type>
>>>>> You forgot to update the column name in the doc?
>>>>> + Shows the time at which
>>>>> <function>pg_stat_statements_reset(0,0,0)</function> was last called.
>>>>> What about updating this to something like "Time at which all statistics
>>>>> in the pg_stat_statements view were last reset." for the sale of
>>>>> onsistency with the description about stats_reset column in other
>>>>> tats views?
>>>>> + /* Read stats_reset */
>>>>> + values[1] = stats.stats_reset;
>>>>> TimestampTzGetDatum() seems necessary.
>>>>> + reset_ts = GetCurrentTimestamp();
>>>>> /* Reset global statistics for pg_stat_statements */
>>>>> Isn't it better to call GetCurrentTimestamp() before taking
>>>>> an exclusive lwlock, in entry_reset()?
>>>>> Regards,
>>>> Thanks for the new comment.
>>>> I got the following pointers earlier.
>>>>> + reset_ts = GetCurrentTimestamp();
>>>>> GetCurrentTimestamp() needs to be called only when all the entries
>>>>> are removed. But it's called even in other cases.
>>>> Which do you think is better? I think the new pointing out is better,
>>>> because entry_reset is not likely to be called often.
>>> I was thinking that GetCurrentTimestamp() should be called before
>>> pgss->lock lwlock is taken, only when all three arguments userid, dbid
>>> and queryid are zero. But on second thought, we should call
>>> GetCurrentTimestamp() and reset the stats, after the following codes?
>>> /* All entries are removed? */
>>> if (num_entries != num_remove)
>>> goto release_lock;
>>> That is, IMO that even when pg_stat_statements_reset() with non-zero
>>> arguments is executed, if it removes all the entries, we should reset
>>> the stats. Thought?
>>> Regards,
>>
>> +1.Fixed the patch.
>> We tested various reset patterns and they seemed to be fine.
>>
>
> Since BlessTupleDesc() is for SRFs according to the comments, I think, we can
> remove it here. Correct?
>
> + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> + elog(ERROR, "return type must be a row type");
> +
> + tupdesc = BlessTupleDesc(tupdesc);
Here are other comments from me:
The patch seems to contain whitespace errors.
You can see them by "git diff --check".
+ <para>
+ Time at which all statistics in the pg_stat_statements view were last reset.
+ </para></entry>
"pg_stat_statements" in the above should be enclosed with
<structname> and </structname>.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From | Date | Subject | |
---|---|---|---|
Next Message | Greg Nancarrow | 2020-12-09 12:17:14 | Re: On login trigger: take three |
Previous Message | Amit Kapila | 2020-12-09 12:12:40 | Re: Parallel INSERT (INTO ... SELECT ...) |