2020-12-14 23:01 に Fujii Masao さんは書きました:
On 2020/12/14 18:17, Seino Yuki wrote:
2020-12-09 21:14 に Fujii Masao さんは書きました:
On 2020/12/09 20:42, Li Japin wrote:
Hi,
On Dec 9, 2020, at 6:37 PM, Seino Yuki <sein...@oss.nttdata.com
<mailto:sein...@oss.nttdata.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,
Thank you for your comments, Mr. Fujii and Mr. Li.
I've posted a patch reflecting your comments.
Please check it out.
Thanks for updating the patch!
+ reset_ts = GetCurrentTimestamp();
+ /* Reset global statistics for pg_stat_statements */
+ {
+ volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
+
+ SpinLockAcquire(&s->mutex);
+ s->stats.stats_reset = reset_ts; /* reset execution time */
+ SpinLockRelease(&s->mutex);
This code makes me think that "dealloc" field also should be reset at
the same time together, i.e., whenever all entries are removed.
Otherwise, even when pg_stat_statements_reset() with non-zero
arguments discards all statistics, "dealloc" field in
pg_stat_statements_info is not reset. Only pg_stat_statements_reset(0,
0, 0) resets "dealloc" field. That seems confusing. Thought?
I updated the patch so that "dealloc" field is also reset whenever all
entries are removed. Attached is the updated version of the patch.
+ result_tuple = heap_form_tuple(tupdesc, values, nulls);
+ return HeapTupleGetDatum(result_tuple);
Isn't it better to use PG_RETURN_DATUM() here, instead? I applied this
change to the patch.
I also applied some cosmetic changes to the patch. Could you review
this version?
Regards,
Thank you for providing the patch.
I have checked the operation.
There was no problem.
Regards.
