pg_stat_statements: more test coverage

Started by Peter Eisentrautabout 2 years ago13 messages
#1Peter Eisentraut
peter@eisentraut.org
5 attachment(s)

pg_stat_statements has some significant gaps in test coverage,
especially around the serialization of data around server restarts, so I
wrote a test for that and also made some other smaller tweaks to
increase the coverage a bit. These patches are all independent of each
other.

After that, the only major function that isn't tested is gc_qtexts().
Maybe a future project.

Attachments:

v1-0001-pg_stat_statements-Exclude-from-lcov-functions-th.patchtext/plain; charset=UTF-8; name=v1-0001-pg_stat_statements-Exclude-from-lcov-functions-th.patchDownload
From 32b51698b581b816f7ca2ff16c92be8d25e7fd66 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 14:25:26 +0100
Subject: [PATCH v1 1/5] pg_stat_statements: Exclude from lcov functions that
 are impossible to test

The current code only supports installing version 1.4 of
pg_stat_statements and upgrading from there.  So the C code entry
points for older versions are not reachable from the tests.  To
prevent this from ruining the test coverage figures, mark those
functions are excluded from lcov.
---
 contrib/pg_stat_statements/pg_stat_statements.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6f62a531f7..8ac73bf55e 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -311,13 +311,15 @@ static bool pgss_save = true;	/* whether to save stats across shutdown */
 PG_FUNCTION_INFO_V1(pg_stat_statements_reset);
 PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_7);
 PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_11);
+/* LCOV_EXCL_START */
+PG_FUNCTION_INFO_V1(pg_stat_statements);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
+/* LCOV_EXCL_STOP */
 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_1_9);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_10);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_11);
-PG_FUNCTION_INFO_V1(pg_stat_statements);
 PG_FUNCTION_INFO_V1(pg_stat_statements_info);
 
 static void pgss_shmem_request(void);
@@ -1610,6 +1612,8 @@ pg_stat_statements_1_3(PG_FUNCTION_ARGS)
 	return (Datum) 0;
 }
 
+/* LCOV_EXCL_START */
+
 Datum
 pg_stat_statements_1_2(PG_FUNCTION_ARGS)
 {
@@ -1633,6 +1637,8 @@ pg_stat_statements(PG_FUNCTION_ARGS)
 	return (Datum) 0;
 }
 
