pgsql: Update pg_stat_statements extension for parallel query.

Started by Robert Haasover 9 years ago3 messages
#1Robert Haas
rhaas@postgresql.org

Update pg_stat_statements extension for parallel query.

All functions provided by this extension are PARALLEL SAFE. Given the
general prohibition against write operations in parallel queries, it is
perhaps a bit surprising that pg_stat_statements_reset() is parallel safe.
But since it only modifies shared memory, not the database, it's OK.

Andreas Karlsson

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/496899ccc2fd1b84bd1a8c8b3a7f0c667e5329f0

Modified Files
--------------
contrib/pg_stat_statements/Makefile | 6 +--
.../pg_stat_statements--1.3--1.4.sql | 7 ++++
.../pg_stat_statements/pg_stat_statements--1.3.sql | 48 ----------------------
.../pg_stat_statements/pg_stat_statements--1.4.sql | 48 ++++++++++++++++++++++
.../pg_stat_statements/pg_stat_statements.control | 2 +-
5 files changed, 59 insertions(+), 52 deletions(-)

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#2Vik Fearing
vik@2ndquadrant.fr
In reply to: Robert Haas (#1)
1 attachment(s)
Re: pgsql: Update pg_stat_statements extension for parallel query.

On 10/06/16 17:01, Robert Haas wrote:

Update pg_stat_statements extension for parallel query.

I couldn't readily find a review for this patch, and I am unsatisfied
with it. I think it's very strange that a 1.4 version would call a
function labeled 1.3, and when we make a 1.5 the code will look really
weird because it'll be missing a version.

Attached is my attempt to fix this. It might not be the best way to do
it, but I feel that *something* should be done.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Attachments:

pss14.patchtext/x-patch; name=pss14.patchDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.3--1.4.sql b/contrib/pg_stat_statements/pg_stat_statements--1.3--1.4.sql
index ae70c1f..fd54aa8 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.3--1.4.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.3--1.4.sql
@@ -3,5 +3,52 @@
 -- complain if script is sourced in psql, rather than via ALTER EXTENSION
 \echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.4'" to load this file. \quit
 
+/* First we have to remove them from the extension */
+ALTER EXTENSION pg_stat_statements DROP VIEW pg_stat_statements;
+ALTER EXTENSION pg_stat_statements DROP FUNCTION pg_stat_statements(boolean);
+
+/* Then we can drop them */
+DROP VIEW pg_stat_statements;
+DROP FUNCTION pg_stat_statements(boolean);
+
+/* Now redefine */
+CREATE FUNCTION pg_stat_statements(IN showtext boolean,
+    OUT userid oid,
+    OUT dbid oid,
+    OUT queryid bigint,
+    OUT query text,
+    OUT calls int8,
+    OUT total_time float8,
+    OUT min_time float8,
+    OUT max_time float8,
+    OUT mean_time float8,
+    OUT stddev_time float8,
+    OUT rows int8,
+    OUT shared_blks_hit int8,
+    OUT shared_blks_read int8,
+    OUT shared_blks_dirtied int8,
+    OUT shared_blks_written int8,
+    OUT local_blks_hit int8,
+    OUT local_blks_read int8,
+    OUT local_blks_dirtied int8,
+    OUT local_blks_written int8,
+    OUT temp_blks_read int8,
+    OUT temp_blks_written int8,
+    OUT blk_read_time float8,
+    OUT blk_write_time float8
+)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'pg_stat_statements_1_4'
+LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
+
+CREATE VIEW pg_stat_statements AS
+  SELECT * FROM pg_stat_statements(true);
+
+GRANT SELECT ON pg_stat_statements TO PUBLIC;
+
+/*
+ * Given the general prohibition against write operations in parallel queries,
+ * it is perhaps a bit surprising that pg_stat_statements_reset() is parallel
+ * safe.  But since it only modifies shared memory, not the database, it's OK.
+ */
 ALTER FUNCTION pg_stat_statements_reset() PARALLEL SAFE;
-ALTER FUNCTION pg_stat_statements(boolean) PARALLEL SAFE;
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.4.sql b/contrib/pg_stat_statements/pg_stat_statements--1.4.sql
index 58cdf60..86bfb6b 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.4.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.4.sql
@@ -35,7 +35,7 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
     OUT blk_write_time float8
 )
 RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'pg_stat_statements_1_3'
