Add privileges test for pg_stat_statements to improve coverage

Started by Nonameover 1 year ago12 messages

Noname

kuroda.keisuke@nttcom.co.jp

over 1 year ago

1 attachment(s)

Hi hackers,

I proposal adding privileges test to improve
test coverage of pg_stat_statements.

## test procedure

./configure --enable-coverage --enable-tap-tests --with-llvm CFLAGS=-O0
make check-world
make coverage-html

## coverage

before Line Coverage 74.0 %(702/949 lines)
after Line Coverage 74.4 %(706/949 lines)

Although the improvement is small, I think that test regarding
privileges is necessary.

As a side note,
Initially, I was considering adding a test for dealloc.
However, after reading the thread below, I confirmed that
it is difficult to create tests due to differences due to endian.
(/messages/by-id/40d1e4f2-835f-448f-a541-8ff5db75bf3d@eisentraut.org)
For this reason, I first added a test about privileges.

Best Regards,

Keisuke Kuroda
NTT Comware

Attachments:

add_privileges_test_for_pg_stat_statements.patchtext/x-diff; name=add_privileges_test_for_pg_stat_statements.patchDownload

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..383895c87d
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/privileges.out
@@ -0,0 +1,102 @@
+--
+-- Superusers or roles with the privileges of pg_read_all_stats members
+-- can read query text and queryid
+--
+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;
+SET ROLE regress_stats_user1;
+SELECT 1 AS "ONE";
+ ONE 
+-----
+   1
+(1 row)
+
+-- Superuser can read query text and queryid
+RESET ROLE;
+SELECT
+  CASE
+    WHEN queryid <> 0 THEN TRUE ELSE FALSE
+  END AS queryid_bool
+  ,query FROM pg_stat_statements ORDER BY query COLLATE "C";
+ queryid_bool |                       query                        
+--------------+----------------------------------------------------
+ t            | CREATE ROLE regress_stats_user1
+ t            | CREATE ROLE regress_stats_user2
+ t            | GRANT pg_read_all_stats TO regress_stats_user2
+ t            | RESET ROLE
+ t            | SELECT $1 AS "ONE"
+ t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t
+ t            | SET ROLE regress_stats_user1
+(7 rows)
+
+--
+-- regress_stats_user1 can not read query text and queryid
+-- executed by other users
+--
+SET ROLE regress_stats_user1;
+SELECT
+  CASE
+    WHEN queryid <> 0 THEN TRUE ELSE FALSE
+  END AS queryid_bool
+  ,query FROM pg_stat_statements ORDER BY query COLLATE "C";
+ queryid_bool |            query             
+--------------+------------------------------
+ f            | <insufficient privilege>
+ f            | <insufficient privilege>
+ f            | <insufficient privilege>
+ f            | <insufficient privilege>
+ f            | <insufficient privilege>
+ f            | <insufficient privilege>
+ t            | SELECT $1 AS "ONE"
+ t            | SET ROLE regress_stats_user1
+(8 rows)
+
+RESET ROLE;
+-- regress_stats_user2 can read query text and queryid
+SET ROLE regress_stats_user2;
+SELECT
+  CASE
+    WHEN queryid <> 0 THEN TRUE ELSE FALSE
+  END AS queryid_bool
+  ,query FROM pg_stat_statements ORDER BY query COLLATE "C";
+ queryid_bool |                            query                            
+--------------+-------------------------------------------------------------
+ t            | CREATE ROLE regress_stats_user1
+ t            | CREATE ROLE regress_stats_user2
+ t            | GRANT pg_read_all_stats TO regress_stats_user2
+ t            | RESET ROLE
+ t            | SELECT                                                     +
+              |   CASE                                                     +
+              |     WHEN queryid <> $1 THEN $2 ELSE $3                     +
+              |   END AS queryid_bool                                      +
+              |   ,query FROM pg_stat_statements ORDER BY query COLLATE "C"
+ t            | SELECT                                                     +
+              |   CASE                                                     +
+              |     WHEN queryid <> $1 THEN $2 ELSE $3                     +
+              |   END AS queryid_bool                                      +
+              |   ,query FROM pg_stat_statements ORDER BY query COLLATE "C"
+ t            | SELECT $1 AS "ONE"
+ t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t
+ t            | SET ROLE regress_stats_user1
+ t            | SET ROLE regress_stats_user2
+(10 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/sql/privileges.sql b/contrib/pg_stat_statements/sql/privileges.sql
new file mode 100644
index 0000000000..69f4abf234
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/privileges.sql
@@ -0,0 +1,52 @@
+--
+-- Superusers or roles with the privileges of pg_read_all_stats members
+-- can read query text and queryid
+--
+
+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;
+
+SET ROLE regress_stats_user1;
+SELECT 1 AS "ONE";
+
+-- Superuser can read query text and queryid
+
+RESET ROLE;
+SELECT
+  CASE
+    WHEN queryid <> 0 THEN TRUE ELSE FALSE
+  END AS queryid_bool
+  ,query FROM pg_stat_statements 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
+  CASE
+    WHEN queryid <> 0 THEN TRUE ELSE FALSE
+  END AS queryid_bool
+  ,query FROM pg_stat_statements ORDER BY query COLLATE "C";
+RESET ROLE;
+
+-- regress_stats_user2 can read query text and queryid
+
+SET ROLE regress_stats_user2;
+SELECT
+  CASE
+    WHEN queryid <> 0 THEN TRUE ELSE FALSE
+  END AS queryid_bool
+  ,query FROM pg_stat_statements 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;

Fujii Masao

masao.fujii@oss.nttdata.com

over 1 year ago

In reply to: Noname (#1)

Re: Add privileges test for pg_stat_statements to improve coverage

On 2024/04/23 15:44, kuroda.keisuke@nttcom.co.jp wrote:

Hi hackers,

I proposal adding privileges test to improve
test coverage of pg_stat_statements.

Here are the review comments:

meson.build needs to be updated as well, like the Makefile.

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?

+SELECT
+  CASE
+    WHEN queryid <> 0 THEN TRUE ELSE FALSE
+  END AS queryid_bool
+  ,query FROM pg_stat_statements ORDER BY query COLLATE "C";

Can't we simplify "CASE ... END" to just "queryid <> 0"?

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?

Regards,

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

Noname

kuroda.keisuke@nttcom.co.jp

over 1 year ago

In reply to: Fujii Masao (#2)

1 attachment(s)

Re: Add privileges test for pg_stat_statements to improve coverage

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

Attachments:

V2_add_privileges_test_for_pg_stat_statements.patchtext/x-diff; name=V2_add_privileges_test_for_pg_stat_statements.patchDownload

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;

Fujii Masao

masao.fujii@oss.nttdata.com

over 1 year ago

In reply to: Noname (#3)

Re: Add privileges test for pg_stat_statements to improve coverage

On 2024/07/22 15:23, kuroda.keisuke@nttcom.co.jp wrote:

Hi Fujii-san,
Thank you for your reply and comment!

attach v2 fixed patch.

Thanks for updating the patch!

+SELECT query, calls, rows FROM pg_stat_statements
+  WHERE queryid IS NULL ORDER BY query COLLATE "C";

Shouldn't we also include calls and rows in the ORDER BY clause?
Without this, if there are multiple records with the same query
but different calls or rows, the query result might be unstable.
I believe this is causing the test failure reported by
he PostgreSQL Patch Tester.

http://cfbot.cputube.org/
https://cirrus-ci.com/task/4533613939654656

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";

I was thinking of adding "queryid <> 0" in the SELECT clause
instead of the WHERE clause. This way, we can verify if
the query results are as expected regardless of the queryid value,
including both queryid <> 0 and queryid = 0.

Regards,

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

Michael Paquier

michael@paquier.xyz

over 1 year ago

In reply to: Fujii Masao (#4)

Re: Add privileges test for pg_stat_statements to improve coverage

On Tue, Jul 23, 2024 at 01:51:19AM +0900, Fujii Masao wrote:

+SELECT query, calls, rows FROM pg_stat_statements
+  WHERE queryid IS NULL ORDER BY query COLLATE "C";
Shouldn't we also include calls and rows in the ORDER BY clause?
Without this, if there are multiple records with the same query
but different calls or rows, the query result might be unstable.
I believe this is causing the test failure reported by
he PostgreSQL Patch Tester.

http://cfbot.cputube.org/
https://cirrus-ci.com/task/4533613939654656

+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)

I'd recommend to add a GROUP BY on calls and rows, with a
count(query), rather than print the same row without the query text
multiple times.

+-- 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"   |       |

We have two entries here with the same query and the same query ID,
because they have a different userid. Shouldn't this query reflect
this information rather than have the reader guess it? This is going
to require a join with pg_authid to grab the role name, and an ORDER
BY on the role name.
--
Michael

Noname

kuroda.keisuke@nttcom.co.jp

over 1 year ago

In reply to: Fujii Masao (#4)

1 attachment(s)

Re: Add privileges test for pg_stat_statements to improve coverage

Hi Fujii-san,
Thank you for your reply and comment!

attach v3 fixed patch.

Shouldn't we also include calls and rows in the ORDER BY clause?
Without this, if there are multiple records with the same query
but different calls or rows, the query result might be unstable.
I believe this is causing the test failure reported by
he PostgreSQL Patch Tester.

I was thinking of adding "queryid <> 0" in the SELECT clause
instead of the WHERE clause. This way, we can verify if
the query results are as expected regardless of the queryid value,
including both queryid <> 0 and queryid = 0.

It's exactly as you said.
* Add calls and rows in the ORDER BY caluse.
* Modify "queryid <> 0" in the SELECT clause.
Modify test SQL belows, and the regress_stats_user1 check SQL
only needs to be done once.

+SELECT queryid <> 0 AS queryid_bool, query, calls, rows
+  FROM pg_stat_statements
+  ORDER BY query COLLATE "C", calls, rows;

Best Regards,
Keisuke Kuroda
NTT Comware

Attachments:

V3_add_privileges_test_for_pg_stat_statements.patchtext/x-diff; name=V3_add_privileges_test_for_pg_stat_statements.patchDownload

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..55687f0256
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/privileges.out
@@ -0,0 +1,87 @@
+--
+-- 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 queryid <> 0 AS queryid_bool, query, calls, rows
+  FROM pg_stat_statements
+  ORDER BY query COLLATE "C", calls, rows;
+ queryid_bool |                       query                        | calls | rows 
+--------------+----------------------------------------------------+-------+------
+ t            | SELECT $1 AS "ONE"                                 |     1 |    1
+ t            | SELECT $1+$2 AS "TWO"                              |     1 |    1
+ t            | 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,
+-- but can read calls and rows
+--
+SET ROLE regress_stats_user1;
+SELECT queryid <> 0 AS queryid_bool, query, calls, rows
+  FROM pg_stat_statements
+  ORDER BY query COLLATE "C", calls, rows;
+ queryid_bool |          query           | calls | rows 
+--------------+--------------------------+-------+------
+              | <insufficient privilege> |     1 |    1
+              | <insufficient privilege> |     1 |    1
+              | <insufficient privilege> |     1 |    3
+ t            | SELECT $1+$2 AS "TWO"    |     1 |    1
+(4 rows)
+
+RESET ROLE;
+-- regress_stats_user2 can read query text and queryid
+SET ROLE regress_stats_user2;
+SELECT queryid <> 0 AS queryid_bool, query, calls, rows
+  FROM pg_stat_statements
+  ORDER BY query COLLATE "C", calls, rows;
+ queryid_bool |                          query                           | calls | rows 
+--------------+----------------------------------------------------------+-------+------
+ t            | SELECT $1 AS "ONE"                                       |     1 |    1
+ t            | SELECT $1+$2 AS "TWO"                                    |     1 |    1
+ t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t       |     1 |    1
+ t            | SELECT queryid <> $1 AS queryid_bool, query, calls, rows+|     1 |    3
+              |   FROM pg_stat_statements                               +|       | 
+              |   ORDER BY query COLLATE "C", calls, rows                |       | 
+ t            | SELECT queryid <> $1 AS queryid_bool, query, calls, rows+|     1 |    4
+              |   FROM pg_stat_statements                               +|       | 
+              |   ORDER BY query COLLATE "C", calls, rows                |       | 
+(5 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..74555530e9
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/privileges.sql
@@ -0,0 +1,49 @@
+--
+-- 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 queryid <> 0 AS queryid_bool, query, calls, rows
+  FROM pg_stat_statements
+  ORDER BY query COLLATE "C", calls, rows;
+
+--
+-- regress_stats_user1 can not read query text and queryid
+-- executed by other users,
+-- but can read calls and rows
+--
+
+SET ROLE regress_stats_user1;
+SELECT queryid <> 0 AS queryid_bool, query, calls, rows
+  FROM pg_stat_statements
+  ORDER BY query COLLATE "C", calls, rows;
+RESET ROLE;
+
+-- regress_stats_user2 can read query text and queryid
+
+SET ROLE regress_stats_user2;
+SELECT queryid <> 0 AS queryid_bool, query, calls, rows
+  FROM pg_stat_statements
+  ORDER BY query COLLATE "C", calls, rows;
+
+--
+-- cleanup
+--
+
+RESET ROLE;
+DROP ROLE regress_stats_user1;
+DROP ROLE regress_stats_user2;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;

Noname

kuroda.keisuke@nttcom.co.jp

over 1 year ago

In reply to: Michael Paquier (#5)

1 attachment(s)

Re: Add privileges test for pg_stat_statements to improve coverage

Hi Michael-san,
Thank you for your reply and comment!

attach v4 fixed patch.

We have two entries here with the same query and the same query ID,
because they have a different userid. Shouldn't this query reflect
this information rather than have the reader guess it? This is going
to require a join with pg_authid to grab the role name, and an ORDER
BY on the role name.

I agree.
The information of different userids is mixed up.
It is easier to understand if the role name is displayed.
Join with pg_roles (view of pg_authid) to output the role name.

I'd recommend to add a GROUP BY on calls and rows, with a
count(query), rather than print the same row without the query text
multiple times.

Indeed, same row have been output multiple times.
If we use GROUP BY, we would expect the following.

```
SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, count(ss.query),
ss.calls, ss.rows
FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
GROUP BY r.rolname, queryid_bool, ss.calls, ss.rows
ORDER BY r.rolname, count(ss.query), ss.calls, ss.rows;
rolname | queryid_bool | count | calls | rows
---------------------+--------------+-------+-------+------
postgres | | 1 | 1 | 3
postgres | | 2 | 1 | 1
regress_stats_user1 | t | 1 | 1 | 1
(3 rows)
```

However, in this test I would like to see '<insufficient permissions>'
output
and the SQL text 'SELECT $1+$2 AS “TWO”' executed by
regress_stats_user1.

The attached patch executes the following SQL.
What do you think?

Attachments:

V4_add_privileges_test_for_pg_stat_statements.patchtext/x-diff; name=V4_add_privileges_test_for_pg_stat_statements.patchDownload

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..4c7794ce0c
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/privileges.out
@@ -0,0 +1,86 @@
+--
+-- 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 r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+       rolname       | queryid_bool |                       query                        | calls | rows 
+---------------------+--------------+----------------------------------------------------+-------+------
+ postgres            | t            | SELECT $1 AS "ONE"                                 |     1 |    1
+ postgres            | t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t |     1 |    1
+ regress_stats_user1 | t            | SELECT $1+$2 AS "TWO"                              |     1 |    1
+(3 rows)
+
+--
+-- regress_stats_user1 can not read query text and queryid
+-- executed by other users,
+-- but can read calls and rows
+--
+SET ROLE regress_stats_user1;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+       rolname       | queryid_bool |          query           | calls | rows 
+---------------------+--------------+--------------------------+-------+------
+ postgres            |              | <insufficient privilege> |     1 |    1
+ postgres            |              | <insufficient privilege> |     1 |    1
+ postgres            |              | <insufficient privilege> |     1 |    3
+ regress_stats_user1 | t            | SELECT $1+$2 AS "TWO"    |     1 |    1
+(4 rows)
+
+-- regress_stats_user2 can read query text and queryid
+SET ROLE regress_stats_user2;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+       rolname       | queryid_bool |                                      query                                      | calls | rows 
+---------------------+--------------+---------------------------------------------------------------------------------+-------+------
+ postgres            | t            | SELECT $1 AS "ONE"                                                              |     1 |    1
+ postgres            | t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t                              |     1 |    1
+ postgres            | t            | SELECT r.rolname, ss.queryid <> $1 AS queryid_bool, ss.query, ss.calls, ss.rows+|     1 |    3
+                     |              |   FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid              +|       | 
+                     |              |   ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows                   |       | 
+ regress_stats_user1 | t            | SELECT $1+$2 AS "TWO"                                                           |     1 |    1
+ regress_stats_user1 | t            | SELECT r.rolname, ss.queryid <> $1 AS queryid_bool, ss.query, ss.calls, ss.rows+|     1 |    4
+                     |              |   FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid              +|       | 
+                     |              |   ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows                   |       | 
+(5 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..8ed99a32ee
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/privileges.sql
@@ -0,0 +1,48 @@
+--
+-- 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 r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+--
+-- regress_stats_user1 can not read query text and queryid
+-- executed by other users,
+-- but can read calls and rows
+--
+
+SET ROLE regress_stats_user1;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+-- regress_stats_user2 can read query text and queryid
+
+SET ROLE regress_stats_user2;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+--
+-- cleanup
+--
+
+RESET ROLE;
+DROP ROLE regress_stats_user1;
+DROP ROLE regress_stats_user2;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;

Fujii Masao

masao.fujii@oss.nttdata.com

over 1 year ago

In reply to: Noname (#7)

Re: Add privileges test for pg_stat_statements to improve coverage

On 2024/07/23 10:40, kuroda.keisuke@nttcom.co.jp wrote:

I agree.
The information of different userids is mixed up.
It is easier to understand if the role name is displayed.
Join with pg_roles (view of pg_authid) to output the role name.

+       rolname       | queryid_bool |                       query                        | calls | rows
+---------------------+--------------+----------------------------------------------------+-------+------
+ postgres            | t            | SELECT $1 AS "ONE"                                 |     1 |    1
+ postgres            | t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t |     1 |    1
+ regress_stats_user1 | t            | SELECT $1+$2 AS "TWO"                              |     1 |    1

Using "postgres" as the default superuser name can cause instability.
This is why the Patch Tester reports now test failures again.
You should create and use a different superuser, such as "regress_stats_superuser."

Regards,

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

Noname

kuroda.keisuke@nttcom.co.jp

over 1 year ago

In reply to: Fujii Masao (#8)

1 attachment(s)

Re: Add privileges test for pg_stat_statements to improve coverage

Hi Fujii-san,
Thank you for your comment!

attach v5 fixed patch.

Using "postgres" as the default superuser name can cause instability.
This is why the Patch Tester reports now test failures again.
You should create and use a different superuser, such as
"regress_stats_superuser."

I understand.
Add "regress_stats_superuser" for test by superuser.

Best Regards,
Keisuke Kuroda
NTT Comware

Attachments:

V5_add_privileges_test_for_pg_stat_statements.patchtext/x-diff; name=V5_add_privileges_test_for_pg_stat_statements.patchDownload

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..06b38db4f4
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/privileges.out
@@ -0,0 +1,89 @@
+--
+-- 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;
+CREATE ROLE regress_stats_superuser SUPERUSER;
+CREATE ROLE regress_stats_user1;
+CREATE ROLE regress_stats_user2;
+GRANT pg_read_all_stats TO regress_stats_user2;
+SET ROLE regress_stats_superuser;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
+(1 row)
+
+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
+SET ROLE regress_stats_superuser;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+         rolname         | queryid_bool |                       query                        | calls | rows 
+-------------------------+--------------+----------------------------------------------------+-------+------
+ regress_stats_superuser | t            | SELECT $1 AS "ONE"                                 |     1 |    1
+ regress_stats_superuser | t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t |     1 |    1
+ regress_stats_user1     | t            | SELECT $1+$2 AS "TWO"                              |     1 |    1
+(3 rows)
+
+--
+-- regress_stats_user1 can not read query text and queryid
+-- executed by other users,
+-- but can read calls and rows
+--
+SET ROLE regress_stats_user1;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+         rolname         | queryid_bool |          query           | calls | rows 
+-------------------------+--------------+--------------------------+-------+------
+ regress_stats_superuser |              | <insufficient privilege> |     1 |    1
+ regress_stats_superuser |              | <insufficient privilege> |     1 |    1
+ regress_stats_superuser |              | <insufficient privilege> |     1 |    3
+ regress_stats_user1     | t            | SELECT $1+$2 AS "TWO"    |     1 |    1
+(4 rows)
+
+-- regress_stats_user2 can read query text and queryid
+SET ROLE regress_stats_user2;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+         rolname         | queryid_bool |                                      query                                      | calls | rows 
+-------------------------+--------------+---------------------------------------------------------------------------------+-------+------
+ regress_stats_superuser | t            | SELECT $1 AS "ONE"                                                              |     1 |    1
+ regress_stats_superuser | t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t                              |     1 |    1
+ regress_stats_superuser | t            | SELECT r.rolname, ss.queryid <> $1 AS queryid_bool, ss.query, ss.calls, ss.rows+|     1 |    3
+                         |              |   FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid              +|       | 
+                         |              |   ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows                   |       | 
+ regress_stats_user1     | t            | SELECT $1+$2 AS "TWO"                                                           |     1 |    1
+ regress_stats_user1     | t            | SELECT r.rolname, ss.queryid <> $1 AS queryid_bool, ss.query, ss.calls, ss.rows+|     1 |    4
+                         |              |   FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid              +|       | 
+                         |              |   ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows                   |       | 
+(5 rows)
+
+--
+-- cleanup
+--
+RESET ROLE;
+DROP ROLE regress_stats_superuser;
+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..b54686e6c4
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/privileges.sql
@@ -0,0 +1,52 @@
+--
+-- 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;
+CREATE ROLE regress_stats_superuser SUPERUSER;
+CREATE ROLE regress_stats_user1;
+CREATE ROLE regress_stats_user2;
+GRANT pg_read_all_stats TO regress_stats_user2;
+
+SET ROLE regress_stats_superuser;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT 1 AS "ONE";
+
+SET ROLE regress_stats_user1;
+SELECT 1+1 AS "TWO";
+
+-- Superuser can read query text and queryid
+
+SET ROLE regress_stats_superuser;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+--
+-- regress_stats_user1 can not read query text and queryid
+-- executed by other users,
+-- but can read calls and rows
+--
+
+SET ROLE regress_stats_user1;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+-- regress_stats_user2 can read query text and queryid
+
+SET ROLE regress_stats_user2;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+--
+-- cleanup
+--
+
+RESET ROLE;
+DROP ROLE regress_stats_superuser;
+DROP ROLE regress_stats_user1;
+DROP ROLE regress_stats_user2;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;

#10

Fujii Masao

masao.fujii@oss.nttdata.com

over 1 year ago

In reply to: Noname (#9)

1 attachment(s)

Re: Add privileges test for pg_stat_statements to improve coverage

On 2024/07/23 15:02, kuroda.keisuke@nttcom.co.jp wrote:

Hi Fujii-san,
Thank you for your comment!

attach v5 fixed patch.

Using "postgres" as the default superuser name can cause instability.
This is why the Patch Tester reports now test failures again.
You should create and use a different superuser, such as
"regress_stats_superuser."

I understand.
Add "regress_stats_superuser" for test by superuser.

Thanks for updating the patch!

I've slightly modified the comments in the regression tests for clarity.
Attached is the v6 patch. If there are no objections,
I will push this version.

Regards,

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

Attachments:

v6-0001-pg_stat_statements-Add-regression-test-for-privil.patchtext/plain; charset=UTF-8; name=v6-0001-pg_stat_statements-Add-regression-test-for-privil.patchDownload

From d2dfc611750bce0b06ba3a3f82b136014d399987 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 24 Jul 2024 02:50:35 +0900
Subject: [PATCH v6] pg_stat_statements: Add regression test for privilege
 handling.

This commit adds a regression test to verify that pg_stat_statements
correctly handles privileges, improving its test coverage.

Author: Keisuke Kuroda
Reviewed-by: Michael Paquier, Fujii Masao
Discussion: https://postgr.es/m/2224ccf2e12c41ccb81702ef3303d5ac@nttcom.co.jp
---
 contrib/pg_stat_statements/Makefile           |  3 +-
 .../expected/privileges.out                   | 97 +++++++++++++++++++
 contrib/pg_stat_statements/meson.build        |  1 +
 contrib/pg_stat_statements/sql/privileges.sql | 60 ++++++++++++
 4 files changed, 160 insertions(+), 1 deletion(-)
 create mode 100644 contrib/pg_stat_statements/expected/privileges.out
 create mode 100644 contrib/pg_stat_statements/sql/privileges.sql

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..daaaa0bb83
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/privileges.out
@@ -0,0 +1,97 @@
+--
+-- Only superusers and roles with privileges of the pg_read_all_stats role
+-- are allowed to see the SQL text and queryid of queries executed by
+-- other users. Other users can see the statistics.
+--
+SET pg_stat_statements.track_utility = FALSE;
+CREATE ROLE regress_stats_superuser SUPERUSER;
+CREATE ROLE regress_stats_user1;
+CREATE ROLE regress_stats_user2;
+GRANT pg_read_all_stats TO regress_stats_user2;
+SET ROLE regress_stats_superuser;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
+(1 row)
+
+SELECT 1 AS "ONE";
+ ONE 
+-----
+   1
+(1 row)
+
+SET ROLE regress_stats_user1;
+SELECT 1+1 AS "TWO";
+ TWO 
+-----
+   2
+(1 row)
+
+--
+-- A superuser can read all columns of queries executed by others,
+-- including query text and queryid.
+--
+SET ROLE regress_stats_superuser;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+         rolname         | queryid_bool |                       query                        | calls | rows 
+-------------------------+--------------+----------------------------------------------------+-------+------
+ regress_stats_superuser | t            | SELECT $1 AS "ONE"                                 |     1 |    1
+ regress_stats_superuser | t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t |     1 |    1
+ regress_stats_user1     | t            | SELECT $1+$2 AS "TWO"                              |     1 |    1
+(3 rows)
+
+--
+-- regress_stats_user1 has no privileges to read the query text or
+-- queryid of queries executed by others but can see statistics
+-- like calls and rows.
+--
+SET ROLE regress_stats_user1;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+         rolname         | queryid_bool |          query           | calls | rows 
+-------------------------+--------------+--------------------------+-------+------
+ regress_stats_superuser |              | <insufficient privilege> |     1 |    1
+ regress_stats_superuser |              | <insufficient privilege> |     1 |    1
+ regress_stats_superuser |              | <insufficient privilege> |     1 |    3
+ regress_stats_user1     | t            | SELECT $1+$2 AS "TWO"    |     1 |    1
+(4 rows)
+
+--
+-- regress_stats_user2, with pg_read_all_stats role privileges, can
+-- read all columns, including query text and queryid, of queries
+-- executed by others.
+--
+SET ROLE regress_stats_user2;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+         rolname         | queryid_bool |                                      query                                      | calls | rows 
+-------------------------+--------------+---------------------------------------------------------------------------------+-------+------
+ regress_stats_superuser | t            | SELECT $1 AS "ONE"                                                              |     1 |    1
+ regress_stats_superuser | t            | SELECT pg_stat_statements_reset() IS NOT NULL AS t                              |     1 |    1
+ regress_stats_superuser | t            | SELECT r.rolname, ss.queryid <> $1 AS queryid_bool, ss.query, ss.calls, ss.rows+|     1 |    3
+                         |              |   FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid              +|       | 
+                         |              |   ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows                   |       | 
+ regress_stats_user1     | t            | SELECT $1+$2 AS "TWO"                                                           |     1 |    1
+ regress_stats_user1     | t            | SELECT r.rolname, ss.queryid <> $1 AS queryid_bool, ss.query, ss.calls, ss.rows+|     1 |    4
+                         |              |   FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid              +|       | 
+                         |              |   ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows                   |       | 
+(5 rows)
+
+--
+-- cleanup
+--
+RESET ROLE;
+DROP ROLE regress_stats_superuser;
+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..75b1489a47
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/privileges.sql
@@ -0,0 +1,60 @@
+--
+-- Only superusers and roles with privileges of the pg_read_all_stats role
+-- are allowed to see the SQL text and queryid of queries executed by
+-- other users. Other users can see the statistics.
+--
+
+SET pg_stat_statements.track_utility = FALSE;
+CREATE ROLE regress_stats_superuser SUPERUSER;
+CREATE ROLE regress_stats_user1;
+CREATE ROLE regress_stats_user2;
+GRANT pg_read_all_stats TO regress_stats_user2;
+
+SET ROLE regress_stats_superuser;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT 1 AS "ONE";
+
+SET ROLE regress_stats_user1;
+SELECT 1+1 AS "TWO";
+
+--
+-- A superuser can read all columns of queries executed by others,
+-- including query text and queryid.
+--
+
+SET ROLE regress_stats_superuser;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+--
+-- regress_stats_user1 has no privileges to read the query text or
+-- queryid of queries executed by others but can see statistics
+-- like calls and rows.
+--
+
+SET ROLE regress_stats_user1;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+--
+-- regress_stats_user2, with pg_read_all_stats role privileges, can
+-- read all columns, including query text and queryid, of queries
+-- executed by others.
+--
+
+SET ROLE regress_stats_user2;
+SELECT r.rolname, ss.queryid <> 0 AS queryid_bool, ss.query, ss.calls, ss.rows
+  FROM pg_stat_statements ss JOIN pg_roles r ON ss.userid = r.oid
+  ORDER BY r.rolname, ss.query COLLATE "C", ss.calls, ss.rows;
+
+--
+-- cleanup
+--
+
+RESET ROLE;
+DROP ROLE regress_stats_superuser;
+DROP ROLE regress_stats_user1;
+DROP ROLE regress_stats_user2;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
-- 
2.45.2

#11

Noname

kuroda.keisuke@nttcom.co.jp

over 1 year ago

In reply to: Fujii Masao (#10)

Re: Add privileges test for pg_stat_statements to improve coverage

I've slightly modified the comments in the regression tests for
clarity.
Attached is the v6 patch. If there are no objections,
I will push this version.

Thank you for updating patch! I have no objection.

Best Regards,
Keisuke Kuroda
NTT Comware

#12

Fujii Masao

masao.fujii@oss.nttdata.com

over 1 year ago

In reply to: Noname (#11)

Re: Add privileges test for pg_stat_statements to improve coverage

On 2024/07/24 10:23, kuroda.keisuke@nttcom.co.jp wrote:

I've slightly modified the comments in the regression tests for clarity.
Attached is the v6 patch. If there are no objections,
I will push this version.

Thank you for updating patch! I have no objection.

Pushed. Thanks!

Regards,

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