+/* LCOV_EXCL_STOP */
+
 /* Common code for all versions of pg_stat_statements() */
 static void
 pg_stat_statements_internal(FunctionCallInfo fcinfo,

base-commit: 3e2e0d5ad7fcb89d18a71cbfc885ef184e1b6f2e
-- 
2.43.0

v1-0002-pg_stat_statements-Add-coverage-for-pg_stat_state.patchtext/plain; charset=UTF-8; name=v1-0002-pg_stat_statements-Add-coverage-for-pg_stat_state.patchDownload
From 1033183cea71c6bd37934cedb972d35a4e251134 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 14:25:26 +0100
Subject: [PATCH v1 2/5] pg_stat_statements: Add coverage for
 pg_stat_statements_1_8()

This requires reading pg_stat_statements at least once while the 1.8
version of the extension is installed.
---
 .../expected/oldextversions.out               | 24 ++++++++++++-------
 .../pg_stat_statements/sql/oldextversions.sql |  3 ++-
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/contrib/pg_stat_statements/expected/oldextversions.out b/contrib/pg_stat_statements/expected/oldextversions.out
index ec317b0d6b..f3a90cac0a 100644
--- a/contrib/pg_stat_statements/expected/oldextversions.out
+++ b/contrib/pg_stat_statements/expected/oldextversions.out
@@ -88,6 +88,17 @@ SELECT count(*) > 0 AS has_data FROM pg_stat_statements;
 
 -- New functions and views for pg_stat_statements in 1.8
 AlTER EXTENSION pg_stat_statements UPDATE TO '1.8';
+SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
+                                                       pg_get_functiondef                                                       
+--------------------------------------------------------------------------------------------------------------------------------
+ CREATE OR REPLACE FUNCTION public.pg_stat_statements_reset(userid oid DEFAULT 0, dbid oid DEFAULT 0, queryid bigint DEFAULT 0)+
+  RETURNS void                                                                                                                 +
+  LANGUAGE c                                                                                                                   +
+  PARALLEL SAFE STRICT                                                                                                         +
+ AS '$libdir/pg_stat_statements', $function$pg_stat_statements_reset_1_7$function$                                             +
+ 
+(1 row)
+
 \d pg_stat_statements
                     View "public.pg_stat_statements"
        Column        |       Type       | Collation | Nullable | Default 
@@ -125,15 +136,10 @@ AlTER EXTENSION pg_stat_statements UPDATE TO '1.8';
  wal_fpi             | bigint           |           |          | 
  wal_bytes           | numeric          |           |          | 
 
-SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
-                                                       pg_get_functiondef                                                       
---------------------------------------------------------------------------------------------------------------------------------
- CREATE OR REPLACE FUNCTION public.pg_stat_statements_reset(userid oid DEFAULT 0, dbid oid DEFAULT 0, queryid bigint DEFAULT 0)+
-  RETURNS void                                                                                                                 +
-  LANGUAGE c                                                                                                                   +
-  PARALLEL SAFE STRICT                                                                                                         +
- AS '$libdir/pg_stat_statements', $function$pg_stat_statements_reset_1_7$function$                                             +
- 
+SELECT count(*) > 0 AS has_data FROM pg_stat_statements;
+ has_data 
+----------
+ t
 (1 row)
 
 -- New function pg_stat_statement_info, and new function
diff --git a/contrib/pg_stat_statements/sql/oldextversions.sql b/contrib/pg_stat_statements/sql/oldextversions.sql
index ec06caa5dd..5cf515f6b0 100644
--- a/contrib/pg_stat_statements/sql/oldextversions.sql
+++ b/contrib/pg_stat_statements/sql/oldextversions.sql
@@ -33,8 +33,9 @@ CREATE EXTENSION pg_stat_statements WITH VERSION '1.4';
 
 -- New functions and views for pg_stat_statements in 1.8
 AlTER EXTENSION pg_stat_statements UPDATE TO '1.8';
-\d pg_stat_statements
 SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
+\d pg_stat_statements
+SELECT count(*) > 0 AS has_data FROM pg_stat_statements;
 
 -- New function pg_stat_statement_info, and new function
 -- and view for pg_stat_statements introduced in 1.9
-- 
2.43.0

v1-0003-pg_stat_statements-Add-coverage-for-pg_stat_state.patchtext/plain; charset=UTF-8; name=v1-0003-pg_stat_statements-Add-coverage-for-pg_stat_state.patchDownload
From 154485f21bd5e826646874c5febc6c4fea68a1b8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 14:25:26 +0100
Subject: [PATCH v1 3/5] pg_stat_statements: Add coverage for
 pg_stat_statements_reset_1_7

Run pg_stat_statements_reset() once while the appropriate extension
version is installed.
---
 .../pg_stat_statements/expected/oldextversions.out   | 12 ++++++++++++
 contrib/pg_stat_statements/sql/oldextversions.sql    |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/contrib/pg_stat_statements/expected/oldextversions.out b/contrib/pg_stat_statements/expected/oldextversions.out
index f3a90cac0a..5842c930e5 100644
--- a/contrib/pg_stat_statements/expected/oldextversions.out
+++ b/contrib/pg_stat_statements/expected/oldextversions.out
@@ -52,6 +52,12 @@ SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
  
 (1 row)
 
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset 
+--------------------------
+ 
+(1 row)
+
 \d pg_stat_statements
                     View "public.pg_stat_statements"
        Column        |       Type       | Collation | Nullable | Default 
@@ -330,4 +336,10 @@ SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
  
 (1 row)
 
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
+(1 row)
+
 DROP EXTENSION pg_stat_statements;
diff --git a/contrib/pg_stat_statements/sql/oldextversions.sql b/contrib/pg_stat_statements/sql/oldextversions.sql
index 5cf515f6b0..38d5505d0d 100644
--- a/contrib/pg_stat_statements/sql/oldextversions.sql
+++ b/contrib/pg_stat_statements/sql/oldextversions.sql
@@ -28,6 +28,7 @@ CREATE EXTENSION pg_stat_statements WITH VERSION '1.4';
 SELECT pg_stat_statements_reset();
 RESET SESSION AUTHORIZATION;
 SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
+SELECT pg_stat_statements_reset();
 \d pg_stat_statements
 SELECT count(*) > 0 AS has_data FROM pg_stat_statements;
 
@@ -55,5 +56,6 @@ CREATE EXTENSION pg_stat_statements WITH VERSION '1.4';
 SELECT count(*) > 0 AS has_data FROM pg_stat_statements;
 -- New parameter minmax_only of pg_stat_statements_reset function
 SELECT pg_get_functiondef('pg_stat_statements_reset'::regproc);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
 
 DROP EXTENSION pg_stat_statements;
-- 
2.43.0

v1-0004-pg_stat_statements-Add-coverage-for-entry_dealloc.patchtext/plain; charset=UTF-8; name=v1-0004-pg_stat_statements-Add-coverage-for-entry_dealloc.patchDownload
From 6e023ad2bebd534b9ce7ac2697641d1759a8ecf0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 14:25:26 +0100
Subject: [PATCH v1 4/5] pg_stat_statements: Add coverage for entry_dealloc()

This involves creating pg_stat_statements.max entries, then creating
even more entries, and checking that the limit is kept and the least
used entries are kicked out.
---
 contrib/pg_stat_statements/Makefile           |    2 +-
 contrib/pg_stat_statements/expected/max.out   | 1127 +++++++++++++++++
 contrib/pg_stat_statements/meson.build        |    1 +
 .../pg_stat_statements.conf                   |    2 +
 contrib/pg_stat_statements/sql/max.sql        |   18 +
 5 files changed, 1149 insertions(+), 1 deletion(-)
 create mode 100644 contrib/pg_stat_statements/expected/max.out
 create mode 100644 contrib/pg_stat_statements/sql/max.sql

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index aecd1d6a2a..7ee16e8350 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -19,7 +19,7 @@ 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 max 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/max.out b/contrib/pg_stat_statements/expected/max.out
new file mode 100644
index 0000000000..60750d15c2
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/max.out
@@ -0,0 +1,1127 @@
+SHOW pg_stat_statements.max;
+ pg_stat_statements.max 
+------------------------
+ 100
+(1 row)
+
+SELECT format('create table t%s (a int)', lpad(i::text, 3, '0')) FROM generate_series(1, 101) AS x(i) \gexec
+create table t001 (a int)
+create table t002 (a int)
+create table t003 (a int)
+create table t004 (a int)
+create table t005 (a int)
+create table t006 (a int)
+create table t007 (a int)
+create table t008 (a int)
+create table t009 (a int)
+create table t010 (a int)
+create table t011 (a int)
+create table t012 (a int)
+create table t013 (a int)
+create table t014 (a int)
+create table t015 (a int)
+create table t016 (a int)
+create table t017 (a int)
+create table t018 (a int)
+create table t019 (a int)
+create table t020 (a int)
+create table t021 (a int)
+create table t022 (a int)
+create table t023 (a int)
+create table t024 (a int)
+create table t025 (a int)
+create table t026 (a int)
+create table t027 (a int)
+create table t028 (a int)
+create table t029 (a int)
+create table t030 (a int)
+create table t031 (a int)
+create table t032 (a int)
+create table t033 (a int)
+create table t034 (a int)
+create table t035 (a int)
+create table t036 (a int)
+create table t037 (a int)
+create table t038 (a int)
+create table t039 (a int)
+create table t040 (a int)
+create table t041 (a int)
+create table t042 (a int)
+create table t043 (a int)
+create table t044 (a int)
+create table t045 (a int)
+create table t046 (a int)
+create table t047 (a int)
+create table t048 (a int)
+create table t049 (a int)
+create table t050 (a int)
+create table t051 (a int)
+create table t052 (a int)
+create table t053 (a int)
+create table t054 (a int)
+create table t055 (a int)
+create table t056 (a int)
+create table t057 (a int)
+create table t058 (a int)
+create table t059 (a int)
+create table t060 (a int)
+create table t061 (a int)
+create table t062 (a int)
+create table t063 (a int)
+create table t064 (a int)
+create table t065 (a int)
+create table t066 (a int)
+create table t067 (a int)
+create table t068 (a int)
+create table t069 (a int)
+create table t070 (a int)
+create table t071 (a int)
+create table t072 (a int)
+create table t073 (a int)
+create table t074 (a int)
+create table t075 (a int)
+create table t076 (a int)
+create table t077 (a int)
+create table t078 (a int)
+create table t079 (a int)
+create table t080 (a int)
+create table t081 (a int)
+create table t082 (a int)
+create table t083 (a int)
+create table t084 (a int)
+create table t085 (a int)
+create table t086 (a int)
+create table t087 (a int)
+create table t088 (a int)
+create table t089 (a int)
+create table t090 (a int)
+create table t091 (a int)
+create table t092 (a int)
+create table t093 (a int)
+create table t094 (a int)
+create table t095 (a int)
+create table t096 (a int)
+create table t097 (a int)
+create table t098 (a int)
+create table t099 (a int)
+create table t100 (a int)
+create table t101 (a int)
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
+(1 row)
+
+SELECT format('select * from t%s', lpad(i::text, 3, '0')) FROM generate_series(1, 98) AS x(i) \gexec
+select * from t001
+ a 
+---
+(0 rows)
+
+select * from t002
+ a 
+---
+(0 rows)
+
+select * from t003
+ a 
+---
+(0 rows)
+
+select * from t004
+ a 
+---
+(0 rows)
+
+select * from t005
+ a 
+---
+(0 rows)
+
+select * from t006
+ a 
+---
+(0 rows)
+
+select * from t007
+ a 
+---
+(0 rows)
+
+select * from t008
+ a 
+---
+(0 rows)
+
+select * from t009
+ a 
+---
+(0 rows)
+
+select * from t010
+ a 
+---
+(0 rows)
+
+select * from t011
+ a 
+---
+(0 rows)
+
+select * from t012
+ a 
+---
+(0 rows)
+
+select * from t013
+ a 
+---
+(0 rows)
+
+select * from t014
+ a 
+---
+(0 rows)
+
+select * from t015
+ a 
+---
+(0 rows)
+
+select * from t016
+ a 
+---
+(0 rows)
+
+select * from t017
+ a 
+---
+(0 rows)
+
+select * from t018
+ a 
+---
+(0 rows)
+
+select * from t019
+ a 
+---
+(0 rows)
+
+select * from t020
+ a 
+---
+(0 rows)
+
+select * from t021
+ a 
+---
+(0 rows)
+
+select * from t022
+ a 
+---
+(0 rows)
+
+select * from t023
+ a 
+---
+(0 rows)
+
+select * from t024
+ a 
+---
+(0 rows)
+
+select * from t025
+ a 
+---
+(0 rows)
+
+select * from t026
+ a 
+---
+(0 rows)
+
+select * from t027
+ a 
+---
+(0 rows)
+
+select * from t028
+ a 
+---
+(0 rows)
+
+select * from t029
+ a 
+---
+(0 rows)
+
+select * from t030
+ a 
+---
+(0 rows)
+
+select * from t031
+ a 
+---
+(0 rows)
+
+select * from t032
+ a 
+---
+(0 rows)
+
+select * from t033
+ a 
+---
+(0 rows)
+
+select * from t034
+ a 
+---
+(0 rows)
+
+select * from t035
+ a 
+---
+(0 rows)
+
+select * from t036
+ a 
+---
+(0 rows)
+
+select * from t037
+ a 
+---
+(0 rows)
+
+select * from t038
+ a 
+---
+(0 rows)
+
+select * from t039
+ a 
+---
+(0 rows)
+
+select * from t040
+ a 
+---
+(0 rows)
+
+select * from t041
+ a 
+---
+(0 rows)
+
+select * from t042
+ a 
+---
+(0 rows)
+
+select * from t043
+ a 
+---
+(0 rows)
+
+select * from t044
+ a 
+---
+(0 rows)
+
+select * from t045
+ a 
+---
+(0 rows)
+
+select * from t046
+ a 
+---
+(0 rows)
+
+select * from t047
+ a 
+---
+(0 rows)
+
+select * from t048
+ a 
+---
+(0 rows)
+
+select * from t049
+ a 
+---
+(0 rows)
+
+select * from t050
+ a 
+---
+(0 rows)
+
+select * from t051
+ a 
+---
+(0 rows)
+
+select * from t052
+ a 
+---
+(0 rows)
+
+select * from t053
+ a 
+---
+(0 rows)
+
+select * from t054
+ a 
+---
+(0 rows)
+
+select * from t055
+ a 
+---
+(0 rows)
+
+select * from t056
+ a 
+---
+(0 rows)
+
+select * from t057
+ a 
+---
+(0 rows)
+
+select * from t058
+ a 
+---
+(0 rows)
+
+select * from t059
+ a 
+---
+(0 rows)
+
+select * from t060
+ a 
+---
+(0 rows)
+
+select * from t061
+ a 
+---
+(0 rows)
+
+select * from t062
+ a 
+---
+(0 rows)
+
+select * from t063
+ a 
+---
+(0 rows)
+
+select * from t064
+ a 
+---
+(0 rows)
+
+select * from t065
+ a 
+---
+(0 rows)
+
+select * from t066
+ a 
+---
+(0 rows)
+
+select * from t067
+ a 
+---
+(0 rows)
+
+select * from t068
+ a 
+---
+(0 rows)
+
+select * from t069
+ a 
+---
+(0 rows)
+
+select * from t070
+ a 
+---
+(0 rows)
+
+select * from t071
+ a 
+---
+(0 rows)
+
+select * from t072
+ a 
+---
+(0 rows)
+
+select * from t073
+ a 
+---
+(0 rows)
+
+select * from t074
+ a 
+---
+(0 rows)
+
+select * from t075
+ a 
+---
+(0 rows)
+
+select * from t076
+ a 
+---
+(0 rows)
+
+select * from t077
+ a 
+---
+(0 rows)
+
+select * from t078
+ a 
+---
+(0 rows)
+
+select * from t079
+ a 
+---
+(0 rows)
+
+select * from t080
+ a 
+---
+(0 rows)
+
+select * from t081
+ a 
+---
+(0 rows)
+
+select * from t082
+ a 
+---
+(0 rows)
+
+select * from t083
+ a 
+---
+(0 rows)
+
+select * from t084
+ a 
+---
+(0 rows)
+
+select * from t085
+ a 
+---
+(0 rows)
+
+select * from t086
+ a 
+---
+(0 rows)
+
+select * from t087
+ a 
+---
+(0 rows)
+
+select * from t088
+ a 
+---
+(0 rows)
+
+select * from t089
+ a 
+---
+(0 rows)
+
+select * from t090
+ a 
+---
+(0 rows)
+
+select * from t091
+ a 
+---
+(0 rows)
+
+select * from t092
+ a 
+---
+(0 rows)
+
+select * from t093
+ a 
+---
+(0 rows)
+
+select * from t094
+ a 
+---
+(0 rows)
+
+select * from t095
+ a 
+---
+(0 rows)
+
+select * from t096
+ a 
+---
+(0 rows)
+
+select * from t097
+ a 
+---
+(0 rows)
+
+select * from t098
+ a 
+---
+(0 rows)
+
+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;
+ ?column? 
+----------
+ t
+(1 row)
+
+SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' OR query LIKE '%t098%' ORDER BY query;
+       query        
+--------------------
+ select * from t001
+ select * from t098
+(2 rows)
+
+SELECT format('select * from t%s', lpad(i::text, 3, '0')) from generate_series(2, 98) AS x(i) \gexec
+select * from t002
+ a 
+---
+(0 rows)
+
+select * from t003
+ a 
+---
+(0 rows)
+
+select * from t004
+ a 
+---
+(0 rows)
+
+select * from t005
+ a 
+---
+(0 rows)
+
+select * from t006
+ a 
+---
+(0 rows)
+
+select * from t007
+ a 
+---
+(0 rows)
+
+select * from t008
+ a 
+---
+(0 rows)
+
+select * from t009
+ a 
+---
+(0 rows)
+
+select * from t010
+ a 
+---
+(0 rows)
+
+select * from t011
+ a 
+---
+(0 rows)
+
+select * from t012
+ a 
+---
+(0 rows)
+
+select * from t013
+ a 
+---
+(0 rows)
+
+select * from t014
+ a 
+---
+(0 rows)
+
+select * from t015
+ a 
+---
+(0 rows)
+
+select * from t016
+ a 
+---
+(0 rows)
+
+select * from t017
+ a 
+---
+(0 rows)
+
+select * from t018
+ a 
+---
+(0 rows)
+
+select * from t019
+ a 
+---
+(0 rows)
+
+select * from t020
+ a 
+---
+(0 rows)
+
+select * from t021
+ a 
+---
+(0 rows)
+
+select * from t022
+ a 
+---
+(0 rows)
+
+select * from t023
+ a 
+---
+(0 rows)
+
+select * from t024
+ a 
+---
+(0 rows)
+
+select * from t025
+ a 
+---
+(0 rows)
+
+select * from t026
+ a 
+---
+(0 rows)
+
+select * from t027
+ a 
+---
+(0 rows)
+
+select * from t028
+ a 
+---
+(0 rows)
+
+select * from t029
+ a 
+---
+(0 rows)
+
+select * from t030
+ a 
+---
+(0 rows)
+
+select * from t031
+ a 
+---
+(0 rows)
+
+select * from t032
+ a 
+---
+(0 rows)
+
+select * from t033
+ a 
+---
+(0 rows)
+
+select * from t034
+ a 
+---
+(0 rows)
+
+select * from t035
+ a 
+---
+(0 rows)
+
+select * from t036
+ a 
+---
+(0 rows)
+
+select * from t037
+ a 
+---
+(0 rows)
+
+select * from t038
+ a 
+---
+(0 rows)
+
+select * from t039
+ a 
+---
+(0 rows)
+
+select * from t040
+ a 
+---
+(0 rows)
+
+select * from t041
+ a 
+---
+(0 rows)
+
+select * from t042
+ a 
+---
+(0 rows)
+
+select * from t043
+ a 
+---
+(0 rows)
+
+select * from t044
+ a 
+---
+(0 rows)
+
+select * from t045
+ a 
+---
+(0 rows)
+
+select * from t046
+ a 
+---
+(0 rows)
+
+select * from t047
+ a 
+---
+(0 rows)
+
+select * from t048
+ a 
+---
+(0 rows)
+
+select * from t049
+ a 
+---
+(0 rows)
+
+select * from t050
+ a 
+---
+(0 rows)
+
+select * from t051
+ a 
+---
+(0 rows)
+
+select * from t052
+ a 
+---
+(0 rows)
+
+select * from t053
+ a 
+---
+(0 rows)
+
+select * from t054
+ a 
+---
+(0 rows)
+
+select * from t055
+ a 
+---
+(0 rows)
+
+select * from t056
+ a 
+---
+(0 rows)
+
+select * from t057
+ a 
+---
+(0 rows)
+
+select * from t058
+ a 
+---
+(0 rows)
+
+select * from t059
+ a 
+---
+(0 rows)
+
+select * from t060
+ a 
+---
+(0 rows)
+
+select * from t061
+ a 
+---
+(0 rows)
+
+select * from t062
+ a 
+---
+(0 rows)
+
+select * from t063
+ a 
+---
+(0 rows)
+
+select * from t064
+ a 
+---
+(0 rows)
+
+select * from t065
+ a 
+---
+(0 rows)
+
+select * from t066
+ a 
+---
+(0 rows)
+
+select * from t067
+ a 
+---
+(0 rows)
+
+select * from t068
+ a 
+---
+(0 rows)
+
+select * from t069
+ a 
+---
+(0 rows)
+
+select * from t070
+ a 
+---
+(0 rows)
+
+select * from t071
+ a 
+---
+(0 rows)
+
+select * from t072
+ a 
+---
+(0 rows)
+
+select * from t073
+ a 
+---
+(0 rows)
+
+select * from t074
+ a 
+---
+(0 rows)
+
+select * from t075
+ a 
+---
+(0 rows)
+
+select * from t076
+ a 
+---
+(0 rows)
+
+select * from t077
+ a 
+---
+(0 rows)
+
+select * from t078
+ a 
+---
+(0 rows)
+
+select * from t079
+ a 
+---
+(0 rows)
+
+select * from t080
+ a 
+---
+(0 rows)
+
+select * from t081
+ a 
+---
+(0 rows)
+
+select * from t082
+ a 
+---
+(0 rows)
+
+select * from t083
+ a 
+---
+(0 rows)
+
+select * from t084
+ a 
+---
+(0 rows)
+
+select * from t085
+ a 
+---
+(0 rows)
+
+select * from t086
+ a 
+---
+(0 rows)
+
+select * from t087
+ a 
+---
+(0 rows)
+
+select * from t088
+ a 
+---
+(0 rows)
+
+select * from t089
+ a 
+---
+(0 rows)
+
+select * from t090
+ a 
+---
+(0 rows)
+
+select * from t091
+ a 
+---
+(0 rows)
+
+select * from t092
+ a 
+---
+(0 rows)
+
+select * from t093
+ a 
+---
+(0 rows)
+
+select * from t094
+ a 
+---
+(0 rows)
+
+select * from t095
+ a 
+---
+(0 rows)
+
+select * from t096
+ a 
+---
+(0 rows)
+
+select * from t097
+ a 
+---
+(0 rows)
+
+select * from t098
+ a 
+---
+(0 rows)
+
+SELECT format('select * from t%s', lpad(i::text, 3, '0')) from generate_series(99, 100) AS x(i) \gexec
+select * from t099
+ a 
+---
+(0 rows)
+
+select * from t100
+ a 
+---
+(0 rows)
+
+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;
+ ?column? 
+----------
+ t
+(1 row)
+
+-- record for t001 has been kicked out
+SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' ORDER BY query;
+ query 
+-------
+(0 rows)
+
diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index 81fe1eb917..a66acaa5b8 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',
+      'max',
       'cleanup',
       'oldextversions',
     ],
