public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ilia Evdokimov <[email protected]>
To: Sami Imseih <[email protected]>
To: Andrey M. Borodin <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Greg Sabino Mullane <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Sample rate added to pg_stat_statements
Date: Wed, 8 Jan 2025 22:39:18 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAA5RZ0sBsajHdE_HPoJ-C5rp8QmfArd6a6yJo6bDMVpyW25E+Q@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CAKAnmmLD8C-Y6CVF74Ccup8h9YpzBYzSvWWRG_Qf8+5YEMh4-g@mail.gmail.com>
<[email protected]>
<CAPpHfdsTKAQqC3A48-MGQhrhfEamXZPb64w=utk7thQcOMNr7Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAA5RZ0s=JH4owVr=Tfr4DYpFnBrYJjKoq2d7NqdkBQKY5whiRQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAA5RZ0sBsajHdE_HPoJ-C5rp8QmfArd6a6yJo6bDMVpyW25E+Q@mail.gmail.com>
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.
Attachments:
[text/x-patch] v9-0001-Allow-setting-sample-ratio-for-pg_stat_statements.patch (8.7K, ../[email protected]/2-v9-0001-Allow-setting-sample-ratio-for-pg_stat_statements.patch)
download | inline diff:
From b40ab06155cac38e57f0551a3858d0574d96865f Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov <[email protected]>
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
view thread (58+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Sample rate added to pg_stat_statements
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox