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: Tue, 7 Jan 2025 14:05:54 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAA5RZ0s=JH4owVr=Tfr4DYpFnBrYJjKoq2d7NqdkBQKY5whiRQ@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>
On 06.01.2025 18:57, Andrey M. Borodin wrote:
>> On 6 Jan 2025, at 15:50, Ilia Evdokimov <[email protected]> wrote:
>>
>> Any suggestions for improvements?
> The patch looks good to me, just a few nits.
>
> 0. Perhaps, we could have a test for edge values of 0 and 1. I do not insist, just an idea to think about.
I would add tests, because they are never useless. I've added a simple
test which verifies hash table with queries after setting sample_rate =
0.0 and sample_rate = 1.0.
>
> 1. This code seems a little different from your patch. It is trying to avoid engaging PRNG. I'm not sure it's a good idea, but still. Also, it uses "<=", not "<".
>
> xact_is_sampled = log_xact_sample_rate != 0 &&
> (log_xact_sample_rate == 1 ||
> pg_prng_double(&pg_global_prng_state) <= log_xact_sample_rate);
>
> Thanks!
>
>
> Best regards, Andrey Borodin.
>
Are we sure we're discussing the same patch? Because these remarks refer
to the 5 version of the patch, which I abandoned due to your remarks.
On 06.01.2025 20:51, Sami Imseih wrote:
> Hi,
>
> I was looking at this patch, and I was specifically curious about
> how this works with prepared statements. The way the patch
> works now is that it determines if the statement is to be sampled
> at post_parse_analyze time which could lead to unexpected
> behavior.
>
> Let's take an example below in which the
> pg_stat_statements.sample_rate is set to 0 ( to mimic
> some sampling rate < 1 in which this query does not
> get sampled ). At that point, all subsequent executions
> of the statement will not get tracked at all. Is this
> what is expected for prepared statements? My concern
> is we will even lose more stats than what a user
> may expect.
>
> This of course will not be an issue for simple query.
>
> postgres=# set pg_stat_statements.sample_rate = 0;
> SET
> postgres=# select pg_stat_statements_reset();
> pg_stat_statements_reset
> -------------------------------
> 2025-01-06 11:45:23.484793-06
> (1 row)
>
> postgres=# SELECT $1 \parse stmt
>
> postgres=#
> postgres=# \bind_named stmt 1 \g
> ?column?
> ----------
> 1
> (1 row)
>
> postgres=# \bind_named stmt 1 \g
> ?column?
> ----------
> 1
> (1 row)
>
> postgres=# set pg_stat_statements.sample_rate = 1;
> SET
> postgres=# \bind_named stmt 1 \g
> ?column?
> ----------
> 1
> (1 row)
>
> postgres=# \bind_named stmt 1 \g
> ?column?
> ----------
> 1
> (1 row)
>
> postgres=# SELECT query, calls FROM pg_stat_statements;
> query | calls
> -------+-------
> (0 rows)
>
> Regards,
>
> Sami Imseih
> Amazon Web Services (AWS)
>
>
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.
If you have any objections or suggestions on how to improve it, I'm
always open to feedback.
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
Attachments:
[text/x-patch] v7-0001-Allow-setting-sample-ratio-for-pg_stat_statements.patch (6.9K, ../[email protected]/2-v7-0001-Allow-setting-sample-ratio-for-pg_stat_statements.patch)
download | inline diff:
From 388c12632610cc6abddd6fc750a1b15712d62e63 Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov <[email protected]>
Date: Tue, 7 Jan 2025 14:02:32 +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 | 37 ++++++++++++++++++
.../pg_stat_statements/pg_stat_statements.c | 38 ++++++++++++++++++-
contrib/pg_stat_statements/sql/select.sql | 11 ++++++
doc/src/sgml/pgstatstatements.sgml | 18 +++++++++
4 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/contrib/pg_stat_statements/expected/select.out b/contrib/pg_stat_statements/expected/select.out
index 37a30af034..724ec9c8ac 100644
--- a/contrib/pg_stat_statements/expected/select.out
+++ b/contrib/pg_stat_statements/expected/select.out
@@ -153,6 +153,43 @@ 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;
+ 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;
+ 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 | 0
+(3 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..4bab2a74ea 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"
@@ -255,6 +256,7 @@ typedef struct pgssSharedState
/* Current nesting depth of planner/ExecutorRun/ProcessUtility calls */
static int nesting_level = 0;
+static double random_sample = 0.0;
/* Saved hook values */
static shmem_request_hook_type prev_shmem_request_hook = NULL;
@@ -294,12 +296,17 @@ 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) \
(!IsParallelWorker() && \
(pgss_track == PGSS_TRACK_ALL || \
- (pgss_track == PGSS_TRACK_TOP && (level) == 0)))
+ (pgss_track == PGSS_TRACK_TOP && (level) == 0)) && \
+ current_query_sampled)
#define record_gc_qtexts() \
do { \
@@ -414,6 +421,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,
@@ -835,6 +855,11 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate)
if (prev_post_parse_analyze_hook)
prev_post_parse_analyze_hook(pstate, query, jstate);
+ random_sample = pg_prng_double(&pg_global_prng_state);
+
+ if (nesting_level == 0)
+ current_query_sampled = random_sample < pgss_sample_rate;
+
/* Safety check... */
if (!pgss || !pgss_hash || !pgss_enabled(nesting_level))
return;
@@ -889,6 +914,9 @@ pgss_planner(Query *parse,
{
PlannedStmt *result;
+ if (nesting_level == 0)
+ current_query_sampled = random_sample < pgss_sample_rate;
+
/*
* 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
@@ -1111,7 +1139,13 @@ 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)
+ current_query_sampled = random_sample < pgss_sample_rate;
+
+ /* current_query_sampled is in pgss_enabled */
+ enabled = pgss_track_utility && pgss_enabled(nesting_level);
/*
* 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..0a881c3cbf 100644
--- a/contrib/pg_stat_statements/sql/select.sql
+++ b/contrib/pg_stat_statements/sql/select.sql
@@ -59,6 +59,17 @@ 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;
+SET pg_stat_statements.sample_rate = 1.0;
+SELECT 1 AS "int";
+SELECT query, calls FROM pg_stat_statements;
+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