diff --git a/contrib/pg_stat_statements/pg_stat_statements.conf b/contrib/pg_stat_statements/pg_stat_statements.conf
index 0e900d7119..0119f681d7 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.conf
+++ b/contrib/pg_stat_statements/pg_stat_statements.conf
@@ -1,2 +1,4 @@
 shared_preload_libraries = 'pg_stat_statements'
 max_prepared_transactions = 5
+
+pg_stat_statements.max = 100
diff --git a/contrib/pg_stat_statements/sql/max.sql b/contrib/pg_stat_statements/sql/max.sql
new file mode 100644
index 0000000000..ce6a1bd84e
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/max.sql
@@ -0,0 +1,18 @@
+SHOW pg_stat_statements.max;
+
+SELECT format('create table t%s (a int)', lpad(i::text, 3, '0')) FROM generate_series(1, 101) AS x(i) \gexec
+
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+
+SELECT format('select * from t%s', lpad(i::text, 3, '0')) FROM generate_series(1, 98) AS x(i) \gexec
+
+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;
+SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' OR query LIKE '%t098%' ORDER BY query;
+
+SELECT format('select * from t%s', lpad(i::text, 3, '0')) from generate_series(2, 98) AS x(i) \gexec
+
+SELECT format('select * from t%s', lpad(i::text, 3, '0')) from generate_series(99, 100) AS x(i) \gexec
+
+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;
+-- record for t001 has been kicked out
+SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' ORDER BY query;
-- 
2.43.0

