Since at the moment these changes are not a priority, and it's more
important to address [0] for pg_stat_statements, I'm marking this patch
as "Returned with Feedback".
[0]: https://www.postgresql.org/message-id/Zz0MFPq1s4WFxxpL%40paquier.xyz
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
Hi,
I attached rebased patch v20.
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
From accee46b077e1debdc3db61555923b2c11e18d5e Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov
Date: Fri, 21 Mar 2025 17:37:08 +0300
Subject: [PATCH v20] Allow setting sample rate for pg_stat_statements
New config
On 20.02.2025 04:04, Sami Imseih wrote:
In my opinion, sample rate is a better fit for pg_stat_statements,
since the queries that
you care about the most are usually the most frequently executed. Sampling them
will still provide enough good data without the risk of not capturing
statistics about
On 20.02.2025 03:32, Sami Imseih wrote:
As you can see, the query has not been normalized. Is it a bug, expected or
undefined behavior?
No, this behavior is expected. The ability to normalize a statement is
only available
during post_parse_analyze (pgss_post_parse_analyze), as this is when
we
> But instead of blindly reducing the frequency via PRNG, we can take a more
> thoughtful approach with threshold by execute time:
> Find the most frequent query by column 'calls' in pg_stat_statements;
> In this query look at info about execution time: min_exec_time,
> max_exec_time, etc;
> Gra
> As you can see, the query has not been normalized. Is it a bug, expected or
> undefined behavior?
No, this behavior is expected. The ability to normalize a statement is
only available
during post_parse_analyze (pgss_post_parse_analyze), as this is when
we have access to
JumbleState.
In your ex
On 19.02.2025 22:11, Sami Imseih wrote:
I don't see v18 attached.
Ah, sorry, I attached v18.
But, I think it's wrong that you changed the design of this patch
(sample rate to duration based ) after it was ready for committer.
I've decided to reconsider how to better reduce the load on
I don't see v18 attached.
But, I think it's wrong that you changed the design of this patch
(sample rate to duration based ) after it was ready for committer.
This CF [0] should go back to "Needs Review" if this is the case.
--
Sami
[0] https://commitfest.postgresql.org/patch/5390/
On 14.02.2025 16:17, Ilia Evdokimov wrote:
Hi hackers,
I've decided to explore a slightly different approach to reducing
spinlock contention—by introducing a simple execution time threshold.
If a query’s execution time exceeds this threshold, it is recorded in
pg_stat_statements; otherwise,
On 14.02.2025 16:17, Ilia Evdokimov wrote:
Hi hackers,
I've decided to explore a slightly different approach to reducing
spinlock contention—by introducing a simple execution time threshold.
If a query’s execution time exceeds this threshold, it is recorded in
pg_stat_statements; otherwise,
Hi hackers,
I've decided to explore a slightly different approach to reducing
spinlock contention—by introducing a simple execution time threshold. If
a query’s execution time exceeds this threshold, it is recorded in
pg_stat_statements; otherwise, it is ignored. As Alexander [0] pointed
out,
Hi hackers,
Since current patch is in the commitfest with the status 'Ready for
committer', I’d like to summarize what it does, the problems it
addresses, and answer the key questions raised in the discussion thread.
Enabling pg_stat_statements can cause a performance drop due to two main
re
On 04.02.2025 20:59, Sami Imseih wrote:
To summarize the results of all benchmarks, I compiled them into a table:
Thanks for compiling the benchmark data above.
The main benefit of this patch will be to give the user
a toggle if they are observing high spinlock contention
due to pg_stat_state
> To summarize the results of all benchmarks, I compiled them into a table:
Thanks for compiling the benchmark data above.
The main benefit of this patch will be to give the user
a toggle if they are observing high spinlock contention
due to pg_stat_statements which will likely occur
on larger mac
On 29.01.2025 21:52, Ilia Evdokimov wrote:
... I also attached the benchmark.sh
script used to generate the output.
In my opinion, if we can't observe bottleneck of spinlock on 32 CPUs,
we should determine the CPU count at which it becomes. This will help
us understand the scale of the p
> and we've been trying to solve a non-existent problem?
Anecdotally speaking, Most users will encounter bottlenecks
due to exclusive locks on pgss hash ( dealloc and garbage collection )
more often than they will on the spinlock. The spinlock will likely
become more of an issue when you are not s
On 28.01.2025 23:50, Ilia Evdokimov wrote:
If anyone has the capability to run this benchmark on machines with
more
CPUs or with different queries, it would be nice. I’d appreciate any
suggestions or feedback.
I wanted to share some additional benchmarks I ran as well
on a r8g.48xlarge (
On 28.01.2025 20:21, Sami Imseih wrote:
All the changes mentioned above are included in the v13 patch. Since the
patch status is 'Ready for Committer,' I believe it is now better for
upstream inclusion, with improved details in tests and documentation. Do
you have any further suggestions?
I am
> All the changes mentioned above are included in the v13 patch. Since the
> patch status is 'Ready for Committer,' I believe it is now better for
> upstream inclusion, with improved details in tests and documentation. Do
> you have any further suggestions?
I am not quite clear on the sample_1.out
On 28.01.2025 02:00, Sami Imseih wrote:
Hi,
I have a few comments about the patch.
1/
tests are missing for pg_stat_statements.track=all and with a sample
rate < 1 applied.
Can we add tests for this case?
I think I’ve come up with a good idea for implementing this. We can
create a new file
Hi,
I have a few comments about the patch.
1/
tests are missing for pg_stat_statements.track=all and with a sample
rate < 1 applied.
Can we add tests for this case?
2/
This declaration:
+static bool current_query_sampled = false;
should be moved near the declaration of nesting_level,
the othe
On 23.01.2025 08:21, Andrey Borodin wrote:
There’s a typo in the commit message (ratio instead of rate). Besides this the
patch looks ready for committer.
Best regards, Andrey Borodin.
Fixed. Thank you for review!
I noticed that the code has not enough comments, so I added additional
o
>
There’s a typo in the commit message (ratio instead of rate). Besides this the
patch looks ready for committer.
Best regards, Andrey Borodin.
On 20.01.2025 17:20, Ilia Evdokimov wrote:
On 15.01.2025 20:16, Sami Imseih wrote:
Probably, but first I suggest benchmarking with sampling applied to
all queries. If the results are good, we can later filter certain
queries based on different characteristics.
Absolutely. The benchmark numbe
On 15.01.2025 20:16, Sami Imseih wrote:
Probably, but first I suggest benchmarking with sampling applied to all
queries. If the results are good, we can later filter certain queries based on
different characteristics.
Absolutely. The benchmark numbers to justify this feature are
the next step
On 15.01.2025 12:47, Ilia Evdokimov wrote:
On 06.01.2025 18:57, Andrey M. Borodin wrote:
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_rat
> Probably, but first I suggest benchmarking with sampling applied to all
> queries. If the results are good, we can later filter certain queries based
> on different characteristics.
Absolutely. The benchmark numbers to justify this feature are
the next step. Thanks for your work on this!
Rega
On 15.01.2025 01:07, Sami Imseih wrote:
Alena, Sami – I apologize for not including you in the previous email.
If you're interested in this approach, I'm open to any suggestions.
[0]:
https://www.postgresql.org/message-id/1b13d748-5e98-479c-9222-3253a734a038%40tantorlabs.com
Here are my though
> Alena, Sami – I apologize for not including you in the previous email.
> If you're interested in this approach, I'm open to any suggestions.
>
> [0]:
> https://www.postgresql.org/message-id/1b13d748-5e98-479c-9222-3253a734a038%40tantorlabs.com
Here are my thoughts on this:
There is good reason
On 09.01.2025 22:13, Alena Rybakina wrote:
Hi! Thank you for the work with this subject.
I looked at your patch and noticed that this part of the code is
repeated several times:
if (nesting_level == 0)
{
if (!IsParallelWorker())
current_query_sampled =
pg_prng_double
On 22.11.2024 09:08, Alexander Korotkov wrote:
On Wed, Nov 20, 2024 at 12:07 AM Michael Paquier wrote:
On Tue, Nov 19, 2024 at 09:39:21AM -0500, Greg Sabino Mullane wrote:
Oh, and a +1 in general to the patch, OP, although it would also be nice to
start finding the bottlenecks that cause suc
On 09.01.2025 22:13, Alena Rybakina wrote:
Hi! Thank you for the work with this subject.
I looked at your patch and noticed that this part of the code is
repeated several times:
if (nesting_level == 0)
{
if (!IsParallelWorker())
current_query_sampled =
pg_prng_doubl
Hi! Thank you for the work with this subject.
I looked at your patch and noticed that this part of the code is
repeated several times:
if (nesting_level == 0)
{
if (!IsParallelWorker())
current_query_sampled =
pg_prng_double(&pg_global_prng_state) < pgss_sample_rate;
On 09.01.2025 05:29, Sami Imseih wrote:
Unfortunately, these changes do not achieve the intended sampling goal.
I looked into this more deeply: while the sampled-out queries do not
appear in pg_stat_statements, an entry is still allocated in the hash
table after normalization, which, in my view
> Unfortunately, these changes do not achieve the intended sampling goal.
> I looked into this more deeply: while the sampled-out queries do not
> appear in pg_stat_statements, an entry is still allocated in the hash
> table after normalization, which, in my view, should not happen when
> sampling
On 08.01.2025 22:39, Ilia Evdokimov wrote:
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
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
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' an
>> 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
> On 7 Jan 2025, at 16:05, Ilia Evdokimov wrote:
>
>> 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_xac
I completely forgot about ordering pg_stat_statements in the test, which
is why the test failed in [0]. I've added ORDER BY query COLLATE "C" to
avoid any non-deterministic ordering in the table.
[0]: https://cirrus-ci.com/task/5724458477944832
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
On 06.01.2025 18:57, Andrey M. Borodin wrote:
On 6 Jan 2025, at 15:50, Ilia Evdokimov 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 a
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 whi
> On 6 Jan 2025, at 15:50, Ilia Evdokimov 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.
1. This code seems a little different from your pat
On 10.12.2024 17:35, Ilia Evdokimov wrote:
Hi everyone,
I attached previous sampling patch for pg_stat_statements (v4).
Suggestions like sampling based on execution time remain unfeasible,
as pg_stat_statements can track query during query planning, not
execution.
To evaluate the implement
Hi everyone,
I attached previous sampling patch for pg_stat_statements (v4).
Suggestions like sampling based on execution time remain unfeasible, as
pg_stat_statements can track query during query planning, not execution.
To evaluate the implementation, I ran a benchmark creating 1,000 random
On 26.11.2024 01:15, Ilia Evdokimov wrote:
On 22.11.2024 09:08, Alexander Korotkov wrote:
On Wed, Nov 20, 2024 at 12:07 AM Michael Paquier wrote:
On Tue, Nov 19, 2024 at 09:39:21AM -0500, Greg Sabino Mullane wrote:
Oh, and a +1 in general to the patch, OP, although it would also be nice to
On 22.11.2024 09:08, Alexander Korotkov wrote:
On Wed, Nov 20, 2024 at 12:07 AM Michael Paquier wrote:
On Tue, Nov 19, 2024 at 09:39:21AM -0500, Greg Sabino Mullane wrote:
Oh, and a +1 in general to the patch, OP, although it would also be nice to
start finding the bottlenecks that cause such
On Wed, Nov 20, 2024 at 12:07 AM Michael Paquier wrote:
> On Tue, Nov 19, 2024 at 09:39:21AM -0500, Greg Sabino Mullane wrote:
> > Oh, and a +1 in general to the patch, OP, although it would also be nice to
> > start finding the bottlenecks that cause such performance issues.
>
> FWIW, I'm not eag
> On 19 Nov 2024, at 17:39, Greg Sabino Mullane wrote:
>
> I'm curious what "using it wrong" means exactly?
Here's an example. pgSCV is querying pgss for every database separately every
minute. It makes sense for the project. But when you have ~1000 databases, you
have a lot of traffic to p
On 20.11.2024 01:07, Michael Paquier wrote:
On Tue, Nov 19, 2024 at 09:39:21AM -0500, Greg Sabino Mullane wrote:
Oh, and a +1 in general to the patch, OP, although it would also be nice to
start finding the bottlenecks that cause such performance issues.
FWIW, I'm not eager to integrate this p
On Tue, Nov 19, 2024 at 5:07 PM Michael Paquier wrote:
> One piece of it would be to see how much of such "bottlenecks" we
> would be able to get rid of by integrating pg_stat_statements into
> the central pgstats with the custom APIs, without pushing the module
> into core.
Any particular reas
On Tue, Nov 19, 2024 at 7:12 AM Andrey M. Borodin
wrote:
> +1 for the idea. I heard a lot of complaints about that pgss is costly.
> Most of them were using it wrong though.
I'm curious what "using it wrong" means exactly?
Oh, and a +1 in general to the patch, OP, although it would also be nic
On Tue, Nov 19, 2024 at 09:39:21AM -0500, Greg Sabino Mullane wrote:
> Oh, and a +1 in general to the patch, OP, although it would also be nice to
> start finding the bottlenecks that cause such performance issues.
FWIW, I'm not eager to integrate this proposal without looking at this
exact argume
On 19.11.2024 15:11, Andrey M. Borodin wrote:
On 18 Nov 2024, at 23:33, Ilia Evdokimov wrote:
Hi hackers,
Under high-load scenarios with a significant number of transactions per second,
pg_stat_statements introduces substantial overhead due to the collection and
storage of statistics. Cur
> On 18 Nov 2024, at 23:33, Ilia Evdokimov
> wrote:
>
> Hi hackers,
>
> Under high-load scenarios with a significant number of transactions per
> second, pg_stat_statements introduces substantial overhead due to the
> collection and storage of statistics. Currently, we are sometimes forced
Hi everyone,
I believe we should also include this check in the pgss_ExecutorEnd()
function because sampling in pgss_ExecutorEnd() ensures that a query not
initially sampled in pgss_ExecutorStart() can still be logged if it
meets the pg_stat_statements.sample_rate criteria. This approach adds
Hi hackers,
Under high-load scenarios with a significant number of transactions per
second, pg_stat_statements introduces substantial overhead due to the
collection and storage of statistics. Currently, we are sometimes forced
to disable pg_stat_statements or adjust the size of the statistics
58 matches
Mail list logo
