Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tI4GL-00Bt0h-10 for pgsql-hackers@arkaria.postgresql.org; Mon, 02 Dec 2024 11:06:45 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1tI4GI-00GH8D-Ih for pgsql-hackers@arkaria.postgresql.org; Mon, 02 Dec 2024 11:06:43 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tI4GI-00GH83-3o for pgsql-hackers@lists.postgresql.org; Mon, 02 Dec 2024 11:06:43 +0000 Received: from forward103b.mail.yandex.net ([2a02:6b8:c02:900:1:45:d181:d103]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tI4GE-000dYQ-Ma for pgsql-hackers@postgresql.org; Mon, 02 Dec 2024 11:06:42 +0000 Received: from mail-nwsmtp-smtp-production-main-60.sas.yp-c.yandex.net (mail-nwsmtp-smtp-production-main-60.sas.yp-c.yandex.net [IPv6:2a02:6b8:c11:1115:0:640:1385:0]) by forward103b.mail.yandex.net (Yandex) with ESMTPS id C1E3360B65; Mon, 2 Dec 2024 14:06:34 +0300 (MSK) Received: by mail-nwsmtp-smtp-production-main-60.sas.yp-c.yandex.net (smtp/Yandex) with ESMTPSA id U6cEdomOoiE0-mOCXyis8; Mon, 02 Dec 2024 14:06:33 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tantorlabs.com; s=mail; t=1733137594; bh=2imWfUKUeYMAasvv1uWn7Hhmy1OlvWHxMUTdEAKxwqU=; h=In-Reply-To:Cc:Date:References:To:Subject:Message-ID:From; b=ocEPLefESGa/+7nkq0NBgO0lckqblurb287GPDrfBXDMLDucNGTQsuwaA3jC9iq2B wIdce+GEBWAu4QMatHHUk0DEOiUVT2mPLz0SFvT/YmqEGTV3HMmb0TVm29t4i96sOK h97FuzCZnjMg+klDqR4tWmCF5MYIXNPSCC+rA6nE= Authentication-Results: mail-nwsmtp-smtp-production-main-60.sas.yp-c.yandex.net; dkim=pass header.i=@tantorlabs.com Content-Type: multipart/alternative; boundary="------------YxCFKBTrcciCSFlg7hz0acRS" Message-ID: Date: Mon, 2 Dec 2024 14:06:30 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: Sample rate added to pg_stat_statements From: Ilia Evdokimov To: Alexander Korotkov Cc: Greg Sabino Mullane , "Andrey M. Borodin" , pgsql-hackers , Michael Paquier References: <6707FCA9-FD1A-4609-A1A6-142456C14E0C@yandex-team.ru> <320a31e6-4c91-438b-ab17-8a1d72384727@tantorlabs.com> Content-Language: en-US In-Reply-To: <320a31e6-4c91-438b-ab17-8a1d72384727@tantorlabs.com> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------YxCFKBTrcciCSFlg7hz0acRS Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 >>>> start finding the bottlenecks that cause such performance issues. >>> FWIW, I'm not eager to integrate this proposal without looking at this >>> exact argument in depth. >>> >>> 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. This means that we would combine the existing hash of pgss >>> to shrink to 8 bytes for objid rather than 13 bytes now as the current >>> code relies on (toplevel, userid, queryid) for the entry lookup (entry >>> removal is sniped with these three values as well, or dshash seq >>> scans). The odds of conflicts still still play in our favor even if >>> we have a few million entries, or even ten times that. >> If you run "pgbench -S -M prepared" on a pretty large machine with >> high concurrency, then spin lock in pgss_store() could become pretty >> much of a bottleneck. And I'm not sure switching all counters to >> atomics could somehow improve the situation given we already have >> pretty many counters. >> >> I'm generally +1 for the approach taken in this thread. But I would >> suggest introducing a threshold value for a query execution time, and >> sample just everything below that threshold. The slower query >> shouldn't be sampled, because it can't be too frequent, and also it >> could be more valuable to be counter individually (while very fast >> queries probably only matter "in average"). >> >> ------ >> Regards, >> Alexander Korotkov >> Supabase > > I really liked your idea, and I’d like to propose an enhancement that > I believe improves it further. > > Yes, if a query’s execution time exceeds the threshold, it should > always be tracked without sampling. However, for queries with > execution times below the threshold, the sampling logic should > prioritize shorter queries over those closer to the threshold. In my > view, the ideal approach is for shorter queries to have the highest > probability of being sampled, while queries closer to the threshold > are less likely to be sampled. > > This behavior can be achieved with the following logic: > > pg_stat_statements.sample_exectime_threshold * random(0, 1) < total_time > > Here’s how it works: > > * As a query’s execution time approaches zero, the probability of it > being sampled approaches one. > * Conversely, as a query’s execution time approaches the threshold, > the probability of it being sampled approaches zero. > > In other words, the sampling probability decreases linearly from 1 to > 0 as the execution time gets closer to the threshold. > > I believe this approach offers an ideal user experience. I have > attached a new patch implementing this logic. Please let me know if > you have any feedback regarding the comments in the code, the naming > of variables or documentation. I’m always open to discussion. > > -- > Best regards, > Ilia Evdokimov, > Tantor Labs LLC. > I’ve been closely reviewing my last (v5-*.patch) patch on implementing time-based sampling, and I’ve realized that it might not be the best approach. Let me explain the reasons. * We can only perform sampling before the 'pgss_planner()' function. However, at that point, we don’t yet know the query's execution time since it only becomes available during 'pgss_ExecutorEnd()' or 'pgss_ProcessUtility()'; * If we wait to sample until the execution completes and we have the actual execution time, this introduces a problem. By that point, we might have already recorded the query's statistics into shared memory from the 'pgss_planner()' making it too late to decide whether to sample the query; * Delaying sampling until the execution finishes would require waiting for the execution time, which could introduce performance overhead. This defeats the purpose of sampling, which aims to reduce the cost of tracking query. I believe we should reconsider the approach and revert to sampling based on v4-*.patch. If I’ve missed anything or there’s an alternative way to implement time threshold-based sampling efficiently, I’d be grateful to hear your insights. -- Best regards, Ilia Evdokimov, Tantor Labs LLC. --------------YxCFKBTrcciCSFlg7hz0acRS Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit


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 <michael@paquier.xyz> 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 proposal without looking at this
exact argument in depth.

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.  This means that we would combine the existing hash of pgss
to shrink to 8 bytes for objid rather than 13 bytes now as the current
code relies on (toplevel, userid, queryid) for the entry lookup (entry
removal is sniped with these three values as well, or dshash seq
scans).  The odds of conflicts still still play in our favor even if
we have a few million entries, or even ten times that.

If you run "pgbench -S -M prepared" on a pretty large machine with
high concurrency, then spin lock in pgss_store() could become pretty
much of a bottleneck.  And I'm not sure switching all counters to
atomics could somehow improve the situation given we already have
pretty many counters.

I'm generally +1 for the approach taken in this thread.  But I would
suggest introducing a threshold value for a query execution time, and
sample just everything below that threshold.  The slower query
shouldn't be sampled, because it can't be too frequent, and also it
could be more valuable to be counter individually (while very fast
queries probably only matter "in average").

------
Regards,
Alexander Korotkov
Supabase

I really liked your idea, and I’d like to propose an enhancement that I believe improves it further.

Yes, if a query’s execution time exceeds the threshold, it should always be tracked without sampling. However, for queries with execution times below the threshold, the sampling logic should prioritize shorter queries over those closer to the threshold. In my view, the ideal approach is for shorter queries to have the highest probability of being sampled, while queries closer to the threshold are less likely to be sampled.

This behavior can be achieved with the following logic:

pg_stat_statements.sample_exectime_threshold * random(0, 1) < total_time

Here’s how it works:

  • As a query’s execution time approaches zero, the probability of it being sampled approaches one.
  • Conversely, as a query’s execution time approaches the threshold, the probability of it being sampled approaches zero.

In other words, the sampling probability decreases linearly from 1 to 0 as the execution time gets closer to the threshold.

I believe this approach offers an ideal user experience. I have attached a new patch implementing this logic. Please let me know if you have any feedback regarding the comments in the code, the naming of variables or documentation. I’m always open to discussion.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.


I’ve been closely reviewing my last (v5-*.patch) patch on implementing time-based sampling, and I’ve realized that it might not be the best approach. Let me explain the reasons.

  • We can only perform sampling before the 'pgss_planner()' function. However, at that point, we don’t yet know the query's execution time since it only becomes available during 'pgss_ExecutorEnd()' or 'pgss_ProcessUtility()';
  • If we wait to sample until the execution completes and we have the actual execution time, this introduces a problem. By that point, we might have already recorded the query's statistics into shared memory from the 'pgss_planner()' making it too late to decide whether to sample the query;
  • Delaying sampling until the execution finishes would require waiting for the execution time, which could introduce performance overhead. This defeats the purpose of sampling, which aims to reduce the cost of tracking query.

I believe we should reconsider the approach and revert to sampling based on v4-*.patch. If I’ve missed anything or there’s an alternative way to implement time threshold-based sampling efficiently, I’d be grateful to hear your insights.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

--------------YxCFKBTrcciCSFlg7hz0acRS--