v1-0005-pg_stat_statements-Add-TAP-test-for-testing-resta.patchtext/plain; charset=UTF-8; name=v1-0005-pg_stat_statements-Add-TAP-test-for-testing-resta.patchDownload
From b4a64f5e4fbf7b08e14caa680f88eebb09da2c69 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 14:25:26 +0100
Subject: [PATCH v1 5/5] pg_stat_statements: Add TAP test for testing restarts

This tests that pg_stat_statement contents are successfully kept
across restart.  (This similar to
src/test/recovery/t/029_stats_restart.pl for the stats collector.)
---
 contrib/pg_stat_statements/Makefile         |  2 +
 contrib/pg_stat_statements/meson.build      |  5 +++
 contrib/pg_stat_statements/t/010_restart.pl | 50 +++++++++++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100644 contrib/pg_stat_statements/t/010_restart.pl

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 7ee16e8350..20834bb0ee 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -24,6 +24,8 @@ REGRESS = select dml cursors utility level_tracking planning \
 # which typical installcheck users do not have (e.g. buildfarm clients).
 NO_INSTALLCHECK = 1
 
+TAP_TESTS = 1
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index a66acaa5b8..3e42328f6c 100644
--- a/contrib/pg_stat_statements/meson.build
+++ b/contrib/pg_stat_statements/meson.build
@@ -60,4 +60,9 @@ tests += {
     # runningcheck users do not have (e.g. buildfarm clients).
     'runningcheck': false,
   },
+  'tap': {
+    'tests': [
+      't/010_restart.pl',
+    ],
+  },
 }
