[PATCH] Add features to pg_stat_statements

Started by Katsuragi Yutaover 5 years ago25 messages
#1Katsuragi Yuta
btkatsuragiyu@oss.nttdata.com
1 attachment(s)

This is a proposal to add some features to pg_stat_statements.
Attached is the patch of this.

pg_stat_statements uses a hash table to hold statistics,
and the maximum number of its entries can be configured through
pg_stat_statements.max.
When the number of entries exceeds the pg_stat_statements.max,
pg_stat_statements deallocate existing entries.

Currently, it is impossible to know how many times/when this
deallocation happened.
But, with this information, more detailed performance analysis would be
possible.
So, this patch provides access to this information.

This patch provides a view (pg_stat_statements_ctl) and
a function (pg_stat_statements_ctl).
The pg_stat_statements_ctl view is defined
in terms of a function also named pg_stat_statements_ctl.

The entry of pg_stat_statements_ctl view is removed
when pg_stat_statements_reset(0,0,0) is called.
Maybe, it is convenient to provide a function named
pg_stat_statements_ctl_reset
that removes only the entry of pg_stat_statements_ctl.

The following is an example of this feature.
Here, a deallocation is shown with the INFO message.
pg_stat_statements_ctl tracks the number of deallocations
and timestamp of last deallocation.

testdb=# select * from pg_stat_statements_ctl;
dealloc | last_dealloc
---------+-------------------------------
2 | 2020-09-18 16:55:31.128256+09
(1 row)

testdb=# select count(*) from test1;
2020-09-18 16:59:20.745 JST [3920] INFO: deallocate
2020-09-18 16:59:20.745 JST [3920] STATEMENT: select count(*) from
test1;
INFO: deallocate
count
-------
90
(1 row)

testdb=# select * from pg_stat_statements_ctl;
dealloc | last_dealloc
---------+-------------------------------
3 | 2020-09-18 16:59:20.745652+09
(1 row)

Regards,
Katsuragi Yuta

Attachments:

pg_stat_statements_ctl_v1.patchtext/x-diff; charset=us-ascii; name=pg_stat_statements_ctl_v1.patchDownload
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 081f997d70..63fd4874b1 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -6,7 +6,7 @@ OBJS = \
 	pg_stat_statements.o
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql\
 	pg_stat_statements--1.7--1.8.sql pg_stat_statements--1.6--1.7.sql \
 	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
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
new file mode 100644
index 0000000000..43c7966946
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
@@ -0,0 +1,18 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.9'" to load this file. \quit
+
+--- Define pg_stat_statements_ctl
+CREATE FUNCTION pg_stat_statements_ctl (
+	OUT dealloc integer,
+	OUT last_dealloc TIMESTAMP WITH TIME ZONE
+)
+RETURNS record
+AS 'MODULE_PATHNAME'
+LANGUAGE  C STRICT VOLATILE PARALLEL SAFE;
+
+CREATE VIEW pg_stat_statements_ctl AS
+	SELECT * FROM pg_stat_statements_ctl();
+
+GRANT SELECT ON pg_stat_statements TO PUBLIC;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 1eac9edaee..aa116db498 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;
 
@@ -193,6 +194,17 @@ typedef struct Counters
 	uint64		wal_bytes;		/* total amount of WAL bytes generated */
 } Counters;
 
+/*
+ * Counter for pg_stat_statements_ctl
+ */
+typedef struct pgssCtlCounter
+{
+	int64		dealloc;	/* # of deallocation */
+	TimestampTz	last_dealloc;	/* timestamp of last deallocation */
+	bool		ts_isnull;	/* if true last_dealloc is null */
+	slock_t		mutex;
+} pgssCtlCounter;
+
 /*
  * Statistics per statement
  *
@@ -279,6 +291,7 @@ static ProcessUtility_hook_type prev_ProcessUtility = NULL;
 /* Links to shared memory state */
 static pgssSharedState *pgss = NULL;
 static HTAB *pgss_hash = NULL;
+static pgssCtlCounter *pgss_ctl = NULL;
 
 /*---- GUC variables ----*/
 
@@ -327,6 +340,7 @@ PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_3);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_8);
 PG_FUNCTION_INFO_V1(pg_stat_statements);
+PG_FUNCTION_INFO_V1(pg_stat_statements_ctl);
 
 static void pgss_shmem_startup(void);
 static void pgss_shmem_shutdown(int code, Datum arg);
@@ -380,6 +394,8 @@ static char *generate_normalized_query(pgssJumbleState *jstate, const char *quer
 static void fill_in_constant_lengths(pgssJumbleState *jstate, const char *query,
 									 int query_loc);
 static int	comp_location(const void *a, const void *b);
+static void update_ctl(void);
+static void reset_ctl(void);
 
 
 /*
@@ -518,6 +534,7 @@ static void
 pgss_shmem_startup(void)
 {
 	bool		found;
+	bool		found_ctl;
 	HASHCTL		info;
 	FILE	   *file = NULL;
 	FILE	   *qfile = NULL;
@@ -534,6 +551,7 @@ pgss_shmem_startup(void)
 	/* reset in case this is a restart within the postmaster */
 	pgss = NULL;
 	pgss_hash = NULL;
+	pgss_ctl = NULL;
 
 	/*
 	 * Create or attach to the shared memory state, including hash table
@@ -556,6 +574,17 @@ pgss_shmem_startup(void)
 		pgss->gc_count = 0;
 	}
 
+	pgss_ctl = ShmemInitStruct("pg_stat_statements ctl",
+								sizeof(pgssCtlCounter),
+								&found_ctl);
+	if (!found_ctl)
+	{
+		pgss_ctl->dealloc = 0;
+		pgss_ctl->ts_isnull = true;
+		SpinLockInit(&pgss_ctl->mutex);
+		Assert(pgss_ctl->dealloc == 0);
+	}
+
 	memset(&info, 0, sizeof(info));
 	info.keysize = sizeof(pgssHashKey);
 	info.entrysize = sizeof(pgssEntry);
@@ -577,7 +606,10 @@ pgss_shmem_startup(void)
 	 * Done if some other process already completed our initialization.
 	 */
 	if (found)
+	{
+		Assert(found == found_ctl);
 		return;
+	}
 
 	/*
 	 * Note: we don't bother with locks here, because there should be no other
@@ -672,6 +704,12 @@ pgss_shmem_startup(void)
 		/* copy in the actual stats */
 		entry->counters = temp.counters;
 	}
+	// Assert(feof(file) != 0);	// EOF
+
+	/* Read pgss_ctl */
+	if (feof(file) == 0)
+		if (fread(pgss_ctl, sizeof(pgssCtlCounter), 1, file) != 1)
+			goto read_error;
 
 	pfree(buffer);
 	FreeFile(file);
@@ -748,7 +786,7 @@ pgss_shmem_shutdown(int code, Datum arg)
 		return;
 
 	/* Safety check ... shouldn't get here unless shmem is set up. */
-	if (!pgss || !pgss_hash)
+	if (!pgss || !pgss_hash || !pgss_ctl)
 		return;
 
 	/* Don't dump if told not to. */
@@ -794,6 +832,10 @@ pgss_shmem_shutdown(int code, Datum arg)
 		}
 	}
 
+	/* Dump pgss_ctl */
+	if (fwrite(pgss_ctl, sizeof(pgssCtlCounter), 1, file) != 1)
+		goto error;
+
 	free(qbuffer);
 	qbuffer = NULL;
 
@@ -818,6 +860,7 @@ error:
 			(errcode_for_file_access(),
 			 errmsg("could not write file \"%s\": %m",
 					PGSS_DUMP_FILE ".tmp")));
+
 	if (qbuffer)
 		free(qbuffer);
 	if (file)
@@ -1453,6 +1496,33 @@ done:
 		pfree(norm_query);
 }
 
+static void
+update_ctl(void)
+{
+	TimestampTz	current_ts = GetCurrentTimestamp();
+
+	{
+		volatile pgssCtlCounter *c = (volatile pgssCtlCounter *) pgss_ctl;
+		SpinLockAcquire(&c->mutex);
+		pgss_ctl->dealloc += 1;	/* increment dealloc count */
+		pgss_ctl->last_dealloc = current_ts;
+		pgss_ctl->ts_isnull = false;
+		SpinLockRelease(&c->mutex);
+	}
+}
+
+static void
+reset_ctl(void)
+{
+	{
+		volatile pgssCtlCounter *c = (volatile pgssCtlCounter *) pgss_ctl;
+		SpinLockAcquire(&c->mutex);
+		pgss_ctl->dealloc = 0;	/* reset dealloc count */
+		pgss_ctl->ts_isnull = true;
+		SpinLockRelease(&c->mutex);
+	}
+}
+
 /*
  * Reset statement statistics corresponding to userid, dbid, and queryid.
  */
@@ -1490,6 +1560,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_CTL_COLS		2
 
 /*
  * Retrieve statement statistics.
@@ -1862,6 +1933,44 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 	tuplestore_donestoring(tupstore);
 }
 
+Datum
+pg_stat_statements_ctl(PG_FUNCTION_ARGS)
+{
+	TupleDesc	tupdesc;
+	HeapTuple	result_tuple;
+	Datum		result;
+	Datum 		values[PG_STAT_STATEMENTS_CTL_COLS];
+	bool 		nulls[PG_STAT_STATEMENTS_CTL_COLS];
+	bool		ts_isnull;
+	int 		i = 0;
+
+	Assert(PG_STAT_STATEMENTS_CTL_COLS == 2);
+
+	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+		elog(ERROR, "return type must be a row type");
+
+	tupdesc = BlessTupleDesc(tupdesc);
+
+	memset(nulls, 0, sizeof(nulls));
+
+	{
+		volatile pgssCtlCounter *c = (volatile pgssCtlCounter *) pgss_ctl;
+		SpinLockAcquire(&c->mutex);
+		values[i++] = Int64GetDatum(pgss_ctl->dealloc);
+		ts_isnull = pgss_ctl->ts_isnull;
+		if (!ts_isnull)
+			values[i++] = TimestampTzGetDatum(pgss_ctl->last_dealloc);
+		else
+			nulls[i++] = true;
+		SpinLockRelease(&c->mutex);
+	}
+
+	result_tuple = heap_form_tuple(tupdesc, values, nulls);
+	result = HeapTupleGetDatum(result_tuple);
+
+	return result;
+}
+
 /*
  * Estimate shared memory space needed.
  */
@@ -1872,6 +1981,7 @@ pgss_memsize(void)
 
 	size = MAXALIGN(sizeof(pgssSharedState));
 	size = add_size(size, hash_estimate_size(pgss_max, sizeof(pgssEntry)));
+	size = add_size(size, MAXALIGN(sizeof(pgssCtlCounter)));
 
 	return size;
 }
@@ -1902,7 +2012,10 @@ entry_alloc(pgssHashKey *key, Size query_offset, int query_len, int encoding,
 
 	/* Make space if needed */
 	while (hash_get_num_entries(pgss_hash) >= pgss_max)
+	{
 		entry_dealloc();
+		update_ctl();
+	}
 
 	/* Find or create an entry with desired hash code */
 	entry = (pgssEntry *) hash_search(pgss_hash, key, HASH_ENTER, &found);
@@ -2503,6 +2616,9 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
 			hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL);
 			num_remove++;
 		}
+
+		/* Reset pgss_ctl */
+		reset_ctl();
 	}
 
 	/* All entries are removed? */
diff --git a/contrib/pg_stat_statements/pg_stat_statements.control b/contrib/pg_stat_statements/pg_stat_statements.control
index 65b18b11d2..2f1ce6ed50 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.control
+++ b/contrib/pg_stat_statements/pg_stat_statements.control
@@ -1,5 +1,5 @@
 # pg_stat_statements extension
 comment = 'track planning and execution statistics of all SQL statements executed'
-default_version = '1.8'
+default_version = '1.9'
 module_pathname = '$libdir/pg_stat_statements'
 relocatable = true
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index cf2d25b7b2..605795fe80 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -23,10 +23,11 @@
  <para>
    When <filename>pg_stat_statements</filename> is loaded, it tracks
    statistics across all databases of the server.  To access and manipulate
-   these statistics, the module provides a view, <structname>pg_stat_statements</structname>,
-   and the utility functions <function>pg_stat_statements_reset</function> and
-   <function>pg_stat_statements</function>.  These are not available globally but
-   can be enabled for a specific database with
+   these statistics, the module provides views, <structname>pg_stat_statements</structname>
+   and <structname>pg_stat_statements_ctl</structname>,
+   and the utility functions <function>pg_stat_statements_reset</function>,
+   <function>pg_stat_statements</function>, and <function>pg_stat_statements_ctl</function>.
+   These are not available globally but can be enabled for a specific database with
    <command>CREATE EXTENSION pg_stat_statements</command>.
  </para>
 
@@ -480,6 +481,57 @@
   </para>
  </sect2>
 
+ <sect2>
+  <title>The <structname>pg_stat_statements_ctl</structname> View</title>
+
+  <para>
+   This module tracks statistics of <filename>pg_stat_statements</filename>
+   itself for detailed performance analysis.
+   The statistics of pg_stat_statements itself are made available via a
+   view named <structname>pg_stat_statements_ctl</structname>.
+   This view contains only a single row.
+   The row is reset only when <function>pg_stat_statements_reset(0,0,0)</function>
+   is called. The columns of the view are
+   shown in <xref linkend="pgstatstatementsctl-columns"/>.
+  </para>
+
+  <table id="pgstatstatementsctl-columns">
+   <title><structname>pg_stat_statements_ctl</structname> Columns</title>
+   <tgroup cols="1">
+    <thead>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       Column Type
+      </para>
+      <para>
+       Description
+      </para></entry>
+     </row>
+    </thead>
+
+    <tbody>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>dealloc</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Total number of deallocations of pg_stat_statements view entry
+      </para></entry>
+     </row>
+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>last_dealloc</structfield> <type>timestamp with time zone</type>
+      </para>
+      <para>
+       The timestamp of last deallocation
+      </para></entry>
+     </row>
+    </tbody>
+   </tgroup>
+  </table>
+ </sect2>
+
  <sect2>
   <title>Functions</title>
 
@@ -537,6 +589,24 @@
      </para>
     </listitem>
    </varlistentry>
+
+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_ctl() returns record</function>
+     <indexterm>
+      <primary>pg_stat_statements_ctl</primary>
+     </indexterm>
+    </term>
+
+    <listitem>
+     <para>
+      The <structname>pg_stat_statements_ctl</structname> view is defined
+      in terms of a function also named <function>pg_stat_statements_ctl</function>.
+      It is possible for clients to call the 
+      <function>pg_stat_statements_ctl</function> directly.
+     </para>
+    </listitem>
+   </varlistentry>
   </variablelist>
  </sect2>
 
#2Julien Rouhaud
rjuju123@gmail.com
In reply to: Katsuragi Yuta (#1)
Re: [PATCH] Add features to pg_stat_statements

Hi,

On Fri, Sep 18, 2020 at 10:54 AM Katsuragi Yuta
<btkatsuragiyu@oss.nttdata.com> wrote:

This is a proposal to add some features to pg_stat_statements.
Attached is the patch of this.

pg_stat_statements uses a hash table to hold statistics,
and the maximum number of its entries can be configured through
pg_stat_statements.max.
When the number of entries exceeds the pg_stat_statements.max,
pg_stat_statements deallocate existing entries.

Currently, it is impossible to know how many times/when this
deallocation happened.
But, with this information, more detailed performance analysis would be
possible.
So, this patch provides access to this information.

This patch provides a view (pg_stat_statements_ctl) and
a function (pg_stat_statements_ctl).
The pg_stat_statements_ctl view is defined
in terms of a function also named pg_stat_statements_ctl.

The entry of pg_stat_statements_ctl view is removed
when pg_stat_statements_reset(0,0,0) is called.
Maybe, it is convenient to provide a function named
pg_stat_statements_ctl_reset
that removes only the entry of pg_stat_statements_ctl.

The following is an example of this feature.
Here, a deallocation is shown with the INFO message.
pg_stat_statements_ctl tracks the number of deallocations
and timestamp of last deallocation.

testdb=# select * from pg_stat_statements_ctl;
dealloc | last_dealloc
---------+-------------------------------
2 | 2020-09-18 16:55:31.128256+09
(1 row)

testdb=# select count(*) from test1;
2020-09-18 16:59:20.745 JST [3920] INFO: deallocate
2020-09-18 16:59:20.745 JST [3920] STATEMENT: select count(*) from
test1;
INFO: deallocate
count
-------
90
(1 row)

testdb=# select * from pg_stat_statements_ctl;
dealloc | last_dealloc
---------+-------------------------------
3 | 2020-09-18 16:59:20.745652+09
(1 row)

I like it, this is especially important since this can lead to quite
huge overhead. Did you consider also adding the cumulated number of
evicted entries? This could be useful to know how to configure
pg_stat_statements.max.

#3legrand legrand
legrand_legrand@hotmail.com
In reply to: Katsuragi Yuta (#1)
Re: [PATCH] Add features to pg_stat_statements

+1 !

An other way is to log evictions, it provides informations about time and
amount :

for (i = 0; i < nvictims; i++)
{
hash_search(pgssp_hash, &entries[i]->key, HASH_REMOVE, NULL);
}

pfree(entries);

/* trace when evicting entries, if appening too often this can slow queries
...
* increasing pg_stat_sql_plans.max value could help */
ereport(LOG,
(errmsg("pg_stat_sql_plans evicting %d entries", nvictims),
errhidecontext(true), errhidestmt(true)));

--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

#4Katsuragi Yuta
btkatsuragiyu@oss.nttdata.com
In reply to: Julien Rouhaud (#2)
Re: [PATCH] Add features to pg_stat_statements

On 2020-09-18 18:49, Julien Rouhaud wrote:

Did you consider also adding the cumulated number of
evicted entries? This could be useful to know how to configure
pg_stat_statements.max.

Thank you for your comments!
I overlooked the cumulated number of evicted entries.
This statistic looks important. But, I am not sure
if I should add this statistic to a view.
This is because I am not sure how to utilize the cumulated
number of evicted entries for configuring pg_stat_statements.max.

Not only providing a view but also logging evictions
along with the number of evicted entries might be a choice.
This idea is from legrand legrand [1]/messages/by-id/1600500942767-0.post@n3.nabble.com.

[1]: /messages/by-id/1600500942767-0.post@n3.nabble.com
/messages/by-id/1600500942767-0.post@n3.nabble.com

Regards,
Katsuragi Yuta

#5Julien Rouhaud
rjuju123@gmail.com
In reply to: Katsuragi Yuta (#4)
Re: [PATCH] Add features to pg_stat_statements

On Wed, Sep 23, 2020 at 2:48 PM Katsuragi Yuta
<btkatsuragiyu@oss.nttdata.com> wrote:

On 2020-09-18 18:49, Julien Rouhaud wrote:

Did you consider also adding the cumulated number of
evicted entries? This could be useful to know how to configure
pg_stat_statements.max.

Thank you for your comments!
I overlooked the cumulated number of evicted entries.
This statistic looks important. But, I am not sure
if I should add this statistic to a view.
This is because I am not sure how to utilize the cumulated
number of evicted entries for configuring pg_stat_statements.max.

You're right, as the number of evicted entries isn't depending on the
number of entries that wouls been needed to contain the entirely
workload.

Not only providing a view but also logging evictions
along with the number of evicted entries might be a choice.
This idea is from legrand legrand [1].

+1. I'm wondering if logging each evicted entry, with its queryid,
would help to estimate the actual size of the normalised queries set.

#6Katsuragi Yuta
btkatsuragiyu@oss.nttdata.com
In reply to: legrand legrand (#3)
Re: [PATCH] Add features to pg_stat_statements

On 2020-09-19 16:35, legrand legrand wrote:

+1 !

An other way is to log evictions, it provides informations about time
and
amount :

for (i = 0; i < nvictims; i++)
{
hash_search(pgssp_hash, &entries[i]->key, HASH_REMOVE, NULL);
}

pfree(entries);

/* trace when evicting entries, if appening too often this can slow
queries
...
* increasing pg_stat_sql_plans.max value could help */
ereport(LOG,
(errmsg("pg_stat_sql_plans evicting %d entries", nvictims),
errhidecontext(true), errhidestmt(true)));

--
Sent from:
https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Thank you for your comments!
In addition to providing a view that is like a summary of history,
logging might be a good choice.

Regards,
Katsuragi Yuta

#7Katsuragi Yuta
btkatsuragiyu@oss.nttdata.com
In reply to: Julien Rouhaud (#5)
Re: [PATCH] Add features to pg_stat_statements

On 2020-09-23 16:01, Julien Rouhaud wrote:

Not only providing a view but also logging evictions
along with the number of evicted entries might be a choice.
This idea is from legrand legrand [1].

+1. I'm wondering if logging each evicted entry, with its queryid,
would help to estimate the actual size of the normalised queries set.

I agree with the estimation of the actual size of the
query set is important.
It looks difficult to estimate the actual size of the
query set from queryid because queryid is a 64bit hash value.

Regards,
Katsuragi Yuta

#8legrand legrand
legrand_legrand@hotmail.com
In reply to: Katsuragi Yuta (#6)
Re: [PATCH] Add features to pg_stat_statements

Hi,
Both are probably not needed.
I would prefer it accessible in a view live

Event | date | victims
Eviction...
Reset...
Part reset ...

As there are other needs to track reset times.
Regards
PAscal

--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

#9Katsuragi Yuta
btkatsuragiyu@oss.nttdata.com
In reply to: legrand legrand (#8)
Re: [PATCH] Add features to pg_stat_statements

On 2020-09-24 14:15, legrand legrand wrote:

Hi,
Both are probably not needed.
I would prefer it accessible in a view live

Event | date | victims
Eviction...
Reset...
Part reset ...

As there are other needs to track reset times.

Let me confirm one thing.
Is the number of records of this view fixed to three?
Or, will a new record be appended every time
an event (Eviction or Reset or Part Reset) happens?

Regards,
Katsuragi Yuta

#10legrand legrand
legrand_legrand@hotmail.com
In reply to: Katsuragi Yuta (#9)
Re: [PATCH] Add features to pg_stat_statements

Not limited to 3, Like an history table.

Will have to think if data is saved at shutdown.

--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

#11Katsuragi Yuta
btkatsuragiyu@oss.nttdata.com
In reply to: legrand legrand (#10)
Re: [PATCH] Add features to pg_stat_statements

On 2020-09-24 18:55, legrand legrand wrote:

Not limited to 3, Like an history table.

Will have to think if data is saved at shutdown.

Thanks.

This design might introduce some additional complexity
and things to be considered.
For example, the maximum size of "history table",
how to evict entries from the history table and how to manage
the information maintained by evicted entries.

Also, providing a history table looks similar to logging.

Providing the original view (# of dealloc and last_dealloc ts)
and optional logging looks a simple and better way.

Regards,
Katsuragi Yuta

#12Yuki Seino
seinoyu@oss.nttdata.com
In reply to: Katsuragi Yuta (#11)
Re: [PATCH] Add features to pg_stat_statements

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

The patch applies cleanly and looks fine to me. It's a small detail, However wouldn't it be better if the following changes were made.

1.There are unnecessary difference lines 707 and 863 in "pg_stat_statements.c".
2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in "pg_stat_statements.c".
3."update_ctl" and "reset_ctl" are generic and illegible name.You might want to rename it something like "pgss_ctl_update".
4.To improve the readability of the source, why not change the function declaration option in "pg_stat_statements_ctl" from
"AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME', 'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".

The new status of this patch is: Waiting on Author

#13Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Yuki Seino (#12)
Re: [PATCH] Add features to pg_stat_statements

On 2020/10/12 21:18, Yuki Seino wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

The patch applies cleanly and looks fine to me. It's a small detail, However wouldn't it be better if the following changes were made.

1.There are unnecessary difference lines 707 and 863 in "pg_stat_statements.c".
2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in "pg_stat_statements.c".
3."update_ctl" and "reset_ctl" are generic and illegible name.You might want to rename it something like "pgss_ctl_update".
4.To improve the readability of the source, why not change the function declaration option in "pg_stat_statements_ctl" from
"AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME', 'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".

The new status of this patch is: Waiting on Author

Here are other comments from me.

-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql\

One space character needs to be added just before the character "\".

+--- Define pg_stat_statements_ctl

ISTM that this is not good name.
What about pg_stat_statements_info, _stats, _activity or something?

+ OUT dealloc integer,

The type of "dealloc" should be bigint, instead?

+ OUT last_dealloc TIMESTAMP WITH TIME ZONE

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

+LANGUAGE C STRICT VOLATILE PARALLEL SAFE;

There are two space characters just after "LANGUAGE".
One space character should be removed from there.

+CREATE VIEW pg_stat_statements_ctl AS
+	SELECT * FROM pg_stat_statements_ctl();

If we rename the function, this view name also should be changed.

+GRANT SELECT ON pg_stat_statements TO PUBLIC;

"pg_stat_statements" should be "pg_stat_statement_xxx"?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#14seinoyu
seinoyu@oss.nttdata.com
In reply to: Fujii Masao (#13)
Re: [PATCH] Add features to pg_stat_statements

2020-10-22 01:31 に Fujii Masao さんは書きました:

On 2020/10/12 21:18, Yuki Seino wrote:

The following review has been posted through the commitfest
application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

The patch applies cleanly and looks fine to me. It's a small detail,
However wouldn't it be better if the following changes were made.

1.There are unnecessary difference lines 707 and 863 in
"pg_stat_statements.c".
2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in
"pg_stat_statements.c".
3."update_ctl" and "reset_ctl" are generic and illegible name.You
might want to rename it something like "pgss_ctl_update".
4.To improve the readability of the source, why not change the
function declaration option in "pg_stat_statements_ctl" from
"AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME',
'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".

The new status of this patch is: Waiting on Author

Here are other comments from me.

-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql\

One space character needs to be added just before the character "\".

+--- Define pg_stat_statements_ctl

ISTM that this is not good name.
What about pg_stat_statements_info, _stats, _activity or something?

+ OUT dealloc integer,

The type of "dealloc" should be bigint, instead?

+ OUT last_dealloc TIMESTAMP WITH TIME ZONE

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

+LANGUAGE C STRICT VOLATILE PARALLEL SAFE;

There are two space characters just after "LANGUAGE".
One space character should be removed from there.

+CREATE VIEW pg_stat_statements_ctl AS
+	SELECT * FROM pg_stat_statements_ctl();

If we rename the function, this view name also should be changed.

+GRANT SELECT ON pg_stat_statements TO PUBLIC;

"pg_stat_statements" should be "pg_stat_statement_xxx"?

Regards,

Thanks for the comment, Fujii-san.

I will post the patch again in the future to reflect your and my points.

However, let me confirm the following.

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

I thought it would be easy for users to see at a glance that if there is
a case I assumed,
if the last modified date and time is old, there is no need to adjust at
all, and if the
last modified date and time is recent, it would be easy for users to
understand that the
parameters need to be adjusted.
What do you think?

Regards,
Seino Yuki

#15Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: seinoyu (#14)
Re: [PATCH] Add features to pg_stat_statements

On 2020/10/28 17:31, seinoyu wrote:

2020-10-22 01:31 に Fujii Masao さんは書きました:

On 2020/10/12 21:18, Yuki Seino wrote:

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

The patch applies cleanly and looks fine to me. It's a small detail, However wouldn't it be better if the following changes were made.

1.There are unnecessary difference lines 707 and 863 in "pg_stat_statements.c".
2.There is no comment on "slock_t mutex" in "struct pgssCtlCounter" in "pg_stat_statements.c".
3."update_ctl" and "reset_ctl" are generic and illegible name.You might want to rename it something like "pgss_ctl_update".
4.To improve the readability of the source, why not change the function declaration option in "pg_stat_statements_ctl" from
"AS 'MODULE_PATHNAME'" to "AS 'MODULE_PATHNAME', 'pg_stat_statements_ctl'"? in "pg_stat_statements--1.8--1.9.sql".

The new status of this patch is: Waiting on Author

Here are other comments from me.

-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql\

One space character needs to be added just before the character "\".

+--- Define pg_stat_statements_ctl

ISTM that this is not good name.
What about pg_stat_statements_info, _stats, _activity or something?

+    OUT dealloc integer,

The type of "dealloc" should be bigint, instead?

+    OUT last_dealloc TIMESTAMP WITH TIME ZONE

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

+LANGUAGE  C STRICT VOLATILE PARALLEL SAFE;

There are two space characters just after "LANGUAGE".
One space character should be removed from there.

+CREATE VIEW pg_stat_statements_ctl AS
+    SELECT * FROM pg_stat_statements_ctl();

If we rename the function, this view name also should be changed.

+GRANT SELECT ON pg_stat_statements TO PUBLIC;

"pg_stat_statements" should be "pg_stat_statement_xxx"?

Regards,

Thanks for the comment, Fujii-san.

I will post the patch again in the future to reflect your and my points.

Thanks!

However, let me confirm the following.

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

I thought it would be easy for users to see at a glance that if there is a case I assumed,
if the last modified date and time is old, there is no need to adjust at all, and if the
last modified date and time is recent, it would be easy for users to understand that the
parameters need to be adjusted.
What do you think?

Even without the last deallocation timestamp, you can presume
when the deallocation of entries happened by periodically monitoring
the total number of deallocations and seeing those history. Or IMO
it's better to log whenever the deallocation happens as proposed upthread.
Then you can easily check the history of occurrences of deallocations
from the log file.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#16Seino Yuki
seinoyu@oss.nttdata.com
In reply to: Fujii Masao (#15)
1 attachment(s)
Re: [PATCH] Add features to pg_stat_statements

However, let me confirm the following.

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

I thought it would be easy for users to see at a glance that if there
is a case I assumed,
if the last modified date and time is old, there is no need to adjust
at all, and if the
last modified date and time is recent, it would be easy for users to
understand that the
parameters need to be adjusted.
What do you think?

Even without the last deallocation timestamp, you can presume
when the deallocation of entries happened by periodically monitoring
the total number of deallocations and seeing those history. Or IMO
it's better to log whenever the deallocation happens as proposed
upthread.
Then you can easily check the history of occurrences of deallocations
from the log file.

Regards,

+1.I think you should output the deallocation history to the log as
well.

In light of the above, I've posted a patch that reflects the points
made.

Regards,

Attachments:

pg_stat_statements_info.patchtext/x-diff; charset=us-ascii; name=pg_stat_statements_info.patchDownload
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 081f997d70..3ec627b956 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -6,7 +6,7 @@ OBJS = \
 	pg_stat_statements.o
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql \
 	pg_stat_statements--1.7--1.8.sql pg_stat_statements--1.6--1.7.sql \
 	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0edb134f3..85e78c7f46 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -859,4 +859,19 @@ SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE
  SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" |     1 |     0 |    0
 (6 rows)
 
+--
+-- Checking the execution of the pg_stat_statements_info view
+--
+SELECT pg_stat_statements_reset(0,0,0);
+ pg_stat_statements_reset 
+--------------------------
+ 
+(1 row)
+
+SELECT * FROM pg_stat_statements_info;
+ dealloc 
+---------
+       0
+(1 row)
+
 DROP EXTENSION pg_stat_statements;
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
new file mode 100644
index 0000000000..5c491f242a
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
@@ -0,0 +1,17 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.9'" to load this file. \quit
+
+--- Define pg_stat_statements_info
+CREATE FUNCTION pg_stat_statements_info (
+	OUT dealloc bigint
+)
+RETURNS bigint
+AS 'MODULE_PATHNAME'
+LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
+
+CREATE VIEW pg_stat_statements_info AS
+	SELECT * FROM pg_stat_statements_info();
+
+GRANT SELECT ON pg_stat_statements_info TO PUBLIC;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 1eac9edaee..65cf4fb0a7 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -193,6 +193,15 @@ typedef struct Counters
 	uint64		wal_bytes;		/* total amount of WAL bytes generated */
 } Counters;
 
+/*
+ * Counter for pg_stat_statements_info
+ */
+typedef struct pgssInfoCounters
+{
+	int64		dealloc;		/* # of deallocation */
+	slock_t		mutex;			/* protects the counters only */
+} pgssInfoCounters;
+
 /*
  * Statistics per statement
  *
@@ -279,6 +288,7 @@ static ProcessUtility_hook_type prev_ProcessUtility = NULL;
 /* Links to shared memory state */
 static pgssSharedState *pgss = NULL;
 static HTAB *pgss_hash = NULL;
+static pgssInfoCounters *pgss_info = NULL;
 
 /*---- GUC variables ----*/
 
@@ -327,6 +337,7 @@ PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_3);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_8);
 PG_FUNCTION_INFO_V1(pg_stat_statements);
+PG_FUNCTION_INFO_V1(pg_stat_statements_info);
 
 static void pgss_shmem_startup(void);
 static void pgss_shmem_shutdown(int code, Datum arg);
@@ -380,6 +391,8 @@ static char *generate_normalized_query(pgssJumbleState *jstate, const char *quer
 static void fill_in_constant_lengths(pgssJumbleState *jstate, const char *query,
 									 int query_loc);
 static int	comp_location(const void *a, const void *b);
+static void pgss_info_update(void);
+static void pgss_info_reset(void);
 
 
 /*
@@ -518,6 +531,7 @@ static void
 pgss_shmem_startup(void)
 {
 	bool		found;
+	bool		found_info;
 	HASHCTL		info;
 	FILE	   *file = NULL;
 	FILE	   *qfile = NULL;
@@ -534,6 +548,7 @@ pgss_shmem_startup(void)
 	/* reset in case this is a restart within the postmaster */
 	pgss = NULL;
 	pgss_hash = NULL;
+	pgss_info = NULL;
 
 	/*
 	 * Create or attach to the shared memory state, including hash table
@@ -556,6 +571,16 @@ pgss_shmem_startup(void)
 		pgss->gc_count = 0;
 	}
 
+	pgss_info = ShmemInitStruct("pg_stat_statements_info",
+								sizeof(pgssInfoCounters),
+								&found_info);
+	if (!found_info)
+	{
+		pgss_info->dealloc = 0;
+		SpinLockInit(&pgss_info->mutex);
+		Assert(pgss_info->dealloc == 0);
+	}
+
 	memset(&info, 0, sizeof(info));
 	info.keysize = sizeof(pgssHashKey);
 	info.entrysize = sizeof(pgssEntry);
@@ -577,7 +602,10 @@ pgss_shmem_startup(void)
 	 * Done if some other process already completed our initialization.
 	 */
 	if (found)
+	{
+		Assert(found == found_info);
 		return;
+	}
 
 	/*
 	 * Note: we don't bother with locks here, because there should be no other
@@ -673,6 +701,11 @@ pgss_shmem_startup(void)
 		entry->counters = temp.counters;
 	}
 
+	/* Read pgss_info */
+	if (feof(file) == 0)
+		if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+			goto read_error;
+
 	pfree(buffer);
 	FreeFile(file);
 	FreeFile(qfile);
@@ -748,7 +781,7 @@ pgss_shmem_shutdown(int code, Datum arg)
 		return;
 
 	/* Safety check ... shouldn't get here unless shmem is set up. */
-	if (!pgss || !pgss_hash)
+	if (!pgss || !pgss_hash || !pgss_info)
 		return;
 
 	/* Don't dump if told not to. */
@@ -794,6 +827,10 @@ pgss_shmem_shutdown(int code, Datum arg)
 		}
 	}
 
+	/* Dump pgss_info */
+	if (fwrite(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+		goto error;
+
 	free(qbuffer);
 	qbuffer = NULL;
 
@@ -1453,6 +1490,28 @@ done:
 		pfree(norm_query);
 }
 
+static void
+pgss_info_update(void)
+{
+	{
+		volatile pgssInfoCounters *c = (volatile pgssInfoCounters *) pgss_info;
+		SpinLockAcquire(&c->mutex);
+		c->dealloc += 1;	/* increment dealloc count */
+		SpinLockRelease(&c->mutex);
+	}
+}
+
+static void
+pgss_info_reset(void)
+{
+	{
+		volatile pgssInfoCounters *c = (volatile pgssInfoCounters *) pgss_info;
+		SpinLockAcquire(&c->mutex);
+		c->dealloc = 0;	/* reset dealloc count */
+		SpinLockRelease(&c->mutex);
+	}
+}
+
 /*
  * Reset statement statistics corresponding to userid, dbid, and queryid.
  */
@@ -1490,6 +1549,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	1
 
 /*
  * Retrieve statement statistics.
@@ -1862,6 +1922,19 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 	tuplestore_donestoring(tupstore);
 }
 
+Datum
+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+	int64		d_count = 0;
+	{
+		volatile pgssInfoCounters *c = (volatile pgssInfoCounters *) pgss_info;
+		SpinLockAcquire(&c->mutex);
+		d_count = Int64GetDatum(c->dealloc);
+		SpinLockRelease(&c->mutex);
+	}
+	PG_RETURN_INT64(d_count);
+}
+
 /*
  * Estimate shared memory space needed.
  */
@@ -1872,6 +1945,7 @@ pgss_memsize(void)
 
 	size = MAXALIGN(sizeof(pgssSharedState));
 	size = add_size(size, hash_estimate_size(pgss_max, sizeof(pgssEntry)));
+	size = add_size(size, MAXALIGN(sizeof(pgssInfoCounters)));
 
 	return size;
 }
@@ -1902,7 +1976,12 @@ entry_alloc(pgssHashKey *key, Size query_offset, int query_len, int encoding,
 
 	/* Make space if needed */
 	while (hash_get_num_entries(pgss_hash) >= pgss_max)
+	{
 		entry_dealloc();
+		pgss_info_update();
+		ereport(LOG,
+			(errmsg("The information in pg_stat_statements has been deallocated.")));
+	}
 
 	/* Find or create an entry with desired hash code */
 	entry = (pgssEntry *) hash_search(pgss_hash, key, HASH_ENTER, &found);
@@ -2503,6 +2582,9 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
 			hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL);
 			num_remove++;
 		}
+
+		/* Reset pgss_info */
+		pgss_info_reset();
 	}
 
 	/* All entries are removed? */
diff --git a/contrib/pg_stat_statements/pg_stat_statements.control b/contrib/pg_stat_statements/pg_stat_statements.control
index 65b18b11d2..2f1ce6ed50 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.control
+++ b/contrib/pg_stat_statements/pg_stat_statements.control
@@ -1,5 +1,5 @@
 # pg_stat_statements extension
 comment = 'track planning and execution statistics of all SQL statements executed'
-default_version = '1.8'
+default_version = '1.9'
 module_pathname = '$libdir/pg_stat_statements'
 relocatable = true
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index 996a24a293..e3cb0930d9 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -357,4 +357,10 @@ SELECT 42;
 SELECT 42;
 SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
 
+--
+-- Checking the execution of the pg_stat_statements_info view
+--
+SELECT pg_stat_statements_reset(0,0,0);
+SELECT * FROM pg_stat_statements_info;
+
 DROP EXTENSION pg_stat_statements;
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index cf2d25b7b2..b3c53dd39f 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -23,10 +23,11 @@
  <para>
    When <filename>pg_stat_statements</filename> is loaded, it tracks
    statistics across all databases of the server.  To access and manipulate
-   these statistics, the module provides a view, <structname>pg_stat_statements</structname>,
-   and the utility functions <function>pg_stat_statements_reset</function> and
-   <function>pg_stat_statements</function>.  These are not available globally but
-   can be enabled for a specific database with
+   these statistics, the module provides views, <structname>pg_stat_statements</structname>
+   and <structname>pg_stat_statements_info</structname>,
+   and the utility functions <function>pg_stat_statements_reset</function>,
+   <function>pg_stat_statements</function>, and <function>pg_stat_statements_info</function>.
+   These are not available globally but can be enabled for a specific database with
    <command>CREATE EXTENSION pg_stat_statements</command>.
  </para>
 
@@ -480,6 +481,48 @@
   </para>
  </sect2>
 
+ <sect2>
+  <title>The <structname>pg_stat_statements_info</structname> View</title>
+
+  <para>
+   This module tracks statistics of <filename>pg_stat_statements</filename>
+   itself for detailed performance analysis.
+   The statistics of pg_stat_statements itself are made available via a
+   view named <structname>pg_stat_statements_info</structname>.
+   This view contains only a single row.
+   The row is reset only when <function>pg_stat_statements_reset(0,0,0)</function>
+   is called. The columns of the view are
+   shown in <xref linkend="pgstatstatementsctl-columns"/>.
+  </para>
+
+  <table id="pgstatstatementsctl-columns">
+   <title><structname>pg_stat_statements_info</structname> Columns</title>
+   <tgroup cols="1">
+    <thead>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       Column Type
+      </para>
+      <para>
+       Description
+      </para></entry>
+     </row>
+    </thead>
+
+    <tbody>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>dealloc</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Total number of deallocations of pg_stat_statements view entry
+      </para></entry>
+     </row>
+    </tbody>
+   </tgroup>
+  </table>
+ </sect2>
+
  <sect2>
   <title>Functions</title>
 
@@ -537,6 +580,24 @@
      </para>
     </listitem>
    </varlistentry>
+
+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns record</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>
+
+    <listitem>
+     <para>
+      The <structname>pg_stat_statements_info</structname> view is defined
+      in terms of a function also named <function>pg_stat_statements_info</function>.
+      It is possible for clients to call the 
+      <function>pg_stat_statements_info</function> directly.
+     </para>
+    </listitem>
+   </varlistentry>
   </variablelist>
  </sect2>
 
#17Seino Yuki
seinoyu@oss.nttdata.com
In reply to: Seino Yuki (#16)
1 attachment(s)
Re: [PATCH] Add features to pg_stat_statements

2020-11-09 15:39 に Seino Yuki さんは書きました:

However, let me confirm the following.

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

I thought it would be easy for users to see at a glance that if there
is a case I assumed,
if the last modified date and time is old, there is no need to adjust
at all, and if the
last modified date and time is recent, it would be easy for users to
understand that the
parameters need to be adjusted.
What do you think?

Even without the last deallocation timestamp, you can presume
when the deallocation of entries happened by periodically monitoring
the total number of deallocations and seeing those history. Or IMO
it's better to log whenever the deallocation happens as proposed
upthread.
Then you can easily check the history of occurrences of deallocations
from the log file.

Regards,

+1.I think you should output the deallocation history to the log as
well.

In light of the above, I've posted a patch that reflects the points
made.

Regards,

Sorry. There was a typo in the documentation correction.
I'll post a patch to fix it.

Regards,

Attachments:

pg_stat_statements_info.patchtext/x-diff; name=pg_stat_statements_info.patchDownload
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 081f997d70..3ec627b956 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -6,7 +6,7 @@ OBJS = \
 	pg_stat_statements.o
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql \
 	pg_stat_statements--1.7--1.8.sql pg_stat_statements--1.6--1.7.sql \
 	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0edb134f3..85e78c7f46 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -859,4 +859,19 @@ SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE
  SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" |     1 |     0 |    0
 (6 rows)
 
+--
+-- Checking the execution of the pg_stat_statements_info view
+--
+SELECT pg_stat_statements_reset(0,0,0);
+ pg_stat_statements_reset 
+--------------------------
+ 
+(1 row)
+
+SELECT * FROM pg_stat_statements_info;
+ dealloc 
+---------
+       0
+(1 row)
+
 DROP EXTENSION pg_stat_statements;
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
new file mode 100644
index 0000000000..5c491f242a
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
@@ -0,0 +1,17 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.9'" to load this file. \quit
+
+--- Define pg_stat_statements_info
+CREATE FUNCTION pg_stat_statements_info (
+	OUT dealloc bigint
+)
+RETURNS bigint
+AS 'MODULE_PATHNAME'
+LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
+
+CREATE VIEW pg_stat_statements_info AS
+	SELECT * FROM pg_stat_statements_info();
+
+GRANT SELECT ON pg_stat_statements_info TO PUBLIC;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 1eac9edaee..65cf4fb0a7 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -193,6 +193,15 @@ typedef struct Counters
 	uint64		wal_bytes;		/* total amount of WAL bytes generated */
 } Counters;
 
+/*
+ * Counter for pg_stat_statements_info
+ */
+typedef struct pgssInfoCounters
+{
+	int64		dealloc;		/* # of deallocation */
+	slock_t		mutex;			/* protects the counters only */
+} pgssInfoCounters;
+
 /*
  * Statistics per statement
  *
@@ -279,6 +288,7 @@ static ProcessUtility_hook_type prev_ProcessUtility = NULL;
 /* Links to shared memory state */
 static pgssSharedState *pgss = NULL;
 static HTAB *pgss_hash = NULL;
+static pgssInfoCounters *pgss_info = NULL;
 
 /*---- GUC variables ----*/
 
@@ -327,6 +337,7 @@ PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_3);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_8);
 PG_FUNCTION_INFO_V1(pg_stat_statements);
+PG_FUNCTION_INFO_V1(pg_stat_statements_info);
 
 static void pgss_shmem_startup(void);
 static void pgss_shmem_shutdown(int code, Datum arg);
@@ -380,6 +391,8 @@ static char *generate_normalized_query(pgssJumbleState *jstate, const char *quer
 static void fill_in_constant_lengths(pgssJumbleState *jstate, const char *query,
 									 int query_loc);
 static int	comp_location(const void *a, const void *b);
+static void pgss_info_update(void);
+static void pgss_info_reset(void);
 
 
 /*
@@ -518,6 +531,7 @@ static void
 pgss_shmem_startup(void)
 {
 	bool		found;
+	bool		found_info;
 	HASHCTL		info;
 	FILE	   *file = NULL;
 	FILE	   *qfile = NULL;
@@ -534,6 +548,7 @@ pgss_shmem_startup(void)
 	/* reset in case this is a restart within the postmaster */
 	pgss = NULL;
 	pgss_hash = NULL;
+	pgss_info = NULL;
 
 	/*
 	 * Create or attach to the shared memory state, including hash table
@@ -556,6 +571,16 @@ pgss_shmem_startup(void)
 		pgss->gc_count = 0;
 	}
 
+	pgss_info = ShmemInitStruct("pg_stat_statements_info",
+								sizeof(pgssInfoCounters),
+								&found_info);
+	if (!found_info)
+	{
+		pgss_info->dealloc = 0;
+		SpinLockInit(&pgss_info->mutex);
+		Assert(pgss_info->dealloc == 0);
+	}
+
 	memset(&info, 0, sizeof(info));
 	info.keysize = sizeof(pgssHashKey);
 	info.entrysize = sizeof(pgssEntry);
@@ -577,7 +602,10 @@ pgss_shmem_startup(void)
 	 * Done if some other process already completed our initialization.
 	 */
 	if (found)
+	{
+		Assert(found == found_info);
 		return;
+	}
 
 	/*
 	 * Note: we don't bother with locks here, because there should be no other
@@ -673,6 +701,11 @@ pgss_shmem_startup(void)
 		entry->counters = temp.counters;
 	}
 
+	/* Read pgss_info */
+	if (feof(file) == 0)
+		if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+			goto read_error;
+
 	pfree(buffer);
 	FreeFile(file);
 	FreeFile(qfile);
@@ -748,7 +781,7 @@ pgss_shmem_shutdown(int code, Datum arg)
 		return;
 
 	/* Safety check ... shouldn't get here unless shmem is set up. */
-	if (!pgss || !pgss_hash)
+	if (!pgss || !pgss_hash || !pgss_info)
 		return;
 
 	/* Don't dump if told not to. */
@@ -794,6 +827,10 @@ pgss_shmem_shutdown(int code, Datum arg)
 		}
 	}
 
+	/* Dump pgss_info */
+	if (fwrite(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+		goto error;
+
 	free(qbuffer);
 	qbuffer = NULL;
 
@@ -1453,6 +1490,28 @@ done:
 		pfree(norm_query);
 }
 
+static void
+pgss_info_update(void)
+{
+	{
+		volatile pgssInfoCounters *c = (volatile pgssInfoCounters *) pgss_info;
+		SpinLockAcquire(&c->mutex);
+		c->dealloc += 1;	/* increment dealloc count */
+		SpinLockRelease(&c->mutex);
+	}
+}
+
+static void
+pgss_info_reset(void)
+{
+	{
+		volatile pgssInfoCounters *c = (volatile pgssInfoCounters *) pgss_info;
+		SpinLockAcquire(&c->mutex);
+		c->dealloc = 0;	/* reset dealloc count */
+		SpinLockRelease(&c->mutex);
+	}
+}
+
 /*
  * Reset statement statistics corresponding to userid, dbid, and queryid.
  */
@@ -1490,6 +1549,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	1
 
 /*
  * Retrieve statement statistics.
@@ -1862,6 +1922,19 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 	tuplestore_donestoring(tupstore);
 }
 
+Datum
+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+	int64		d_count = 0;
+	{
+		volatile pgssInfoCounters *c = (volatile pgssInfoCounters *) pgss_info;
+		SpinLockAcquire(&c->mutex);
+		d_count = Int64GetDatum(c->dealloc);
+		SpinLockRelease(&c->mutex);
+	}
+	PG_RETURN_INT64(d_count);
+}
+
 /*
  * Estimate shared memory space needed.
  */
@@ -1872,6 +1945,7 @@ pgss_memsize(void)
 
 	size = MAXALIGN(sizeof(pgssSharedState));
 	size = add_size(size, hash_estimate_size(pgss_max, sizeof(pgssEntry)));
+	size = add_size(size, MAXALIGN(sizeof(pgssInfoCounters)));
 
 	return size;
 }
@@ -1902,7 +1976,12 @@ entry_alloc(pgssHashKey *key, Size query_offset, int query_len, int encoding,
 
 	/* Make space if needed */
 	while (hash_get_num_entries(pgss_hash) >= pgss_max)
+	{
 		entry_dealloc();
+		pgss_info_update();
+		ereport(LOG,
+			(errmsg("The information in pg_stat_statements has been deallocated.")));
+	}
 
 	/* Find or create an entry with desired hash code */
 	entry = (pgssEntry *) hash_search(pgss_hash, key, HASH_ENTER, &found);
@@ -2503,6 +2582,9 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
 			hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL);
 			num_remove++;
 		}
+
+		/* Reset pgss_info */
+		pgss_info_reset();
 	}
 
 	/* All entries are removed? */
diff --git a/contrib/pg_stat_statements/pg_stat_statements.control b/contrib/pg_stat_statements/pg_stat_statements.control
index 65b18b11d2..2f1ce6ed50 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.control
+++ b/contrib/pg_stat_statements/pg_stat_statements.control
@@ -1,5 +1,5 @@
 # pg_stat_statements extension
 comment = 'track planning and execution statistics of all SQL statements executed'
-default_version = '1.8'
+default_version = '1.9'
 module_pathname = '$libdir/pg_stat_statements'
 relocatable = true
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index 996a24a293..e3cb0930d9 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -357,4 +357,10 @@ SELECT 42;
 SELECT 42;
 SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
 
+--
+-- Checking the execution of the pg_stat_statements_info view
+--
+SELECT pg_stat_statements_reset(0,0,0);
+SELECT * FROM pg_stat_statements_info;
+
 DROP EXTENSION pg_stat_statements;
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index cf2d25b7b2..b3c53dd39f 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -23,10 +23,11 @@
  <para>
    When <filename>pg_stat_statements</filename> is loaded, it tracks
    statistics across all databases of the server.  To access and manipulate
-   these statistics, the module provides a view, <structname>pg_stat_statements</structname>,
-   and the utility functions <function>pg_stat_statements_reset</function> and
-   <function>pg_stat_statements</function>.  These are not available globally but
-   can be enabled for a specific database with
+   these statistics, the module provides views, <structname>pg_stat_statements</structname>
+   and <structname>pg_stat_statements_info</structname>,
+   and the utility functions <function>pg_stat_statements_reset</function>,
+   <function>pg_stat_statements</function>, and <function>pg_stat_statements_info</function>.
+   These are not available globally but can be enabled for a specific database with
    <command>CREATE EXTENSION pg_stat_statements</command>.
  </para>
 
@@ -480,6 +481,48 @@
   </para>
  </sect2>
 
+ <sect2>
+  <title>The <structname>pg_stat_statements_info</structname> View</title>
+
+  <para>
+   This module tracks statistics of <filename>pg_stat_statements</filename>
+   itself for detailed performance analysis.
+   The statistics of pg_stat_statements itself are made available via a
+   view named <structname>pg_stat_statements_info</structname>.
+   This view contains only a single row.
+   The row is reset only when <function>pg_stat_statements_reset(0,0,0)</function>
+   is called. The columns of the view are
+   shown in <xref linkend="pgstatstatementsctl-columns"/>.
+  </para>
+
+  <table id="pgstatstatementsctl-columns">
+   <title><structname>pg_stat_statements_info</structname> Columns</title>
+   <tgroup cols="1">
+    <thead>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       Column Type
+      </para>
+      <para>
+       Description
+      </para></entry>
+     </row>
+    </thead>
+
+    <tbody>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>dealloc</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Total number of deallocations of pg_stat_statements view entry
+      </para></entry>
+     </row>
+    </tbody>
+   </tgroup>
+  </table>
+ </sect2>
+
  <sect2>
   <title>Functions</title>
 
@@ -537,6 +580,24 @@
      </para>
     </listitem>
    </varlistentry>
+
+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns bigint</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>
+
+    <listitem>
+     <para>
+      The <structname>pg_stat_statements_info</structname> view is defined
+      in terms of a function also named <function>pg_stat_statements_info</function>.
+      It is possible for clients to call the 
+      <function>pg_stat_statements_info</function> directly.
+     </para>
+    </listitem>
+   </varlistentry>
   </variablelist>
  </sect2>
 
#18Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Seino Yuki (#17)
Re: [PATCH] Add features to pg_stat_statements

On 2020/11/09 18:02, Seino Yuki wrote:

2020-11-09 15:39 に Seino Yuki さんは書きました:

However, let me confirm the following.

Is this information really useful?
If there is no valid use case for this, I'd like to drop it.
Thought?

I thought it would be easy for users to see at a glance that if there is a case I assumed,
if the last modified date and time is old, there is no need to adjust at all, and if the
last modified date and time is recent, it would be easy for users to understand that the
parameters need to be adjusted.
What do you think?

Even without the last deallocation timestamp, you can presume
when the deallocation of entries happened by periodically monitoring
the total number of deallocations and seeing those history. Or IMO
it's better to log whenever the deallocation happens as proposed upthread.
Then you can easily check the history of occurrences of deallocations
from the log file.

Regards,

+1.I think you should output the deallocation history to the log as well.

In light of the above, I've posted a patch that reflects the points made.

Regards,

Sorry. There was a typo in the documentation correction.
I'll post a patch to fix it.

Thanks for updating the patch!

+		pgss_info->dealloc = 0;
+		SpinLockInit(&pgss_info->mutex);
+		Assert(pgss_info->dealloc == 0);

Why is this assertion check necessary? It seems not necessary.

+ {
+ Assert(found == found_info);

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what about
including pgssInfoCounters in pgssSharedState?

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

+	/* Read pgss_info */
+	if (feof(file) == 0)
+		if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+			goto read_error;

Why does feof(file) need to be called here?

+pgss_info_update(void)
+{
+	{

Why is the second "{" necessary? It seems redundant.

+pgss_info_reset(void)
+{
+	{

Same as above.

+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+	int64		d_count = 0;
+	{

Same as above.

+		SpinLockAcquire(&c->mutex);
+		d_count = Int64GetDatum(c->dealloc);
+		SpinLockRelease(&c->mutex);

Why does Int64GetDatum() need to be called here? It seems not necessary.

+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns bigint</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>

Isn't it better not to expose pg_stat_statements_info() function in the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#19Seino Yuki
seinoyu@oss.nttdata.com
In reply to: Fujii Masao (#18)
1 attachment(s)
Re: [PATCH] Add features to pg_stat_statements

Thanks for updating the patch!

+		pgss_info->dealloc = 0;
+		SpinLockInit(&pgss_info->mutex);
+		Assert(pgss_info->dealloc == 0);

Why is this assertion check necessary? It seems not necessary.

+ {
+ Assert(found == found_info);

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what
about
including pgssInfoCounters in pgssSharedState?

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

+	/* Read pgss_info */
+	if (feof(file) == 0)
+		if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+			goto read_error;

Why does feof(file) need to be called here?

+pgss_info_update(void)
+{
+	{

Why is the second "{" necessary? It seems redundant.

+pgss_info_reset(void)
+{
+	{

Same as above.

+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+	int64		d_count = 0;
+	{

Same as above.

+		SpinLockAcquire(&c->mutex);
+		d_count = Int64GetDatum(c->dealloc);
+		SpinLockRelease(&c->mutex);

Why does Int64GetDatum() need to be called here? It seems not
necessary.

+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns bigint</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>

Isn't it better not to expose pg_stat_statements_info() function in the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

Regards,

Thanks for the comment.
I'll post a fixed patch.
Due to similar fixed, we have also merged the patches discussed in the
following thread.
https://commitfest.postgresql.org/30/2738/

Why is this assertion check necessary? It seems not necessary.

As indicated, it is unnecessary and will be removed.

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what
about
including pgssInfoCounters in pgssSharedState?

Fix pgssSharedState to include pgssInfoCounters . The related parts were
also corrected accordingly.

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

The value of PGSS_FILE_HEADER has been updated.

Why does feof(file) need to be called here?

As indicated, it is unnecessary and will be removed.

Why is the second "{" necessary? It seems redundant.

As indicated, it is unnecessary and will be removed.
But I left the {} in pg_stat_statements_info() to make the shared memory
edit part explicit.

Why does Int64GetDatum() need to be called here? It seems not
necessary.

As indicated, it is unnecessary and will be removed.

Isn't it better not to expose pg_stat_statements_info() function in the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

As indicated, it is unnecessary and will be removed.

Regards.

Attachments:

pg_stat_statements_info_v2.patchtext/x-diff; name=pg_stat_statements_info_v2.patchDownload
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 081f997d70..3ec627b956 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -6,7 +6,7 @@ OBJS = \
 	pg_stat_statements.o
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql \
 	pg_stat_statements--1.7--1.8.sql pg_stat_statements--1.6--1.7.sql \
 	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0edb134f3..a1eaedca8e 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -859,4 +859,19 @@ SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE
  SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" |     1 |     0 |    0
 (6 rows)
 
+--
+-- Checking the execution of the pg_stat_statements_info view
+--
+SELECT pg_stat_statements_reset(0,0,0);
+ pg_stat_statements_reset 
+--------------------------
+ 
+(1 row)
+
+SELECT dealloc FROM pg_stat_statements_info;
+ dealloc 
+---------
+       0
+(1 row)
+
 DROP EXTENSION pg_stat_statements;
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
new file mode 100644
index 0000000000..4edbc03dc1
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
@@ -0,0 +1,18 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.9'" to load this file. \quit
+
+--- Define pg_stat_statements_info
+CREATE FUNCTION pg_stat_statements_info (
+	OUT dealloc bigint,
+	OUT reset_exec_time TIMESTAMP WITH TIME ZONE
+)
+RETURNS record
+AS 'MODULE_PATHNAME'
+LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
+
+CREATE VIEW pg_stat_statements_info AS
+	SELECT * FROM pg_stat_statements_info();
+
+GRANT SELECT ON pg_stat_statements_info TO PUBLIC;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 1eac9edaee..0857c1e2a4 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;
 
@@ -98,7 +99,7 @@ PG_MODULE_MAGIC;
 #define PGSS_TEXT_FILE	PG_STAT_TMP_DIR "/pgss_query_texts.stat"
 
 /* Magic number identifying the stats file format */
-static const uint32 PGSS_FILE_HEADER = 0x20171004;
+static const uint32 PGSS_FILE_HEADER = 0x20201116;
 
 /* PostgreSQL major version number, changes in which invalidate all entries */
 static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
@@ -193,6 +194,17 @@ typedef struct Counters
 	uint64		wal_bytes;		/* total amount of WAL bytes generated */
 } Counters;
 
+/*
+ * Counter for pg_stat_statements_info
+ */
+typedef struct pgssInfoCounters
+{
+	int64		dealloc;					/* # of deallocation */
+	TimestampTz reset_exec_time;    				/* timestamp with all stats reset */
+	bool		reset_exec_time_isnull;		/* if true last_dealloc is null */
+	slock_t		mutex;						/* protects the counters only */
+} pgssInfoCounters;
+
 /*
  * Statistics per statement
  *
@@ -222,6 +234,7 @@ typedef struct pgssSharedState
 	Size		extent;			/* current extent of query file */
 	int			n_writers;		/* number of active writers to query file */
 	int			gc_count;		/* query file garbage collection cycle count */
+	pgssInfoCounters	pgss_info;	/* Counter for pg_stat_statements_info */
 } pgssSharedState;
 
 /*
@@ -327,6 +340,7 @@ PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_3);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_8);
 PG_FUNCTION_INFO_V1(pg_stat_statements);
+PG_FUNCTION_INFO_V1(pg_stat_statements_info);
 
 static void pgss_shmem_startup(void);
 static void pgss_shmem_shutdown(int code, Datum arg);
@@ -554,6 +568,9 @@ pgss_shmem_startup(void)
 		pgss->extent = 0;
 		pgss->n_writers = 0;
 		pgss->gc_count = 0;
+		pgss->pgss_info.dealloc = 0;
+		pgss->pgss_info.reset_exec_time = 0;
+		pgss->pgss_info.reset_exec_time_isnull = true;
 	}
 
 	memset(&info, 0, sizeof(info));
@@ -673,6 +690,10 @@ pgss_shmem_startup(void)
 		entry->counters = temp.counters;
 	}
 
+	/* Read pgss_info */
+	if (fread(&pgss->pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+		goto read_error;
+
 	pfree(buffer);
 	FreeFile(file);
 	FreeFile(qfile);
@@ -794,6 +815,10 @@ pgss_shmem_shutdown(int code, Datum arg)
 		}
 	}
 
+	/* Dump pgss_info */
+	if (fwrite(&pgss->pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+		goto error;
+
 	free(qbuffer);
 	qbuffer = NULL;
 
@@ -1490,6 +1515,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.
@@ -1862,6 +1888,42 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 	tuplestore_donestoring(tupstore);
 }
 
+Datum
+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+	TupleDesc	tupdesc;
+	HeapTuple	result_tuple;
+	Datum		result;
+	Datum 		values[PG_STAT_STATEMENTS_INFO_COLS];
+	bool 		nulls[PG_STAT_STATEMENTS_INFO_COLS];
+	bool		ts_isnull;
+	int 		i = 0;
+
+	Assert(PG_STAT_STATEMENTS_INFO_COLS == 2);
+
+	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+		elog(ERROR, "return type must be a row type");
+
+	tupdesc = BlessTupleDesc(tupdesc);
+
+	memset(nulls, 0, sizeof(nulls));
+	{
+		volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
+		SpinLockAcquire(&s->mutex);
+		values[i++] = pgss->pgss_info.dealloc;
+		ts_isnull = pgss->pgss_info.reset_exec_time_isnull;
+		if (!ts_isnull)
+			values[i++] = pgss->pgss_info.reset_exec_time;
+		else
+			nulls[i++] = true;
+		SpinLockRelease(&s->mutex);
+	}
+	result_tuple = heap_form_tuple(tupdesc, values, nulls);
+	result = HeapTupleGetDatum(result_tuple);
+
+	return result;
+}
+
 /*
  * Estimate shared memory space needed.
  */
@@ -1902,7 +1964,18 @@ entry_alloc(pgssHashKey *key, Size query_offset, int query_len, int encoding,
 
 	/* Make space if needed */
 	while (hash_get_num_entries(pgss_hash) >= pgss_max)
+	{
 		entry_dealloc();
+		/* Update pgss_info */
+		{
+			volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
+			SpinLockAcquire(&s->mutex);
+			s->pgss_info.dealloc += 1;	/* increment dealloc count */
+			SpinLockRelease(&s->mutex);
+		}
+		ereport(LOG,
+			(errmsg("The information in pg_stat_statements has been deallocated.")));
+	}
 
 	/* Find or create an entry with desired hash code */
 	entry = (pgssEntry *) hash_search(pgss_hash, key, HASH_ENTER, &found);
@@ -2458,6 +2531,9 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
 	long		num_entries;
 	long		num_remove = 0;
 	pgssHashKey key;
+	TimestampTz reset_ts;
+	
+	reset_ts = GetCurrentTimestamp();
 
 	if (!pgss || !pgss_hash)
 		ereport(ERROR,
@@ -2503,6 +2579,16 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
 			hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL);
 			num_remove++;
 		}
+
+		/* Reset and Update pgss_info */
+		{
+			volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
+			SpinLockAcquire(&s->mutex);
+			s->pgss_info.dealloc = 0;					/* reset dealloc count */
+			s->pgss_info.reset_exec_time = reset_ts;	/* reset execution time */
+			s->pgss_info.reset_exec_time_isnull = false;
+			SpinLockRelease(&s->mutex);
+		}
 	}
 
 	/* All entries are removed? */
diff --git a/contrib/pg_stat_statements/pg_stat_statements.control b/contrib/pg_stat_statements/pg_stat_statements.control
index 65b18b11d2..2f1ce6ed50 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.control
+++ b/contrib/pg_stat_statements/pg_stat_statements.control
@@ -1,5 +1,5 @@
 # pg_stat_statements extension
 comment = 'track planning and execution statistics of all SQL statements executed'
-default_version = '1.8'
+default_version = '1.9'
 module_pathname = '$libdir/pg_stat_statements'
 relocatable = true
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index 996a24a293..4016d19553 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -357,4 +357,10 @@ SELECT 42;
 SELECT 42;
 SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
 
+--
+-- Checking the execution of the pg_stat_statements_info view
+--
+SELECT pg_stat_statements_reset(0,0,0);
+SELECT dealloc FROM pg_stat_statements_info;
+
 DROP EXTENSION pg_stat_statements;
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index cf2d25b7b2..21f150aabb 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -23,7 +23,8 @@
  <para>
    When <filename>pg_stat_statements</filename> is loaded, it tracks
    statistics across all databases of the server.  To access and manipulate
-   these statistics, the module provides a view, <structname>pg_stat_statements</structname>,
+   these statistics, the module provides views, <structname>pg_stat_statements</structname>
+   and <structname>pg_stat_statements_info</structname>,
    and the utility functions <function>pg_stat_statements_reset</function> and
    <function>pg_stat_statements</function>.  These are not available globally but
    can be enabled for a specific database with
@@ -480,6 +481,55 @@
   </para>
  </sect2>
 
+ <sect2>
+  <title>The <structname>pg_stat_statements_info</structname> View</title>
+
+  <para>
+   This module tracks statistics of <filename>pg_stat_statements</filename>
+   itself for detailed performance analysis.
+   The statistics of pg_stat_statements itself are made available via a
+   view named <structname>pg_stat_statements_info</structname>.
+   This view contains only a single row.
+   The columns of the view are shown in <xref linkend="pgstatstatementsinfo-columns"/>.
+  </para>
+
+  <table id="pgstatstatementsinfo-columns">
+   <title><structname>pg_stat_statements_info</structname> Columns</title>
+   <tgroup cols="1">
+    <thead>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       Column Type
+      </para>
+      <para>
+       Description
+      </para></entry>
+     </row>
+    </thead>
+
+    <tbody>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>dealloc</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Total number of deallocations of pg_stat_statements view entry
+      </para></entry>
+     </row>
+     
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>reset_exec_time</structfield> <type>timestamp with time zone</type>
+      </para>
+      <para>
+       Shows the time at which <function>pg_stat_statements_reset(0,0,0)</function> was last called.
+      </para></entry>
+     </row>
+    </tbody>
+   </tgroup>
+  </table>
+ </sect2>
+
  <sect2>
   <title>Functions</title>
 
#20Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Seino Yuki (#19)
1 attachment(s)
Re: [PATCH] Add features to pg_stat_statements

On 2020/11/16 12:22, Seino Yuki wrote:

Thanks for updating the patch!

+������� pgss_info->dealloc = 0;
+������� SpinLockInit(&pgss_info->mutex);
+������� Assert(pgss_info->dealloc == 0);

Why is this assertion check necessary? It seems not necessary.

+��� {
+������� Assert(found == found_info);

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what about
including pgssInfoCounters in pgssSharedState?

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

+��� /* Read pgss_info */
+��� if (feof(file) == 0)
+������� if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+����������� goto read_error;

Why does feof(file) need to be called here?

+pgss_info_update(void)
+{
+��� {

Why is the second "{" necessary? It seems redundant.

+pgss_info_reset(void)
+{
+��� {

Same as above.

+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+��� int64������� d_count = 0;
+��� {

Same as above.

+������� SpinLockAcquire(&c->mutex);
+������� d_count = Int64GetDatum(c->dealloc);
+������� SpinLockRelease(&c->mutex);

Why does Int64GetDatum() need to be called here? It seems not necessary.

+�� <varlistentry>
+��� <term>
+���� <function>pg_stat_statements_info() returns bigint</function>
+���� <indexterm>
+����� <primary>pg_stat_statements_info</primary>
+���� </indexterm>
+��� </term>

Isn't it better not to expose pg_stat_statements_info() function in the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

Regards,

Thanks for the comment.
I'll post a fixed patch.
Due to similar fixed, we have also merged the patches discussed in the following thread.
https://commitfest.postgresql.org/30/2738/

I agree that these two patches should use the same infrastructure
because they both try to add the global stats for pg_stat_statements.
But IMO they should not be merged to one patch. It's better to
develop them one by one for ease of review. Thought?

So I extracted the "dealloc" part from the merged version of your patch.
Also I refactored the code and applied some cosmetic changes into
the patch. Attached is the updated version of the patch that implements
only "dealloc" part. Could you review this version?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Attachments:

pg_stat_statements_info_v3.patchtext/plain; charset=UTF-8; name=pg_stat_statements_info_v3.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 081f997d70..3ec627b956 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -6,7 +6,7 @@ OBJS = \
 	pg_stat_statements.o
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql \
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql \
 	pg_stat_statements--1.7--1.8.sql pg_stat_statements--1.6--1.7.sql \
 	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 2a303a7f07..16158525ca 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -861,4 +861,19 @@ SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE
  SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" |     1 |     0 |    0
 (6 rows)
 
+--
+-- access to pg_stat_statements_info view
+--
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset 
+--------------------------
+ 
+(1 row)
+
+SELECT dealloc FROM pg_stat_statements_info;
+ dealloc 
+---------
+       0
+(1 row)
+
 DROP EXTENSION pg_stat_statements;
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
new file mode 100644
index 0000000000..2019a4ffe0
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql
@@ -0,0 +1,17 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.8--1.9.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.9'" to load this file. \quit
+
+--- Define pg_stat_statements_info
+CREATE FUNCTION pg_stat_statements_info(
+    OUT dealloc bigint
+)
+RETURNS bigint
+AS 'MODULE_PATHNAME'
+LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
+
+CREATE VIEW pg_stat_statements_info AS
+  SELECT * FROM pg_stat_statements_info();
+
+GRANT SELECT ON pg_stat_statements_info TO PUBLIC;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index dd963c4644..9de2d53496 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -98,7 +98,7 @@ PG_MODULE_MAGIC;
 #define PGSS_TEXT_FILE	PG_STAT_TMP_DIR "/pgss_query_texts.stat"
 
 /* Magic number identifying the stats file format */
-static const uint32 PGSS_FILE_HEADER = 0x20171004;
+static const uint32 PGSS_FILE_HEADER = 0x20201116;
 
 /* PostgreSQL major version number, changes in which invalidate all entries */
 static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
@@ -193,6 +193,14 @@ typedef struct Counters
 	uint64		wal_bytes;		/* total amount of WAL bytes generated */
 } Counters;
 
+/*
+ * Global statistics for pg_stat_statements
+ */
+typedef struct pgssGlobalStats
+{
+	int64		dealloc;		/* # of times entries were deallocated */
+} pgssGlobalStats;
+
 /*
  * Statistics per statement
  *
@@ -222,6 +230,7 @@ typedef struct pgssSharedState
 	Size		extent;			/* current extent of query file */
 	int			n_writers;		/* number of active writers to query file */
 	int			gc_count;		/* query file garbage collection cycle count */
+	pgssGlobalStats stats;		/* global statistics for pgss */
 } pgssSharedState;
 
 /*
@@ -327,6 +336,7 @@ PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_3);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_8);
 PG_FUNCTION_INFO_V1(pg_stat_statements);
+PG_FUNCTION_INFO_V1(pg_stat_statements_info);
 
 static void pgss_shmem_startup(void);
 static void pgss_shmem_shutdown(int code, Datum arg);
@@ -554,6 +564,7 @@ pgss_shmem_startup(void)
 		pgss->extent = 0;
 		pgss->n_writers = 0;
 		pgss->gc_count = 0;
+		pgss->stats.dealloc = 0;
 	}
 
 	memset(&info, 0, sizeof(info));
@@ -673,6 +684,10 @@ pgss_shmem_startup(void)
 		entry->counters = temp.counters;
 	}
 
+	/* Read global statistics for pg_stat_statements */
+	if (fread(&pgss->stats, sizeof(pgssGlobalStats), 1, file) != 1)
+		goto read_error;
+
 	pfree(buffer);
 	FreeFile(file);
 	FreeFile(qfile);
@@ -794,6 +809,10 @@ pgss_shmem_shutdown(int code, Datum arg)
 		}
 	}
 
+	/* Dump global statistics for pg_stat_statements */
+	if (fwrite(&pgss->stats, sizeof(pgssGlobalStats), 1, file) != 1)
+		goto error;
+
 	free(qbuffer);
 	qbuffer = NULL;
 
@@ -1863,6 +1882,26 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 	tuplestore_donestoring(tupstore);
 }
 
+/*
+ * Return statistics of pg_stat_statements.
+ */
+Datum
+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+	pgssGlobalStats stats;
+
+	/* Read global statistics for pg_stat_statements */
+	{
+		volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
+
+		SpinLockAcquire(&s->mutex);
+		stats = s->stats;
+		SpinLockRelease(&s->mutex);
+	}
+
+	PG_RETURN_INT64(stats.dealloc);
+}
+
 /*
  * Estimate shared memory space needed.
  */
@@ -2018,6 +2057,15 @@ entry_dealloc(void)
 	}
 
 	pfree(entries);
+
+	/* Increment the number of times entries are deallocated */
+	{
+		volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
+
+		SpinLockAcquire(&s->mutex);
+		s->stats.dealloc += 1;
+		SpinLockRelease(&s->mutex);
+	}
 }
 
 /*
@@ -2504,6 +2552,15 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
 			hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL);
 			num_remove++;
 		}
+
+		/* Reset global statistics for pg_stat_statements */
+		{
+			volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
+
+			SpinLockAcquire(&s->mutex);
+			s->stats.dealloc = 0;
+			SpinLockRelease(&s->mutex);
+		}
 	}
 
 	/* All entries are removed? */
diff --git a/contrib/pg_stat_statements/pg_stat_statements.control b/contrib/pg_stat_statements/pg_stat_statements.control
index 65b18b11d2..2f1ce6ed50 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.control
+++ b/contrib/pg_stat_statements/pg_stat_statements.control
@@ -1,5 +1,5 @@
 # pg_stat_statements extension
 comment = 'track planning and execution statistics of all SQL statements executed'
-default_version = '1.8'
+default_version = '1.9'
 module_pathname = '$libdir/pg_stat_statements'
 relocatable = true
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index e9f5bb84e3..6f58d9d0f6 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -358,4 +358,10 @@ SELECT 42;
 SELECT 42;
 SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
 
+--
+-- access to pg_stat_statements_info view
+--
+SELECT pg_stat_statements_reset();
+SELECT dealloc FROM pg_stat_statements_info;
+
 DROP EXTENSION pg_stat_statements;
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index cf2d25b7b2..b0c8d6f0f1 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -23,7 +23,9 @@
  <para>
    When <filename>pg_stat_statements</filename> is loaded, it tracks
    statistics across all databases of the server.  To access and manipulate
-   these statistics, the module provides a view, <structname>pg_stat_statements</structname>,
+   these statistics, the module provides views
+   <structname>pg_stat_statements</structname> and
+   <structname>pg_stat_statements_info</structname>,
    and the utility functions <function>pg_stat_statements_reset</function> and
    <function>pg_stat_statements</function>.  These are not available globally but
    can be enabled for a specific database with
@@ -480,6 +482,50 @@
   </para>
  </sect2>
 
+ <sect2>
+  <title>The <structname>pg_stat_statements_info</structname> View</title>
+
+  <indexterm>
+   <primary>pg_stat_statements_info</primary>
+  </indexterm>
+
+  <para>
+   The statistics of the <filename>pg_stat_statements</filename> module
+   itself are tracked and made available via a view named
+   <structname>pg_stat_statements_info</structname>.  This view contains
+   only a single row.  The columns of the view are shown in
+   <xref linkend="pgstatstatementsinfo-columns"/>.
+  </para>
+
+  <table id="pgstatstatementsinfo-columns">
+   <title><structname>pg_stat_statements_info</structname> Columns</title>
+   <tgroup cols="1">
+    <thead>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       Column Type
+      </para>
+      <para>
+       Description
+      </para></entry>
+     </row>
+    </thead>
+
+    <tbody>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>dealloc</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Total number of times <structname>pg_stat_statements</structname>
+       entries were deallocated
+      </para></entry>
+     </row>
+    </tbody>
+   </tgroup>
+  </table>
+ </sect2>
+
  <sect2>
   <title>Functions</title>
 
@@ -501,9 +547,10 @@
       specified, the default value <literal>0</literal>(invalid) is used for
       each of them and the statistics that match with other parameters will be
       reset.  If no parameter is specified or all the specified parameters are
-      <literal>0</literal>(invalid), it will discard all statistics.  By
-      default, this function can only be executed by superusers.  Access may be
-      granted to others using <command>GRANT</command>.
+      <literal>0</literal>(invalid), it will discard all statistics including
+      the statistics that <structname>pg_stat_statements_info</structname>
+      displays.  By default, this function can only be executed by superusers.
+      Access may be granted to others using <command>GRANT</command>.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index b146b3ea73..65f59ea09f 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3215,6 +3215,7 @@ pgpid_t
 pgsocket
 pgsql_thing_t
 pgssEntry
+pgssGlobalStats
 pgssHashKey
 pgssJumbleState
 pgssLocationLen
#21Seino Yuki
seinoyu@oss.nttdata.com
In reply to: Fujii Masao (#20)
Re: [PATCH] Add features to pg_stat_statements

2020-11-17 01:46 に Fujii Masao さんは書きました:

On 2020/11/16 12:22, Seino Yuki wrote:

Thanks for updating the patch!

+        pgss_info->dealloc = 0;
+        SpinLockInit(&pgss_info->mutex);
+        Assert(pgss_info->dealloc == 0);

Why is this assertion check necessary? It seems not necessary.

+    {
+        Assert(found == found_info);

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what
about
including pgssInfoCounters in pgssSharedState?

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

+    /* Read pgss_info */
+    if (feof(file) == 0)
+        if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 
1)
+            goto read_error;

Why does feof(file) need to be called here?

+pgss_info_update(void)
+{
+    {

Why is the second "{" necessary? It seems redundant.

+pgss_info_reset(void)
+{
+    {

Same as above.

+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+    int64        d_count = 0;
+    {

Same as above.

+        SpinLockAcquire(&c->mutex);
+        d_count = Int64GetDatum(c->dealloc);
+        SpinLockRelease(&c->mutex);

Why does Int64GetDatum() need to be called here? It seems not
necessary.

+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns bigint</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>

Isn't it better not to expose pg_stat_statements_info() function in
the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

Regards,

Thanks for the comment.
I'll post a fixed patch.
Due to similar fixed, we have also merged the patches discussed in the
following thread.
https://commitfest.postgresql.org/30/2738/

I agree that these two patches should use the same infrastructure
because they both try to add the global stats for pg_stat_statements.
But IMO they should not be merged to one patch. It's better to
develop them one by one for ease of review. Thought?

So I extracted the "dealloc" part from the merged version of your
patch.
Also I refactored the code and applied some cosmetic changes into
the patch. Attached is the updated version of the patch that implements
only "dealloc" part. Could you review this version?

Regards,

Thank you for posting the new patch.

I checked "regression test" and "document" and "operation of the view".
No particular problems were found.

I just want to check one thing: will the log output be unnecessary this
time?
Quotes from v2.patch

+	{
entry_dealloc();
+		/* Update pgss_info */
+		{
+			volatile pgssSharedState *s = (volatile pgssSharedState *) pgss;
+			SpinLockAcquire(&s->mutex);
+			s->pgss_info.dealloc += 1;	/* increment dealloc count */
+			SpinLockRelease(&s->mutex);
+		}
+		ereport(LOG,
+			(errmsg("The information in pg_stat_statements has been 
deallocated.")));
+	}

Regards.

#22Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Seino Yuki (#21)
Re: [PATCH] Add features to pg_stat_statements

On 2020/11/25 12:02, Seino Yuki wrote:

2020-11-17 01:46 に Fujii Masao さんは書きました:

On 2020/11/16 12:22, Seino Yuki wrote:

Thanks for updating the patch!

+        pgss_info->dealloc = 0;
+        SpinLockInit(&pgss_info->mutex);
+        Assert(pgss_info->dealloc == 0);

Why is this assertion check necessary? It seems not necessary.

+    {
+        Assert(found == found_info);

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what about
including pgssInfoCounters in pgssSharedState?

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

+    /* Read pgss_info */
+    if (feof(file) == 0)
+        if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+            goto read_error;

Why does feof(file) need to be called here?

+pgss_info_update(void)
+{
+    {

Why is the second "{" necessary? It seems redundant.

+pgss_info_reset(void)
+{
+    {

Same as above.

+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+    int64        d_count = 0;
+    {

Same as above.

+        SpinLockAcquire(&c->mutex);
+        d_count = Int64GetDatum(c->dealloc);
+        SpinLockRelease(&c->mutex);

Why does Int64GetDatum() need to be called here? It seems not necessary.

+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns bigint</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>

Isn't it better not to expose pg_stat_statements_info() function in the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

Regards,

Thanks for the comment.
I'll post a fixed patch.
Due to similar fixed, we have also merged the patches discussed in the following thread.
https://commitfest.postgresql.org/30/2738/

I agree that these two patches should use the same infrastructure
because they both try to add the global stats for pg_stat_statements.
But IMO they should not be merged to one patch. It's better to
develop them one by one for ease of review. Thought?

So I extracted the "dealloc" part from the merged version of your patch.
Also I refactored the code and applied some cosmetic changes into
the patch. Attached is the updated version of the patch that implements
only "dealloc" part. Could you review this version?

Regards,

Thank you for posting the new patch.

I checked "regression test" and "document" and "operation of the view".
No particular problems were found.

Thanks for the review and test!
So you think that this patch can be marked as ready for committer?

I just want to check one thing: will the log output be unnecessary this time?
Quotes from v2.patch

I'm not sure if it's really good idea to add this log message.
If we adopt that logging, in the system where pgss entries are deallocated
very frequently, that message also would be logged very frequently.
Such too many log messages might be noisy to users. To address this issue,
we may want to add new parameter that controls whether log message is
emitted or not when entries are deallocated. But that parameter sounds
too specific... Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#23Seino Yuki
seinoyu@oss.nttdata.com
In reply to: Fujii Masao (#22)
Re: [PATCH] Add features to pg_stat_statements

2020-11-25 13:13 に Fujii Masao さんは書きました:

On 2020/11/25 12:02, Seino Yuki wrote:

2020-11-17 01:46 に Fujii Masao さんは書きました:

On 2020/11/16 12:22, Seino Yuki wrote:

Thanks for updating the patch!

+        pgss_info->dealloc = 0;
+        SpinLockInit(&pgss_info->mutex);
+        Assert(pgss_info->dealloc == 0);

Why is this assertion check necessary? It seems not necessary.

+    {
+        Assert(found == found_info);

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true,
what about
including pgssInfoCounters in pgssSharedState?

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

+    /* Read pgss_info */
+    if (feof(file) == 0)
+        if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 
1)
+            goto read_error;

Why does feof(file) need to be called here?

+pgss_info_update(void)
+{
+    {

Why is the second "{" necessary? It seems redundant.

+pgss_info_reset(void)
+{
+    {

Same as above.

+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+    int64        d_count = 0;
+    {

Same as above.

+        SpinLockAcquire(&c->mutex);
+        d_count = Int64GetDatum(c->dealloc);
+        SpinLockRelease(&c->mutex);

Why does Int64GetDatum() need to be called here? It seems not
necessary.

+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns bigint</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>

Isn't it better not to expose pg_stat_statements_info() function in
the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

Regards,

Thanks for the comment.
I'll post a fixed patch.
Due to similar fixed, we have also merged the patches discussed in
the following thread.
https://commitfest.postgresql.org/30/2738/

I agree that these two patches should use the same infrastructure
because they both try to add the global stats for pg_stat_statements.
But IMO they should not be merged to one patch. It's better to
develop them one by one for ease of review. Thought?

So I extracted the "dealloc" part from the merged version of your
patch.
Also I refactored the code and applied some cosmetic changes into
the patch. Attached is the updated version of the patch that
implements
only "dealloc" part. Could you review this version?

Regards,

Thank you for posting the new patch.

I checked "regression test" and "document" and "operation of the
view".
No particular problems were found.

Thanks for the review and test!
So you think that this patch can be marked as ready for committer?

I just want to check one thing: will the log output be unnecessary
this time?
Quotes from v2.patch

I'm not sure if it's really good idea to add this log message.
If we adopt that logging, in the system where pgss entries are
deallocated
very frequently, that message also would be logged very frequently.
Such too many log messages might be noisy to users. To address this
issue,
we may want to add new parameter that controls whether log message is
emitted or not when entries are deallocated. But that parameter sounds
too specific... Thought?

Regards,

Thanks for the review and test!
So you think that this patch can be marked as ready for committer?

Updated status to ready for committer.

I'm not sure if it's really good idea to add this log message.
If we adopt that logging, in the system where pgss entries are
deallocated
very frequently, that message also would be logged very frequently.
Such too many log messages might be noisy to users. To address this
issue,
we may want to add new parameter that controls whether log message is
emitted or not when entries are deallocated. But that parameter sounds
too specific... Thought?

I think it's no problem to add the log after the user requests it.
Usually I think you should tune pg_stat_statements.max if you have
frequent deallocated.
However, most users may not be that aware of it.
Let's not add a log this time.

Regards.

#24Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Seino Yuki (#23)
Re: [PATCH] Add features to pg_stat_statements

On 2020/11/25 15:40, Seino Yuki wrote:

2020-11-25 13:13 に Fujii Masao さんは書きました:

On 2020/11/25 12:02, Seino Yuki wrote:

2020-11-17 01:46 に Fujii Masao さんは書きました:

On 2020/11/16 12:22, Seino Yuki wrote:

Thanks for updating the patch!

+        pgss_info->dealloc = 0;
+        SpinLockInit(&pgss_info->mutex);
+        Assert(pgss_info->dealloc == 0);

Why is this assertion check necessary? It seems not necessary.

+    {
+        Assert(found == found_info);

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what about
including pgssInfoCounters in pgssSharedState?

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

+    /* Read pgss_info */
+    if (feof(file) == 0)
+        if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+            goto read_error;

Why does feof(file) need to be called here?

+pgss_info_update(void)
+{
+    {

Why is the second "{" necessary? It seems redundant.

+pgss_info_reset(void)
+{
+    {

Same as above.

+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+    int64        d_count = 0;
+    {

Same as above.

+        SpinLockAcquire(&c->mutex);
+        d_count = Int64GetDatum(c->dealloc);
+        SpinLockRelease(&c->mutex);

Why does Int64GetDatum() need to be called here? It seems not necessary.

+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns bigint</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>

Isn't it better not to expose pg_stat_statements_info() function in the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

Regards,

Thanks for the comment.
I'll post a fixed patch.
Due to similar fixed, we have also merged the patches discussed in the following thread.
https://commitfest.postgresql.org/30/2738/

I agree that these two patches should use the same infrastructure
because they both try to add the global stats for pg_stat_statements.
But IMO they should not be merged to one patch. It's better to
develop them one by one for ease of review. Thought?

So I extracted the "dealloc" part from the merged version of your patch.
Also I refactored the code and applied some cosmetic changes into
the patch. Attached is the updated version of the patch that implements
only "dealloc" part. Could you review this version?

Regards,

Thank you for posting the new patch.

I checked "regression test" and "document" and "operation of the view".
No particular problems were found.

Thanks for the review and test!
So you think that this patch can be marked as ready for committer?

I just want to check one thing: will the log output be unnecessary this time?
Quotes from v2.patch

I'm not sure if it's really good idea to add this log message.
If we adopt that logging, in the system where pgss entries are deallocated
very frequently, that message also would be logged very frequently.
Such too many log messages might be noisy to users. To address this issue,
we may want to add new parameter that controls whether log message is
emitted or not when entries are deallocated. But that parameter sounds
too specific... Thought?

Regards,

Thanks for the review and test!
So you think that this patch can be marked as ready for committer?

Updated status to ready for committer.

Thanks!

I'm not sure if it's really good idea to add this log message.
If we adopt that logging, in the system where pgss entries are deallocated
very frequently, that message also would be logged very frequently.
Such too many log messages might be noisy to users. To address this issue,
we may want to add new parameter that controls whether log message is
emitted or not when entries are deallocated. But that parameter sounds
too specific... Thought?

I think it's no problem to add the log after the user requests it.
Usually I think you should tune pg_stat_statements.max if you have frequent deallocated.

Yes, but I guess that there are some users who cannot increase pgss.max
for some reasons (e.g., lack of memory). It seems annoying for them to
*always* log the deallocation of pgss entries.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#25Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Fujii Masao (#24)
Re: [PATCH] Add features to pg_stat_statements

On 2020/11/25 17:07, Fujii Masao wrote:

On 2020/11/25 15:40, Seino Yuki wrote:

2020-11-25 13:13 に Fujii Masao さんは書きました:

On 2020/11/25 12:02, Seino Yuki wrote:

2020-11-17 01:46 に Fujii Masao さんは書きました:

On 2020/11/16 12:22, Seino Yuki wrote:

Thanks for updating the patch!

+        pgss_info->dealloc = 0;
+        SpinLockInit(&pgss_info->mutex);
+        Assert(pgss_info->dealloc == 0);

Why is this assertion check necessary? It seems not necessary.

+    {
+        Assert(found == found_info);

Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what about
including pgssInfoCounters in pgssSharedState?

PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?

+    /* Read pgss_info */
+    if (feof(file) == 0)
+        if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) != 1)
+            goto read_error;

Why does feof(file) need to be called here?

+pgss_info_update(void)
+{
+    {

Why is the second "{" necessary? It seems redundant.

+pgss_info_reset(void)
+{
+    {

Same as above.

+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+    int64        d_count = 0;
+    {

Same as above.

+        SpinLockAcquire(&c->mutex);
+        d_count = Int64GetDatum(c->dealloc);
+        SpinLockRelease(&c->mutex);

Why does Int64GetDatum() need to be called here? It seems not necessary.

+   <varlistentry>
+    <term>
+     <function>pg_stat_statements_info() returns bigint</function>
+     <indexterm>
+      <primary>pg_stat_statements_info</primary>
+     </indexterm>
+    </term>

Isn't it better not to expose pg_stat_statements_info() function in the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?

Regards,

Thanks for the comment.
I'll post a fixed patch.
Due to similar fixed, we have also merged the patches discussed in the following thread.
https://commitfest.postgresql.org/30/2738/

I agree that these two patches should use the same infrastructure
because they both try to add the global stats for pg_stat_statements.
But IMO they should not be merged to one patch. It's better to
develop them one by one for ease of review. Thought?

So I extracted the "dealloc" part from the merged version of your patch.
Also I refactored the code and applied some cosmetic changes into
the patch. Attached is the updated version of the patch that implements
only "dealloc" part. Could you review this version?

Regards,

Thank you for posting the new patch.

I checked "regression test" and "document" and "operation of the view".
No particular problems were found.

Thanks for the review and test!
So you think that this patch can be marked as ready for committer?

I just want to check one thing: will the log output be unnecessary this time?
Quotes from v2.patch

I'm not sure if it's really good idea to add this log message.
If we adopt that logging, in the system where pgss entries are deallocated
very frequently, that message also would be logged very frequently.
Such too many log messages might be noisy to users. To address this issue,
we may want to add new parameter that controls whether log message is
emitted or not when entries are deallocated. But that parameter sounds
too specific... Thought?

Regards,

Thanks for the review and test!
So you think that this patch can be marked as ready for committer?

Updated status to ready for committer.

Thanks!

I modified the docs a bit and pushed the patch. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION