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.
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
index 2019a4ffe0..3504ca7eb1 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
@@ -5,9 +5,10 @@
--- Define pg_stat_statements_info
CREATE FUNCTION pg_stat_statements_info(
- OUT dealloc bigint
+ OUT dealloc bigint,
+ OUT stats_reset timestamp with time zone
)
-RETURNS bigint
+RETURNS record
AS 'MODULE_PATHNAME'
LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 70cfdb2c9d..96b8d0135b 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -81,6 +81,7 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/timestamp.h"
PG_MODULE_MAGIC;
@@ -199,6 +200,7 @@ typedef struct Counters
typedef struct pgssGlobalStats
{
int64 dealloc; /* # of times entries were deallocated */
+ TimestampTz stats_reset; /* timestamp with all stats reset */
} pgssGlobalStats;
/*
@@ -565,6 +567,7 @@ pgss_shmem_startup(void)
pgss->n_writers = 0;
pgss->gc_count = 0;
pgss->stats.dealloc = 0;
+ pgss->stats.stats_reset = GetCurrentTimestamp();
}
memset(&info, 0, sizeof(info));
@@ -1510,6 +1513,7 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_3 23
#define PG_STAT_STATEMENTS_COLS_V1_8 32
#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_INFO_COLS 2
/*
* Retrieve statement statistics.
@@ -1889,7 +1893,16 @@ Datum
pg_stat_statements_info(PG_FUNCTION_ARGS)
{
pgssGlobalStats stats;
+ TupleDesc tupdesc;
+ HeapTuple result_tuple;
+ Datum values[PG_STAT_STATEMENTS_INFO_COLS];
+ bool nulls[PG_STAT_STATEMENTS_INFO_COLS];
+
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
+ MemSet(values, 0, sizeof(values));
+ MemSet(nulls, 0, sizeof(nulls));
/* Read global statistics for pg_stat_statements */
{
volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
@@ -1899,7 +1912,14 @@ pg_stat_statements_info(PG_FUNCTION_ARGS)
SpinLockRelease(&s->mutex);
}
- PG_RETURN_INT64(stats.dealloc);
+ /* Read dealloc */
+ values[0] = Int64GetDatum(stats.dealloc);
+
+ /* Read stats_reset */
+ values[1] = TimestampTzGetDatum(stats.stats_reset);
+
+ result_tuple = heap_form_tuple(tupdesc, values, nulls);
+ return HeapTupleGetDatum(result_tuple);
}
/*
@@ -2507,6 +2527,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
long num_entries;
long num_remove = 0;
pgssHashKey key;
+ TimestampTz reset_ts;
if (!pgss || !pgss_hash)
ereport(ERROR,
@@ -2567,6 +2588,16 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
if (num_entries != num_remove)
goto release_lock;
+ 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);
+ }
+
/*
* Write new empty query file, perhaps even creating a new one to recover
* if the file was missing.
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 81915ea69b..e314b99eea 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -523,6 +523,16 @@
<varname>pg_stat_statements.max</varname> were observed
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>stats_reset</structfield> <type>timestamp with time zone</type>
+ </para>
+ <para>
+ Time at which all statistics in the <structname>pg_stat_statements
+ </structname> view were last reset.
+ </para></entry>
+ </row>
+
</tbody>
</tgroup>
</table>