diff --git a/contrib/pg_stat_statements/t/010_restart.pl b/contrib/pg_stat_statements/t/010_restart.pl
new file mode 100644
index 0000000000..bf0fba6bda
--- /dev/null
+++ b/contrib/pg_stat_statements/t/010_restart.pl
@@ -0,0 +1,50 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Tests for checking that pg_stat_statements contents are preserved
+# across restarts.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+	"shared_preload_libraries = 'pg_stat_statements'");
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION pg_stat_statements');
+
+$node->safe_psql('postgres', 'CREATE TABLE t1 (a int)');
+$node->safe_psql('postgres', 'SELECT a FROM t1');
+
+is( $node->safe_psql(
+		'postgres',
+		"SELECT count(*) FROM pg_stat_statements WHERE query LIKE '%t1%'"),
+	'2',
+	'pg_stat_statements populated');
+
+$node->restart;
+
+is( $node->safe_psql(
+		'postgres',
+		"SELECT count(*) FROM pg_stat_statements WHERE query LIKE '%t1%'"),
+	'2',
+	'pg_stat_statements data kept across restart');
+
+$node->append_conf('postgresql.conf', "pg_stat_statements.save = false");
+$node->reload;
+
+$node->restart;
+
+is( $node->safe_psql(
+		'postgres',
+		"SELECT count(*) FROM pg_stat_statements WHERE query LIKE '%t1%'"),
+	'0',
+	'pg_stat_statements data not kept across restart with .save=false');
+
+$node->stop;
+
+done_testing();
-- 
2.43.0

