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/
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.
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.
Regards.
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..0656308040 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,18 @@ 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");
+
+ tupdesc = BlessTupleDesc(tupdesc);
+ 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 +1914,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 +2529,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,
@@ -2566,7 +2589,17 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
/* All entries are removed? */
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..56e9147b32 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -523,6 +523,15 @@
<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 pg_stat_statements view were last reset.
+ </para></entry>
+ </row>
+
</tbody>
</tgroup>
</table>
