Hi Fujii-san,
Thank you for your reply and comment!
attach v2 fixed patch.
meson.build needs to be updated as well, like the Makefile.
Yes.
Update 'contrib/pg_stat_statements/meson.build'.
For the privileges test, should we explicitly set
pg_stat_statements.track_utility
at the start, as done in other pg_stat_statements tests, to make sure
if utility command statistics are collected or not?
It certainly needs consideration.
I think the results of the utility commands are not needed in privileges
test.
SET 'pg_stat_statements.track_utility = FALSE'.
Can't we simplify "CASE ... END" to just "queryid <> 0"?
Yes.
If we add "queryid <> 0" to the WHERE clause, we can get the same
result.
Change the SQL to the following:
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid <> 0 ORDER BY query COLLATE "C";
Should the test check not only queryid and query but also
the statistics column like "calls"? Roles that aren't superusers
or pg_read_all_stats should be able to see statistics but not
query or queryid. So we should test that such roles can't see
query or queryid but can see statistics. Thoughts?
I agree. We should test that such roles can't see
query or queryid but can see statistics.
Add the SQL to the test.
Test that calls and rows are displayed even if the queryid is NULL.
+-- regress_stats_user1 can read calls and rows
+-- executed by other users
+--
+
+SET ROLE regress_stats_user1;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid IS NULL ORDER BY query COLLATE "C";
Best Regards,
Keisuke Kuroda
NTT Comware
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 414a30856e..c19ccad77e 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -19,7 +19,8 @@ LDFLAGS_SL += $(filter -lm, $(LIBS))
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
REGRESS = select dml cursors utility level_tracking planning \
- user_activity wal entry_timestamp cleanup oldextversions
+ user_activity wal entry_timestamp privileges cleanup \
+ oldextversions
# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
# which typical installcheck users do not have (e.g. buildfarm clients).
NO_INSTALLCHECK = 1
diff --git a/contrib/pg_stat_statements/expected/privileges.out b/contrib/pg_stat_statements/expected/privileges.out
new file mode 100644
index 0000000000..20f5575e83
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/privileges.out
@@ -0,0 +1,95 @@
+--
+-- Superusers or roles with the privileges of pg_read_all_stats members
+-- can read query text and queryid
+--
+SET pg_stat_statements.track_utility = FALSE;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+CREATE ROLE regress_stats_user1;
+CREATE ROLE regress_stats_user2;
+GRANT pg_read_all_stats TO regress_stats_user2;
+SELECT 1 AS "ONE";
+ ONE
+-----
+ 1
+(1 row)
+
+SET ROLE regress_stats_user1;
+SELECT 1+1 AS "TWO";
+ TWO
+-----
+ 2
+(1 row)
+
+-- Superuser can read query text and queryid
+RESET ROLE;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid <> 0 ORDER BY query COLLATE "C";
+ query | calls | rows
+----------------------------------------------------+-------+------
+ SELECT $1 AS "ONE" | 1 | 1
+ SELECT $1+$2 AS "TWO" | 1 | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1 | 1
+(3 rows)
+
+--
+-- regress_stats_user1 can not read query text and queryid
+-- executed by other users
+--
+SET ROLE regress_stats_user1;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid <> 0 ORDER BY query COLLATE "C";
+ query | calls | rows
+-----------------------+-------+------
+ SELECT $1+$2 AS "TWO" | 1 | 1
+(1 row)
+
+RESET ROLE;
+--
+-- regress_stats_user1 can read calls and rows
+-- executed by other users
+--
+SET ROLE regress_stats_user1;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid IS NULL ORDER BY query COLLATE "C";
+ query | calls | rows
+--------------------------+-------+------
+ <insufficient privilege> | 1 | 1
+ <insufficient privilege> | 1 | 1
+ <insufficient privilege> | 1 | 3
+(3 rows)
+
+RESET ROLE;
+-- regress_stats_user2 can read query text and queryid
+SET ROLE regress_stats_user2;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid <> 0 ORDER BY query COLLATE "C";
+ query | calls | rows
+----------------------------------------------------+-------+------
+ SELECT $1 AS "ONE" | 1 | 1
+ SELECT $1+$2 AS "TWO" | 1 | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1 | 1
+ SELECT query, calls, rows FROM pg_stat_statements +| 1 | 1
+ WHERE queryid <> $1 ORDER BY query COLLATE "C" | |
+ SELECT query, calls, rows FROM pg_stat_statements +| 1 | 3
+ WHERE queryid <> $1 ORDER BY query COLLATE "C" | |
+ SELECT query, calls, rows FROM pg_stat_statements +| 1 | 3
+ WHERE queryid IS NULL ORDER BY query COLLATE "C" | |
+(6 rows)
+
+--
+-- cleanup
+--
+RESET ROLE;
+DROP ROLE regress_stats_user1;
+DROP ROLE regress_stats_user2;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index 9bfc9657e1..5cf926d1f8 100644
--- a/contrib/pg_stat_statements/meson.build
+++ b/contrib/pg_stat_statements/meson.build
@@ -50,6 +50,7 @@ tests += {
'user_activity',
'wal',
'entry_timestamp',
+ 'privileges',
'cleanup',
'oldextversions',
],
diff --git a/contrib/pg_stat_statements/sql/privileges.sql b/contrib/pg_stat_statements/sql/privileges.sql
new file mode 100644
index 0000000000..5dc868ed46
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/privileges.sql
@@ -0,0 +1,55 @@
+--
+-- Superusers or roles with the privileges of pg_read_all_stats members
+-- can read query text and queryid
+--
+
+SET pg_stat_statements.track_utility = FALSE;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+CREATE ROLE regress_stats_user1;
+CREATE ROLE regress_stats_user2;
+GRANT pg_read_all_stats TO regress_stats_user2;
+SELECT 1 AS "ONE";
+
+SET ROLE regress_stats_user1;
+SELECT 1+1 AS "TWO";
+
+-- Superuser can read query text and queryid
+
+RESET ROLE;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid <> 0 ORDER BY query COLLATE "C";
+
+--
+-- regress_stats_user1 can not read query text and queryid
+-- executed by other users
+--
+
+SET ROLE regress_stats_user1;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid <> 0 ORDER BY query COLLATE "C";
+RESET ROLE;
+
+--
+-- regress_stats_user1 can read calls and rows
+-- executed by other users
+--
+
+SET ROLE regress_stats_user1;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid IS NULL ORDER BY query COLLATE "C";
+RESET ROLE;
+
+-- regress_stats_user2 can read query text and queryid
+
+SET ROLE regress_stats_user2;
+SELECT query, calls, rows FROM pg_stat_statements
+ WHERE queryid <> 0 ORDER BY query COLLATE "C";
+
+--
+-- cleanup
+--
+
+RESET ROLE;
+DROP ROLE regress_stats_user1;
+DROP ROLE regress_stats_user2;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