#2Michael Paquier
michael@paquier.xyz
In reply to: Peter Eisentraut (#1)
Re: pg_stat_statements: more test coverage

On Sat, Dec 23, 2023 at 03:18:01PM +0100, Peter Eisentraut wrote:

+/* LCOV_EXCL_START */
+PG_FUNCTION_INFO_V1(pg_stat_statements);
PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
+/* LCOV_EXCL_STOP */

The only reason why I've seen this used at the C level was to bump up
the coverage requirements because of some internal company projects.
I'm pretty sure to have proposed in the past at least one patch that
would make use of that, but it got rejected. It is not the only code
area that has a similar pattern. So why introducing that now?

Subject: [PATCH v1 2/5] pg_stat_statements: Add coverage for
pg_stat_statements_1_8()

[...]

Subject: [PATCH v1 3/5] pg_stat_statements: Add coverage for
pg_stat_statements_reset_1_7

Yep, why not.

+SELECT format('create table t%s (a int)', lpad(i::text, 3, '0')) FROM generate_series(1, 101) AS x(i) \gexec
+create table t001 (a int)
[...]
+create table t101 (a int)

That's a lot of bloat. This relies on pg_stat_statements.max's
default to be at 100. Based on the regression tests, the maximum
number of rows we have reported from the view pg_stat_statements is
39 in utility.c. I think that you should just:
- Use a DO block of a PL function, say with something like that to
ensure an amount of N queries? Say with something like that after
tweaking pg_stat_statements.track:
CREATE OR REPLACE FUNCTION create_tables(num_tables int)
RETURNS VOID AS
$func$
BEGIN
FOR i IN 1..num_tables LOOP
EXECUTE format('
CREATE TABLE IF NOT EXISTS %I (id int)', 't_' || i);
END LOOP;
END
$func$ LANGUAGE plpgsql;
- Switch the minimum to be around 40~50 in the local
pg_stat_statements.conf used for the regression tests.

+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;

You could fetch the max value in a \get and reuse it here, as well.

+is( $node->safe_psql(
+		'postgres',
+		"SELECT count(*) FROM pg_stat_statements WHERE query LIKE '%t1%'"),
+	'2',
+	'pg_stat_statements data kept across restart');

Checking that the contents match would be a bit more verbose than just
a count. One trick I've used in the patch is in
027_stream_regress.pl, where there is a query grouping the stats
depending on the beginning of the queries. Not exact, a bit more
verbose.
--
Michael

#3Peter Eisentraut
peter@eisentraut.org
In reply to: Michael Paquier (#2)
Re: pg_stat_statements: more test coverage

On 24.12.23 03:03, Michael Paquier wrote:

On Sat, Dec 23, 2023 at 03:18:01PM +0100, Peter Eisentraut wrote:

+/* LCOV_EXCL_START */
+PG_FUNCTION_INFO_V1(pg_stat_statements);
PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
+/* LCOV_EXCL_STOP */

The only reason why I've seen this used at the C level was to bump up
the coverage requirements because of some internal company projects.
I'm pretty sure to have proposed in the past at least one patch that
would make use of that, but it got rejected. It is not the only code
area that has a similar pattern. So why introducing that now?

What other code areas have similar patterns (meaning, extension entry
points for upgrade support that are not covered by currently available
extension installation files)?

That's a lot of bloat. This relies on pg_stat_statements.max's
default to be at 100.

The default is 5000. I set 100 explicitly in the configuration file for
the test.

- Use a DO block of a PL function, say with something like that to
ensure an amount of N queries? Say with something like that after
tweaking pg_stat_statements.track:
CREATE OR REPLACE FUNCTION create_tables(num_tables int)
RETURNS VOID AS
$func$
BEGIN
FOR i IN 1..num_tables LOOP
EXECUTE format('
CREATE TABLE IF NOT EXISTS %I (id int)', 't_' || i);
END LOOP;
END
$func$ LANGUAGE plpgsql;

I tried it like this first, but this doesn't register as separately
executed commands for pg_stat_statements.

- Switch the minimum to be around 40~50 in the local
pg_stat_statements.conf used for the regression tests.

100 is the hardcoded minimum for the setting.

+is( $node->safe_psql(
+		'postgres',
+		"SELECT count(*) FROM pg_stat_statements WHERE query LIKE '%t1%'"),
+	'2',
+	'pg_stat_statements data kept across restart');

Checking that the contents match would be a bit more verbose than just
a count. One trick I've used in the patch is in
027_stream_regress.pl, where there is a query grouping the stats
depending on the beginning of the queries. Not exact, a bit more
verbose.

Yeah, this could be expanded a bit.

#4Julien Rouhaud
rjuju123@gmail.com
In reply to: Peter Eisentraut (#3)
Re: pg_stat_statements: more test coverage

Hi,

On Tue, Dec 26, 2023 at 10:03 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 24.12.23 03:03, Michael Paquier wrote:

- Use a DO block of a PL function, say with something like that to
ensure an amount of N queries? Say with something like that after
tweaking pg_stat_statements.track:
CREATE OR REPLACE FUNCTION create_tables(num_tables int)
RETURNS VOID AS
$func$
BEGIN
FOR i IN 1..num_tables LOOP
EXECUTE format('
CREATE TABLE IF NOT EXISTS %I (id int)', 't_' || i);
END LOOP;
END
$func$ LANGUAGE plpgsql;

I tried it like this first, but this doesn't register as separately
executed commands for pg_stat_statements.

I was a bit surprised by that so I checked locally. It does work as
expected provided that you set pg_stat_statements.track to all:
=# select create_tables(5);
=# select queryid, query from pg_stat_statements where query like 'CREATE%';
queryid | query
----------------------+-----------------------------------------
-4985234599080337259 | CREATE TABLE IF NOT EXISTS t_5 (id int)
-790506371630237058 | CREATE TABLE IF NOT EXISTS t_2 (id int)
-1104545884488896333 | CREATE TABLE IF NOT EXISTS t_3 (id int)
-2961032912789520428 | CREATE TABLE IF NOT EXISTS t_4 (id int)
7273321309563119428 | CREATE TABLE IF NOT EXISTS t_1 (id int)

#5Peter Eisentraut
peter@eisentraut.org
In reply to: Julien Rouhaud (#4)
3 attachment(s)
Re: pg_stat_statements: more test coverage

On 27.12.23 09:08, Julien Rouhaud wrote:

Hi,

On Tue, Dec 26, 2023 at 10:03 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 24.12.23 03:03, Michael Paquier wrote:

- Use a DO block of a PL function, say with something like that to
ensure an amount of N queries? Say with something like that after
tweaking pg_stat_statements.track:
CREATE OR REPLACE FUNCTION create_tables(num_tables int)
RETURNS VOID AS
$func$
BEGIN
FOR i IN 1..num_tables LOOP
EXECUTE format('
CREATE TABLE IF NOT EXISTS %I (id int)', 't_' || i);
END LOOP;
END
$func$ LANGUAGE plpgsql;

I tried it like this first, but this doesn't register as separately
executed commands for pg_stat_statements.

I was a bit surprised by that so I checked locally. It does work as
expected provided that you set pg_stat_statements.track to all:

Ok, here is an updated patch set that does it that way.

I have committed the patches 0002 and 0003 from the previous patch set
already.

I have also enhanced the TAP test a bit to check the actual content of
the output across restarts.

I'm not too hung up on the 0001 patch if others don't like that approach.

Attachments:

v2-0001-pg_stat_statements-Exclude-from-lcov-functions-th.patchtext/plain; charset=UTF-8; name=v2-0001-pg_stat_statements-Exclude-from-lcov-functions-th.patchDownload
From 8938017869025598024f0aba950c0aeccbcbe651 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 14:25:26 +0100
Subject: [PATCH v2 1/3] pg_stat_statements: Exclude from lcov functions that
 are impossible to test

The current code only supports installing version 1.4 of
pg_stat_statements and upgrading from there.  So the C code entry
points for older versions are not reachable from the tests.  To
prevent this from ruining the test coverage figures, mark those
functions are excluded from lcov.

Discussion: https://www.postgresql.org/message-id/flat/40d1e4f2-835f-448f-a541-8ff5db75bf3d@eisentraut.org
---
 contrib/pg_stat_statements/pg_stat_statements.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6f62a531f7..8ac73bf55e 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -311,13 +311,15 @@ static bool pgss_save = true;	/* whether to save stats across shutdown */
 PG_FUNCTION_INFO_V1(pg_stat_statements_reset);
 PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_7);
 PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_11);
+/* LCOV_EXCL_START */
+PG_FUNCTION_INFO_V1(pg_stat_statements);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_2);
+/* LCOV_EXCL_STOP */
 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_1_9);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_10);
 PG_FUNCTION_INFO_V1(pg_stat_statements_1_11);
-PG_FUNCTION_INFO_V1(pg_stat_statements);
 PG_FUNCTION_INFO_V1(pg_stat_statements_info);
 
 static void pgss_shmem_request(void);
@@ -1610,6 +1612,8 @@ pg_stat_statements_1_3(PG_FUNCTION_ARGS)
 	return (Datum) 0;
 }
 
+/* LCOV_EXCL_START */
+
 Datum
 pg_stat_statements_1_2(PG_FUNCTION_ARGS)
 {
@@ -1633,6 +1637,8 @@ pg_stat_statements(PG_FUNCTION_ARGS)
 	return (Datum) 0;
 }
 
+/* LCOV_EXCL_STOP */
+
 /* Common code for all versions of pg_stat_statements() */
 static void
 pg_stat_statements_internal(FunctionCallInfo fcinfo,

base-commit: 7e6fb5da41d8ee1bddcd5058b7204018ef68d193
-- 
2.43.0

v2-0002-pg_stat_statements-Add-coverage-for-entry_dealloc.patchtext/plain; charset=UTF-8; name=v2-0002-pg_stat_statements-Add-coverage-for-entry_dealloc.patchDownload
From 6fa54b21ae6060fd459f5330130491662361f2d5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 23 Dec 2023 14:25:26 +0100
Subject: [PATCH v2 2/3] pg_stat_statements: Add coverage for entry_dealloc()

This involves creating pg_stat_statements.max entries, then creating
even more entries, and checking that the limit is kept and the least
used entries are kicked out.

Discussion: https://www.postgresql.org/message-id/flat/40d1e4f2-835f-448f-a541-8ff5db75bf3d@eisentraut.org
---
 contrib/pg_stat_statements/Makefile           |  2 +-
 contrib/pg_stat_statements/expected/max.out   | 66 +++++++++++++++++++
 contrib/pg_stat_statements/meson.build        |  1 +
 .../pg_stat_statements.conf                   |  2 +
 contrib/pg_stat_statements/sql/max.sql        | 44 +++++++++++++
 5 files changed, 114 insertions(+), 1 deletion(-)
 create mode 100644 contrib/pg_stat_statements/expected/max.out
 create mode 100644 contrib/pg_stat_statements/sql/max.sql

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index aecd1d6a2a..7ee16e8350 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -19,7 +19,7 @@ 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 max 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/max.out b/contrib/pg_stat_statements/expected/max.out
new file mode 100644
index 0000000000..49c3f56efd
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/max.out
@@ -0,0 +1,66 @@
+SHOW pg_stat_statements.max;
+ pg_stat_statements.max 
+------------------------
+ 100
+(1 row)
+
+SET pg_stat_statements.track = 'all';
+DO $$
+BEGIN
+  FOR i IN 1..101 LOOP
+    EXECUTE format('create table t%s (a int)', lpad(i::text, 3, '0'));
+  END LOOP;
+END
+$$;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
+(1 row)
+
+DO $$
+BEGIN
+  FOR i IN 1..98 LOOP
+    EXECUTE format('select * from t%s', lpad(i::text, 3, '0'));
+  END LOOP;
+END
+$$;
+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;
+ ?column? 
+----------
+ t
+(1 row)
+
+SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' OR query LIKE '%t098%' ORDER BY query;
+       query        
+--------------------
+ select * from t001
+ select * from t098
+(2 rows)
+
+DO $$
+BEGIN
+  FOR i IN 2..98 LOOP
+    EXECUTE format('select * from t%s', lpad(i::text, 3, '0'));
+  END LOOP;
+END
+$$;
+DO $$
+BEGIN
+  FOR i IN 99..100 LOOP
+    EXECUTE format('select * from t%s', lpad(i::text, 3, '0'));
+  END LOOP;
+END
+$$;
+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;
+ ?column? 
+----------
+ t
+(1 row)
+
+-- record for t001 has been kicked out
+SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' ORDER BY query;
+ query 
+-------
+(0 rows)
+
diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index 81fe1eb917..a66acaa5b8 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',
+      'max',
       'cleanup',
       'oldextversions',
     ],
diff --git a/contrib/pg_stat_statements/pg_stat_statements.conf b/contrib/pg_stat_statements/pg_stat_statements.conf
index 0e900d7119..0119f681d7 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.conf
+++ b/contrib/pg_stat_statements/pg_stat_statements.conf
@@ -1,2 +1,4 @@
 shared_preload_libraries = 'pg_stat_statements'
 max_prepared_transactions = 5
+
+pg_stat_statements.max = 100
diff --git a/contrib/pg_stat_statements/sql/max.sql b/contrib/pg_stat_statements/sql/max.sql
new file mode 100644
index 0000000000..741fbc9851
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/max.sql
@@ -0,0 +1,44 @@
+SHOW pg_stat_statements.max;
+
+SET pg_stat_statements.track = 'all';
+
+DO $$
+BEGIN
+  FOR i IN 1..101 LOOP
+    EXECUTE format('create table t%s (a int)', lpad(i::text, 3, '0'));
+  END LOOP;
+END
+$$;
+
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+
+DO $$
+BEGIN
+  FOR i IN 1..98 LOOP
+    EXECUTE format('select * from t%s', lpad(i::text, 3, '0'));
+  END LOOP;
+END
+$$;
+
+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;
+SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' OR query LIKE '%t098%' ORDER BY query;
+
+DO $$
+BEGIN
+  FOR i IN 2..98 LOOP
+    EXECUTE format('select * from t%s', lpad(i::text, 3, '0'));
+  END LOOP;
+END
+$$;
+
+DO $$
+BEGIN
+  FOR i IN 99..100 LOOP
+    EXECUTE format('select * from t%s', lpad(i::text, 3, '0'));
+  END LOOP;
+END
+$$;
+
+SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;
+-- record for t001 has been kicked out
+SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' ORDER BY query;
-- 
2.43.0

v2-0003-pg_stat_statements-Add-TAP-test-for-testing-resta.patchtext/plain; charset=UTF-8; name=v2-0003-pg_stat_statements-Add-TAP-test-for-testing-resta.patchDownload
From 3129dd32ff62350acbd179eea3683d27bea6adc7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 27 Dec 2023 13:08:57 +0100
Subject: [PATCH v2 3/3] pg_stat_statements: Add TAP test for testing restarts

This tests that pg_stat_statement contents are successfully kept
across restart.  (This similar to
src/test/recovery/t/029_stats_restart.pl for the stats collector.)

Discussion: https://www.postgresql.org/message-id/flat/40d1e4f2-835f-448f-a541-8ff5db75bf3d@eisentraut.org
---
 contrib/pg_stat_statements/Makefile         |  2 +
 contrib/pg_stat_statements/meson.build      |  5 ++
 contrib/pg_stat_statements/t/010_restart.pl | 53 +++++++++++++++++++++
 3 files changed, 60 insertions(+)
 create mode 100644 contrib/pg_stat_statements/t/010_restart.pl

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 7ee16e8350..20834bb0ee 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -24,6 +24,8 @@ REGRESS = select dml cursors utility level_tracking planning \
 # which typical installcheck users do not have (e.g. buildfarm clients).
 NO_INSTALLCHECK = 1
 
+TAP_TESTS = 1
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index a66acaa5b8..3e42328f6c 100644
--- a/contrib/pg_stat_statements/meson.build
+++ b/contrib/pg_stat_statements/meson.build
@@ -60,4 +60,9 @@ tests += {
     # runningcheck users do not have (e.g. buildfarm clients).
     'runningcheck': false,
   },
+  'tap': {
+    'tests': [
+      't/010_restart.pl',
+    ],
+  },
 }
diff --git a/contrib/pg_stat_statements/t/010_restart.pl b/contrib/pg_stat_statements/t/010_restart.pl
new file mode 100644
index 0000000000..83a2bf0db8
--- /dev/null
+++ b/contrib/pg_stat_statements/t/010_restart.pl
@@ -0,0 +1,53 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Tests for checking that pg_stat_statements contents are preserved
+# across restarts.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+	"shared_preload_libraries = 'pg_stat_statements'");
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION pg_stat_statements');
+
+$node->safe_psql('postgres', 'CREATE TABLE t1 (a int)');
+$node->safe_psql('postgres', 'SELECT a FROM t1');
+
+is( $node->safe_psql(
+		'postgres',
+		"SELECT query FROM pg_stat_statements WHERE query NOT LIKE '%pg_stat_statements%' ORDER BY query"
+	),
+	"CREATE TABLE t1 (a int)\nSELECT a FROM t1",
+	'pg_stat_statements populated');
+
+$node->restart;
+
+is( $node->safe_psql(
+		'postgres',
+		"SELECT query FROM pg_stat_statements WHERE query NOT LIKE '%pg_stat_statements%' ORDER BY query"
+	),
+	"CREATE TABLE t1 (a int)\nSELECT a FROM t1",
+	'pg_stat_statements data kept across restart');
+
+$node->append_conf('postgresql.conf', "pg_stat_statements.save = false");
+$node->reload;
+
+$node->restart;
+
+is( $node->safe_psql(
+		'postgres',
+		"SELECT count(*) FROM pg_stat_statements WHERE query NOT LIKE '%pg_stat_statements%'"
+	),
+	'0',
+	'pg_stat_statements data not kept across restart with .save=false');
+
+$node->stop;
+
+done_testing();
-- 
2.43.0

#6Julien Rouhaud
rjuju123@gmail.com
In reply to: Peter Eisentraut (#5)
Re: pg_stat_statements: more test coverage

On Wed, Dec 27, 2023 at 8:53 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 27.12.23 09:08, Julien Rouhaud wrote:

I was a bit surprised by that so I checked locally. It does work as
expected provided that you set pg_stat_statements.track to all:

Ok, here is an updated patch set that does it that way.

It looks good to me. One minor complaint, I'm a bit dubious about
those queries:

SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;

Is it to actually test that pg_stat_statements won't store more than
pg_stat_statements.max records? Note also that this query can't
return 0 rows, as the currently executed query will have an entry
added during post_parse_analyze. Maybe a comment saying what this is
actually testing would help.

It would also be good to test that pg_stat_statements_info.dealloc is
more than 0 once enough statements have been issued.

I have committed the patches 0002 and 0003 from the previous patch set
already.

I have also enhanced the TAP test a bit to check the actual content of
the output across restarts.

Nothing much to say about this one, it all looks good.

I'm not too hung up on the 0001 patch if others don't like that approach.

I agree with Michael on this one, the only times I saw this pattern
was to comply with some company internal policy for minimal coverage
numbers.

#7Peter Eisentraut
peter@eisentraut.org
In reply to: Julien Rouhaud (#6)
Re: pg_stat_statements: more test coverage

On 29.12.23 06:14, Julien Rouhaud wrote:

It looks good to me. One minor complaint, I'm a bit dubious about
those queries:

SELECT count(*) <= 100 AND count(*) > 0 FROM pg_stat_statements;

Is it to actually test that pg_stat_statements won't store more than
pg_stat_statements.max records? Note also that this query can't
return 0 rows, as the currently executed query will have an entry
added during post_parse_analyze. Maybe a comment saying what this is
actually testing would help.

Yeah, I think I added that query before I added the queries to check the
contents of pg_stat_statements.query itself, so it's a bit obsolete. I
reworked that part.

It would also be good to test that pg_stat_statements_info.dealloc is
more than 0 once enough statements have been issued.

I added that.

I have committed the patches 0002 and 0003 from the previous patch set
already.

I have also enhanced the TAP test a bit to check the actual content of
the output across restarts.

Nothing much to say about this one, it all looks good.

Ok, I have committed these two patches.

I'm not too hung up on the 0001 patch if others don't like that approach.

I agree with Michael on this one, the only times I saw this pattern
was to comply with some company internal policy for minimal coverage
numbers.

Ok, skipped that.

#8Michael Paquier
michael@paquier.xyz
In reply to: Peter Eisentraut (#7)
Re: pg_stat_statements: more test coverage

On Sat, Dec 30, 2023 at 08:39:47PM +0100, Peter Eisentraut wrote:

On 29.12.23 06:14, Julien Rouhaud wrote:

I agree with Michael on this one, the only times I saw this pattern
was to comply with some company internal policy for minimal coverage
numbers.

Ok, skipped that.

Just to close the loop here. I thought that I had sent a patch on the
lists that made use of these markers, but it looks like that's not the
case. The only thread I've found is this one:
/messages/by-id/d8f6bdd536df403b9b33816e9f7e0b9d@G08CNEXMBPEKD05.g08.fujitsu.local

(FWIW, I'm still skeptic about the idea of painting more backend code
with these outside the parsing areas, but I'm OK to be outnumbered.)
--
Michael

#9Michael Paquier
michael@paquier.xyz
In reply to: Peter Eisentraut (#7)
Re: pg_stat_statements: more test coverage

On Sat, Dec 30, 2023 at 08:39:47PM +0100, Peter Eisentraut wrote:

Ok, I have committed these two patches.

Please note that the buildfarm has turned red, as in:
https://buildfarm.postgresql.org/cgi-bin/show_stagxe_log.pl?nm=pipit&amp;dt=2023-12-31%2001%3A12%3A22&amp;stg=misc-check

pg_stat_statements's regression.diffs holds more details:
SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' OR query LIKE '%t098%' ORDER BY query;
         query
 --------------------
- select * from t001
  select * from t098
-(2 rows)
+(1 row)  
--
Michael
#10Julien Rouhaud
rjuju123@gmail.com
In reply to: Michael Paquier (#9)
Re: pg_stat_statements: more test coverage

On Sun, Dec 31, 2023 at 2:28 PM Michael Paquier <michael@paquier.xyz> wrote:

On Sat, Dec 30, 2023 at 08:39:47PM +0100, Peter Eisentraut wrote:

Ok, I have committed these two patches.

Please note that the buildfarm has turned red, as in:
https://buildfarm.postgresql.org/cgi-bin/show_stagxe_log.pl?nm=pipit&amp;dt=2023-12-31%2001%3A12%3A22&amp;stg=misc-check

pg_stat_statements's regression.diffs holds more details:
SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' OR query LIKE '%t098%' ORDER BY query;
query
--------------------
- select * from t001
select * from t098
-(2 rows)
+(1 row)

That's surprising. I wanted to see if there was any specific
configuration but I get a 403. I'm wondering if this is only due to
other tests being run concurrently evicting an entry earlier than
planned.

#11Peter Eisentraut
peter@eisentraut.org
In reply to: Julien Rouhaud (#10)
Re: pg_stat_statements: more test coverage

On 31.12.23 10:26, Julien Rouhaud wrote:

On Sun, Dec 31, 2023 at 2:28 PM Michael Paquier <michael@paquier.xyz> wrote:

On Sat, Dec 30, 2023 at 08:39:47PM +0100, Peter Eisentraut wrote:

Ok, I have committed these two patches.

Please note that the buildfarm has turned red, as in:
https://buildfarm.postgresql.org/cgi-bin/show_stagxe_log.pl?nm=pipit&amp;dt=2023-12-31%2001%3A12%3A22&amp;stg=misc-check

pg_stat_statements's regression.diffs holds more details:
SELECT query FROM pg_stat_statements WHERE query LIKE '%t001%' OR query LIKE '%t098%' ORDER BY query;
query
--------------------
- select * from t001
select * from t098
-(2 rows)
+(1 row)

That's surprising. I wanted to see if there was any specific
configuration but I get a 403. I'm wondering if this is only due to
other tests being run concurrently evicting an entry earlier than
planned.

These tests are run in a separate instance and serially, so I don't
think concurrency is an issue.

It looks like the failing configurations are exactly all the big-endian
ones: s390x, sparc, powerpc. So it's possible that this is actually a
bug? But unless someone can reproduce this locally and debug it, we
should probably revert this for now.

#12Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#11)
Re: pg_stat_statements: more test coverage

Peter Eisentraut <peter@eisentraut.org> writes:

It looks like the failing configurations are exactly all the big-endian
ones: s390x, sparc, powerpc. So it's possible that this is actually a
bug? But unless someone can reproduce this locally and debug it, we
should probably revert this for now.

I see it failed on my animal mamba, so I should be able to reproduce
it there. Will look.

regards, tom lane

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#11)
1 attachment(s)
Re: pg_stat_statements: more test coverage

Peter Eisentraut <peter@eisentraut.org> writes:

It looks like the failing configurations are exactly all the big-endian
ones: s390x, sparc, powerpc. So it's possible that this is actually a
bug? But unless someone can reproduce this locally and debug it, we
should probably revert this for now.

The reason for the platform-dependent behavior is that we're dealing
with a bunch of entries with identical "usage", so it's just about
random which ones actually get deleted. I do not think our qsort()
has platform-dependent behavior --- but the initial contents of
entry_dealloc's array are filled in hash_seq_search order, and that
*is* platform-dependent.

Now, the test script realizes that hazard. The bug seems to be that
it's wrong about how many competing usage-count-1 entries there are.
Instrumenting the calls to entry_alloc (which'll call entry_dealloc
when we hit 100 entries), I see

2023-12-31 13:21:39.160 EST client backend[3764867] pg_regress/max LOG: calling entry_alloc for 'SELECT pg_stat_statements_reset() IS NOT', cur hash size 0
2023-12-31 13:21:39.160 EST client backend[3764867] pg_regress/max LOG: calling entry_alloc for '$1', cur hash size 1
2023-12-31 13:21:39.160 EST client backend[3764867] pg_regress/max CONTEXT: SQL expression "1"
PL/pgSQL function inline_code_block line 3 at FOR with integer loop variable
2023-12-31 13:21:39.160 EST client backend[3764867] pg_regress/max LOG: calling entry_alloc for 'format($3, lpad(i::text, $4, $5))', cur hash size 2
2023-12-31 13:21:39.160 EST client backend[3764867] pg_regress/max CONTEXT: SQL expression "format('select * from t%s', lpad(i::text, 3, '0'))"
PL/pgSQL function inline_code_block line 4 at EXECUTE
2023-12-31 13:21:39.160 EST client backend[3764867] pg_regress/max LOG: calling entry_alloc for 'select * from t001', cur hash size 3
2023-12-31 13:21:39.160 EST client backend[3764867] pg_regress/max CONTEXT: SQL statement "select * from t001"
PL/pgSQL function inline_code_block line 4 at EXECUTE
...
2023-12-31 13:21:39.165 EST client backend[3764867] pg_regress/max LOG: calling entry_alloc for 'select * from t098', cur hash size 100
2023-12-31 13:21:39.165 EST client backend[3764867] pg_regress/max CONTEXT: SQL statement "select * from t098"
PL/pgSQL function inline_code_block line 4 at EXECUTE
2023-12-31 13:21:39.165 EST client backend[3764867] pg_regress/max LOG: entry_dealloc: zapping 10 of 100 victims

So the dealloc happens sooner than the script expects, and it's pure
chance that the test appeared to work anyway.

The test case needs to be rewritten to allow for more competing
usage-count-1 entries than it currently does. Backing "98" down to
"95" might be enough, but I've not experimented (and I'd recommend
leaving more than the minimum amount of headroom, in case plpgsql
changes behavior about how many subexpressions get put into the
table).

Strongly recommend that while fixing the test, you stick in some
debugging elogs to verify when the dealloc calls actually happen
rather than assuming you predicted it correctly. I did it as
attached.

regards, tom lane

Attachments:

hack-instrument-entry-alloc-and-dealloc.patchtext/x-diff; charset=us-ascii; name=hack-instrument-entry-alloc-and-dealloc.patchDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6f62a531f7..ffdc14a1eb 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1373,6 +1373,10 @@ pgss_store(const char *query, uint64 queryId,
 		if (!stored)
 			goto done;
 
+		elog(LOG, "calling entry_alloc for '%.40s', cur hash size %ld",
+			 norm_query ? norm_query : query, 
+			 hash_get_num_entries(pgss_hash));
+
 		/* OK to create a new hashtable entry */
 		entry = entry_alloc(&key, query_offset, query_len, encoding,
 							jstate != NULL);
@@ -2160,6 +2164,8 @@ entry_dealloc(void)
 	nvictims = Max(10, i * USAGE_DEALLOC_PERCENT / 100);
 	nvictims = Min(nvictims, i);
 
+	elog(LOG, "entry_dealloc: zapping %d of %d victims", nvictims, i);
+
 	for (i = 0; i < nvictims; i++)
 	{
 		hash_search(pgss_hash, &entries[i]->key, HASH_REMOVE, NULL);