+AS 'MODULE_PATHNAME', 'pg_stat_statements_1_4'
 LANGUAGE C STRICT VOLATILE PARALLEL SAFE;
 
 -- Register a view on the function for ease of use.
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 3d9b8e4..552ffb8 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -117,7 +117,8 @@ typedef enum pgssVersion
 	PGSS_V1_0 = 0,
 	PGSS_V1_1,
 	PGSS_V1_2,
-	PGSS_V1_3
+	PGSS_V1_3,
+	PGSS_V1_4
 } pgssVersion;
 
 /*
@@ -281,6 +282,7 @@ void		_PG_fini(void);
 PG_FUNCTION_INFO_V1(pg_stat_statements_reset);
 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_4);
 PG_FUNCTION_INFO_V1(pg_stat_statements);
 
 static void pgss_shmem_startup(void);
@@ -1282,6 +1284,7 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
 #define PG_STAT_STATEMENTS_COLS_V1_1	18
 #define PG_STAT_STATEMENTS_COLS_V1_2	19
 #define PG_STAT_STATEMENTS_COLS_V1_3	23
+#define PG_STAT_STATEMENTS_COLS_V1_4	23		/* only parallel safety was changed */
 #define PG_STAT_STATEMENTS_COLS			23		/* maximum of above */
 
 /*
@@ -1295,6 +1298,16 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
  * function.  Unfortunately we weren't bright enough to do that for 1.1.
  */
 Datum
+pg_stat_statements_1_4(PG_FUNCTION_ARGS)
+{
+	bool		showtext = PG_GETARG_BOOL(0);
+
+	pg_stat_statements_internal(fcinfo, PGSS_V1_4, showtext);
+
+	return (Datum) 0;
+}
+
+Datum
 pg_stat_statements_1_3(PG_FUNCTION_ARGS)
 {
 	bool		showtext = PG_GETARG_BOOL(0);
@@ -1393,8 +1406,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 			if (api_version != PGSS_V1_2)
 				elog(ERROR, "incorrect number of output arguments");
 			break;
-		case PG_STAT_STATEMENTS_COLS_V1_3:
-			if (api_version != PGSS_V1_3)
+		case PG_STAT_STATEMENTS_COLS_V1_3 || PG_STAT_STATEMENTS_COLS_V1_4:
+			/* version 1.4 only changed parallel safety */
+			if ((api_version != PGSS_V1_3) || (api_version != PGSS_V1_4))
 				elog(ERROR, "incorrect number of output arguments");
 			break;
 		default:
@@ -1598,6 +1612,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 					 api_version == PGSS_V1_1 ? PG_STAT_STATEMENTS_COLS_V1_1 :
 					 api_version == PGSS_V1_2 ? PG_STAT_STATEMENTS_COLS_V1_2 :
 					 api_version == PGSS_V1_3 ? PG_STAT_STATEMENTS_COLS_V1_3 :
+					 api_version == PGSS_V1_4 ? PG_STAT_STATEMENTS_COLS_V1_4 :
 					 -1 /* fail if you forget to update this assert */ ));
 
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
#3Robert Haas
robertmhaas@gmail.com
In reply to: Vik Fearing (#2)
Re: [COMMITTERS] pgsql: Update pg_stat_statements extension for parallel query.

On Thu, Jun 16, 2016 at 4:51 PM, Vik Fearing <vik@2ndquadrant.fr> wrote:

On 10/06/16 17:01, Robert Haas wrote:

Update pg_stat_statements extension for parallel query.

I couldn't readily find a review for this patch, and I am unsatisfied
with it. I think it's very strange that a 1.4 version would call a
function labeled 1.3, and when we make a 1.5 the code will look really
weird because it'll be missing a version.

Attached is my attempt to fix this. It might not be the best way to do
it, but I feel that *something* should be done.

Hmm. I don't think this is solving any real problem, is it? You're
just adding backward compatibility code to the C files that doesn't
really need to be there. I don't think it's particularly confusing
that the extension version might sometimes get bumped without changing
the SRF columns.

Another problem with this change is that dropping and redefining the
view will prevent anyone who has a dependency on the view from being
able to update to the latest extension. It doesn't seem like a wise
idea to force that on users unnecessarily.

(I am sorry you are unsatisfied, though. I didn't feel a need to post
a detailed review of each of these many patches on the relevant
thread, because they are mostly pretty boilerplate.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers