On 07.01.2025 22:29, Sami Imseih wrote:
You are right. This is absolutely unexpected for users. Thank you for
the review.
To fix this, I suggest storing a random number in the [0, 1) range in a
separate variable, which will be used for comparisons in any place. We
will compare 'sample_rate' and random value not only in
pgss_post_parse_analyze(), but also in pgss_ProcessUtility() and
pgss_planner().
I attached patch with test and fixes.
I still think this is not the correct approach. It seems that post_parse_analyze
should not have to deal with checking for a sample rate. This is because
post_parse_analyze, which is the only hook with access to jstate, is
only responsible
for storing a normalized query text on disk and creating a not-yet
user visible entry
in the hash. i.e. pgss_store will never increment counters when called from
pgss_post_parse_analyze.
/* Increment the counts, except when jstate is not NULL */
if (!jstate)
{
What I think may be a better idea is to add something similar
to auto_explain.c, but it should only be added to the top of
pgss_planner, pgss_ExecutorStart and pgss_ProcessUtility.
if (nesting_level == 0)
{
if (!IsParallelWorker())
current_query_sampled = pg_prng_double(&pg_global_prng_state) <
pgss_sample_rate;
else
current_query_sampled = false;
}
This is needed for ExecutorStart and not ExecutorEnd because
ExecutorStart is where the instrumentation is initialized with
queryDesc->totaltime. The above code block could be
turned into a macro and reused in the routines mentioned.
I added this code in to the top of pgss_planner, pgss_ExecutorStart and
pgss_ProcessUtility. I removed this checking from
pgss_post_parse_analyze. I attached v9-patch.
However, it seems with this change, we can see a much
higher likelihood of non-normalized query texts being stored.
This is because jstate is only available during post_parse_analyze.
Therefore, if the first time you are sampling the statement is in ExecutorEnd,
then you will end up storing a non-normalized version of the query text,
see below example with the attached v8.
postgres=# set pg_stat_statements.sample_rate = 0;
SET
postgres=# select pg_stat_statements_reset();
pg_stat_statements_reset
-------------------------------
2025-01-07 13:02:11.807952-06
(1 row)
postgres=# SELECT 1 \parse stmt
postgres=# \bind_named stmt \g
?column?
----------
1
(1 row)
postgres=# \bind_named stmt \g
?column?
----------
1
(1 row)
postgres=# SELECT query, calls FROM pg_stat_statements;
query | calls
-------+-------
(0 rows)
postgres=# set pg_stat_statements.sample_rate = 1;
SET
postgres=# \bind_named stmt \g
?column?
----------
1
(1 row)
postgres=# \bind_named stmt \g
?column?
----------
1
(1 row)
postgres=# SELECT query, calls FROM pg_stat_statements;
query | calls
-------+-------
(0 rows)
postgres=# \bind_named stmt \g
?column?
----------
1
(1 row)
postgres=# SELECT query, calls FROM pg_stat_statements;
query | calls
---------------------------------------------+-------
SELECT 1 | 1
SELECT query, calls FROM pg_stat_statements | 1
(2 rows)
postgres=# \bind_named stmt \g
?column?
----------
1
(1 row)
postgres=# SELECT query, calls FROM pg_stat_statements;
query | calls
---------------------------------------------+-------
SELECT 1 | 2
SELECT query, calls FROM pg_stat_statements | 2
(2 rows)
One idea is to make jstate available to all hooks, and completely
remove reliance on post_parse_analyze in pg_stat_statements.
I can't find the thread now, but I know this has come up in past discussions
when troubleshooting gaps in query normalization. My concern is this
feature will greatly increase the likelihood of non-normalized query texts.
Also, with regards to the benchmarks, it seems
sampling will be beneficial for workloads that are touching a small number
of entries with high concurrency. This is why we see benefit for
a standard pgbench workload.
Samping becomes less beneficial when there is a large set of queries
being updated.
I still think this is a good approach for workloads that touch a small set
of entries.
Regards,
Sami
After the changes in v9-patch, won’t all the necessary queries now be
normalized? Since there are no longer any restrictions in the parser
hook, queries will be normalized as usual, and pgss_planner,
pgss_ExecutorStart, and ExecutorEnd will simply fetch them from
'pgss_query_texts.stat' file.
For now, this seems like the simplest approach instead of making
JumbleState available to other hooks. Moreover, if we do proceed with
that, we might be able to remove the 'pgss_query_texts.stat' file
altogether and more other improvements. In my view, if no other options
arise, we should address making JumbleState available to other hooks in
a separate thread. If you have any suggestions, I'm always open to feedback.
I attached v9 patch with changes and test above.
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
From b40ab06155cac38e57f0551a3858d0574d96865f Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov <ilya.evdoki...@tantorlabs.com>
Date: Wed, 8 Jan 2025 22:15:02 +0300
Subject: [PATCH] Allow setting sample ratio for pg_stat_statements
New configuration parameter pg_stat_statements.sample_ratio makes it
possible to track just a fraction of the queries meeting the configured
threshold, to reduce the amount of tracking.
---
.../pg_stat_statements/expected/select.out | 76 +++++++++++++++++++
.../pg_stat_statements/pg_stat_statements.c | 54 ++++++++++++-
contrib/pg_stat_statements/sql/select.sql | 21 +++++
doc/src/sgml/pgstatstatements.sgml | 18 +++++
4 files changed, 166 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_stat_statements/expected/select.out b/contrib/pg_stat_statements/expected/select.out
index 37a30af034..558d93fb46 100644
--- a/contrib/pg_stat_statements/expected/select.out
+++ b/contrib/pg_stat_statements/expected/select.out
@@ -153,6 +153,82 @@ SELECT pg_stat_statements_reset() IS NOT NULL AS t;
t
(1 row)
+--
+-- sample statements
+--
+SET pg_stat_statements.sample_rate = 0.0;
+SELECT 1 AS "int";
+ int
+-----
+ 1
+(1 row)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(1 row)
+
+SET pg_stat_statements.sample_rate = 1.0;
+SELECT 1 AS "int";
+ int
+-----
+ 1
+(1 row)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT $1 AS "int" | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+set pg_stat_statements.sample_rate = 0.0;
+select pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT 1 \parse stmt
+\bind_named stmt \g
+ ?column?
+----------
+ 1
+(1 row)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+-------+-------
+(0 rows)
+
+set pg_stat_statements.sample_rate = 1.0;
+\bind_named stmt \g
+ ?column?
+----------
+ 1
+(1 row)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT $1 | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(2 rows)
+
+select pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
--
-- queries with locking clauses
--
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index bebf8134eb..52957c4628 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -50,6 +50,7 @@
#include "access/parallel.h"
#include "catalog/pg_authid.h"
#include "common/int.h"
+#include "common/pg_prng.h"
#include "executor/instrument.h"
#include "funcapi.h"
#include "jit/jit.h"
@@ -294,6 +295,10 @@ static bool pgss_track_utility = true; /* whether to track utility commands */
static bool pgss_track_planning = false; /* whether to track planning
* duration */
static bool pgss_save = true; /* whether to save stats across shutdown */
+static double pgss_sample_rate = 1;
+
+/* Is the current top-level query to be sampled? */
+static bool current_query_sampled = false;
#define pgss_enabled(level) \
@@ -414,6 +419,19 @@ _PG_init(void)
NULL,
NULL);
+ DefineCustomRealVariable("pg_stat_statements.sample_rate",
+ "Fraction of queries to track.",
+ NULL,
+ &pgss_sample_rate,
+ 1.0,
+ 0.0,
+ 1.0,
+ PGC_SUSET,
+ 0,
+ NULL,
+ NULL,
+ NULL);
+
DefineCustomEnumVariable("pg_stat_statements.track",
"Selects which statements are tracked by pg_stat_statements.",
NULL,
@@ -889,12 +907,21 @@ pgss_planner(Query *parse,
{
PlannedStmt *result;
+ if (nesting_level == 0)
+ {
+ if (!IsParallelWorker())
+ current_query_sampled = pg_prng_double(&pg_global_prng_state) < pgss_sample_rate;
+ else
+ current_query_sampled = false;
+
+ }
+
/*
* We can't process the query if no query_string is provided, as
* pgss_store needs it. We also ignore query without queryid, as it would
* be treated as a utility statement, which may not be the case.
*/
- if (pgss_enabled(nesting_level)
+ if (pgss_enabled(nesting_level) && current_query_sampled
&& pgss_track_planning && query_string
&& parse->queryId != UINT64CONST(0))
{
@@ -994,12 +1021,22 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
else
standard_ExecutorStart(queryDesc, eflags);
+ if (nesting_level == 0)
+ {
+ if (!IsParallelWorker())
+ current_query_sampled = pg_prng_double(&pg_global_prng_state) < pgss_sample_rate;
+ else
+ current_query_sampled = false;
+
+ }
+
/*
* If query has queryId zero, don't track it. This prevents double
* counting of optimizable statements that are directly contained in
* utility statements.
*/
- if (pgss_enabled(nesting_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
+ if (pgss_enabled(nesting_level) && current_query_sampled &&
+ queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
@@ -1111,7 +1148,18 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
uint64 saved_queryId = pstmt->queryId;
int saved_stmt_location = pstmt->stmt_location;
int saved_stmt_len = pstmt->stmt_len;
- bool enabled = pgss_track_utility && pgss_enabled(nesting_level);
+ bool enabled;
+
+ if (nesting_level == 0)
+ {
+ if (!IsParallelWorker())
+ current_query_sampled = pg_prng_double(&pg_global_prng_state) < pgss_sample_rate;
+ else
+ current_query_sampled = false;
+ }
+
+ /* current_query_sampled is in pgss_enabled */
+ enabled = pgss_track_utility && pgss_enabled(nesting_level) && current_query_sampled;
/*
* Force utility statements to get queryId zero. We do this even in cases
diff --git a/contrib/pg_stat_statements/sql/select.sql b/contrib/pg_stat_statements/sql/select.sql
index e0be58d5e2..21f09ef94a 100644
--- a/contrib/pg_stat_statements/sql/select.sql
+++ b/contrib/pg_stat_statements/sql/select.sql
@@ -59,6 +59,27 @@ DEALLOCATE pgss_test;
SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+--
+-- sample statements
+--
+SET pg_stat_statements.sample_rate = 0.0;
+SELECT 1 AS "int";
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+SET pg_stat_statements.sample_rate = 1.0;
+SELECT 1 AS "int";
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+
+set pg_stat_statements.sample_rate = 0.0;
+select pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT 1 \parse stmt
+\bind_named stmt \g
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+set pg_stat_statements.sample_rate = 1.0;
+\bind_named stmt \g
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+select pg_stat_statements_reset() IS NOT NULL AS t;
+
--
-- queries with locking clauses
--
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 501b468e9a..8ade4e3ced 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -936,6 +936,24 @@
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>
+ <varname>pg_stat_statements.sample_rate</varname> (<type>real</type>)
+ <indexterm>
+ <primary><varname>pg_stat_statements.sample_rate</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+
+ <listitem>
+ <para>
+ <varname>pg_stat_statements.sample_rate</varname> causes pg_stat_statements to only
+ track a fraction of the statements in each session. The default is 1,
+ meaning track all the queries. In case of nested statements,
+ either all will be tracked or none. Only superusers can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term>
<varname>pg_stat_statements.save</varname> (<type>boolean</type>)
--
2.34.1
