Content-Type: multipart/mixed; boundary="------------QGR02bJISSJrqkO0hXgWyEnQ"
Message-ID: <b610758f-0f19-420f-8847-1c01d8792619@tantorlabs.com>
Date: Tue, 19 Nov 2024 17:23:01 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Sample rate added to pg_stat_statements
To: "Andrey M. Borodin" <x4mmm@yandex-team.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <fe99e0ca-e564-480e-b865-5f0cee30bc60@tantorlabs.com>
 <c7ce658a-e0eb-4c5f-92a8-511314d89bbb@tantorlabs.com>
 <6707FCA9-FD1A-4609-A1A6-142456C14E0C@yandex-team.ru>
Content-Language: en-US
From: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
In-Reply-To: <6707FCA9-FD1A-4609-A1A6-142456C14E0C@yandex-team.ru>
Archived-At: <https://www.postgresql.org/message-id/b610758f-0f19-420f-8847-1c01d8792619%40tantorlabs.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------QGR02bJISSJrqkO0hXgWyEnQ
Content-Type: multipart/alternative;
 boundary="------------LOGJwgHrKWJQ6q6oVU07IVLQ"

--------------LOGJwgHrKWJQ6q6oVU07IVLQ
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 19.11.2024 15:11, Andrey M. Borodin wrote:

>
>> On 18 Nov 2024, at 23:33, Ilia Evdokimov<ilya.evdokimov@tantorlabs.com>  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 to disable pg_stat_statements or adjust the size of the statistics using pg_stat_statements.max, which is not always optimal. One potential solution to this issue could be query sampling in pg_stat_statements.
>>
>> A similar approach has been implemented in extensions like auto_explain and pg_store_plans, and it has proven very useful in high-load systems. However, this approach has its trade-offs, as it sacrifices statistical accuracy for improved performance. This patch introduces a new configuration parameter, pg_stat_statements.sample_rate for the pg_stat_statements extension. The patch provides the ability to control the sampling of query statistics in pg_stat_statements.
>>
>> This patch serves as a proof of concept (POC), and I would like to hear your thoughts on whether such an approach is viable and applicable.
> +1 for the idea. I heard a lot of complaints about that pgss is costly. Most of them were using it wrong though. But at least it could give an easy way to rule out performance impact of pgss.
Thank you for review.
>> On 19 Nov 2024, at 15:09, Ilia Evdokimov<ilya.evdokimov@tantorlabs.com>  wrote:
>>
>> 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 flexibility by allowing critical queries to be captured while maintaining efficient sampling.
> Is there a reason why pgss_ProcessUtility is excluded?
>
>
> Best regards, Andrey Borodin.
>

Ah, you’re right! Moreover, this check is needed not only in 
pgss_ProcessUtility() but in all places where pgss_enabled() is called. 
Therefore, it’s better to move the sampling check directly into 
pgss_enabled().

However, another issue arises with the initialization of 
'current_query_sample' variable that contains whether query is sampling 
or not. Initializing it in pgss_ExecutorStart()||is not sufficient, 
because pgss_post_parse_analyze() or pgss_ProcessUtility() might be 
called earlier. This could lead to different values of 
'current_query_sample' being used in these functions, which is 
undesirable. Run the regression tests for pg_stat_statements with 
initializing in pgss_ExecutorStart(), and you'll see this.

To avoid this, we need to find a function that is called earlier than 
all the others. In my opinion, pgss_post_parse_analyze() is a good 
candidate for this purpose. If you have objections or better 
suggestions, feel free to share them. I attached the patch with fixes.

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

--------------LOGJwgHrKWJQ6q6oVU07IVLQ
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>On 19.11.2024 15:11, Andrey M. Borodin wrote:</p>
    <blockquote type="cite"
      cite="mid:6707FCA9-FD1A-4609-A1A6-142456C14E0C@yandex-team.ru">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">On 18 Nov 2024, at 23:33, Ilia Evdokimov <a class="moz-txt-link-rfc2396E" href="mailto:ilya.evdokimov@tantorlabs.com">&lt;ilya.evdokimov@tantorlabs.com&gt;</a> 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 to disable pg_stat_statements or adjust the size of the statistics using pg_stat_statements.max, which is not always optimal. One potential solution to this issue could be query sampling in pg_stat_statements.

A similar approach has been implemented in extensions like auto_explain and pg_store_plans, and it has proven very useful in high-load systems. However, this approach has its trade-offs, as it sacrifices statistical accuracy for improved performance. This patch introduces a new configuration parameter, pg_stat_statements.sample_rate for the pg_stat_statements extension. The patch provides the ability to control the sampling of query statistics in pg_stat_statements.

This patch serves as a proof of concept (POC), and I would like to hear your thoughts on whether such an approach is viable and applicable.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
+1 for the idea. I heard a lot of complaints about that pgss is costly. Most of them were using it wrong though. But at least it could give an easy way to rule out performance impact of pgss.
</pre>
    </blockquote>
    Thank you for review.<br>
    <blockquote type="cite"
      cite="mid:6707FCA9-FD1A-4609-A1A6-142456C14E0C@yandex-team.ru">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">On 19 Nov 2024, at 15:09, Ilia Evdokimov <a class="moz-txt-link-rfc2396E" href="mailto:ilya.evdokimov@tantorlabs.com">&lt;ilya.evdokimov@tantorlabs.com&gt;</a> wrote:

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 flexibility by allowing critical queries to be captured while maintaining efficient sampling.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Is there a reason why pgss_ProcessUtility is excluded?


Best regards, Andrey Borodin.

</pre>
    </blockquote>
    <br>
    <p>Ah, you’re right! Moreover, this check is needed not only in
      pgss_ProcessUtility() but in all places where pgss_enabled() is
      called. Therefore, it’s better to move the sampling check directly
      into pgss_enabled().</p>
    <p>However, another issue arises with the initialization of
      'current_query_sample' variable that contains whether query is
      sampling or not. Initializing it in pgss_ExecutorStart()<code> </code>is
      not sufficient, because pgss_post_parse_analyze() or
      pgss_ProcessUtility() might be called earlier. This could lead to
      different values of 'current_query_sample' being used in these
      functions, which is undesirable. Run the regression tests for
      pg_stat_statements with initializing in pgss_ExecutorStart(), and
      you'll see this.<br>
    </p>
    <p>To avoid this, we need to find a function that is called earlier
      than all the others. In my opinion, pgss_post_parse_analyze() is a
      good candidate for this purpose. If you have objections or better
      suggestions, feel free to share them. I attached the patch with
      fixes.<br>
    </p>
    <p>--<br>
      Best regards,<br>
      Ilia Evdokimov,<br>
      Tantor Labs LLC.<br>
    </p>
  </body>
</html>

--------------LOGJwgHrKWJQ6q6oVU07IVLQ--
--------------QGR02bJISSJrqkO0hXgWyEnQ
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0001-Allow-setting-sample-ratio-for-pg_stat_statements.patch"
Content-Disposition: attachment;
 filename*0="v3-0001-Allow-setting-sample-ratio-for-pg_stat_statements.pa";
 filename*1="tch"
Content-Transfer-Encoding: base64

RnJvbSA3MjVkMGE0MjgwNDBkNDZjZmVkYWMwY2E3MDdkN2Q3YTI3ZGI4NzIxIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBJbGlhIEV2ZG9raW1vdiA8aWx5YS5ldmRva2ltb3ZA
dGFudG9ybGFicy5jb20+CkRhdGU6IFR1ZSwgMTkgTm92IDIwMjQgMTY6NTg6MjggKzAzMDAK
U3ViamVjdDogW1BBVENIXSBBbGxvdyBzZXR0aW5nIHNhbXBsZSByYXRpbyBmb3IgcGdfc3Rh
dF9zdGF0ZW1lbnRzCgpOZXcgY29uZmlndXJhdGlvbiBwYXJhbWV0ZXIgcGdfc3RhdF9zdGF0
ZW1lbnRzLnNhbXBsZV9yYXRpbyBtYWtlcyBpdApwb3NzaWJsZSB0byBjb250cm9sIGp1c3Qg
YSBmcmFjdGlvbiBvZiB0aGUgcXVlcmllcyBtZWV0aW5nIHRoZSBjb25maWd1cmVkCnRocmVz
aG9sZCwgdG8gcmVkdWNlIHRoZSBhbW91bnQgb2YgY29udHJvbGxpbmcuCi0tLQogLi4uL3Bn
X3N0YXRfc3RhdGVtZW50cy9wZ19zdGF0X3N0YXRlbWVudHMuYyAgIHwgMjIgKysrKysrKysr
KysrKysrKysrLQogZG9jL3NyYy9zZ21sL3Bnc3RhdHN0YXRlbWVudHMuc2dtbCAgICAgICAg
ICAgIHwgMTcgKysrKysrKysrKysrKysKIDIgZmlsZXMgY2hhbmdlZCwgMzggaW5zZXJ0aW9u
cygrKSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdpdCBhL2NvbnRyaWIvcGdfc3RhdF9zdGF0
ZW1lbnRzL3BnX3N0YXRfc3RhdGVtZW50cy5jIGIvY29udHJpYi9wZ19zdGF0X3N0YXRlbWVu
dHMvcGdfc3RhdF9zdGF0ZW1lbnRzLmMKaW5kZXggNDljNjU3YjNlMC4uMDE1ODZjMzJiMCAx
MDA2NDQKLS0tIGEvY29udHJpYi9wZ19zdGF0X3N0YXRlbWVudHMvcGdfc3RhdF9zdGF0ZW1l
bnRzLmMKKysrIGIvY29udHJpYi9wZ19zdGF0X3N0YXRlbWVudHMvcGdfc3RhdF9zdGF0ZW1l
bnRzLmMKQEAgLTQ5LDYgKzQ5LDcgQEAKIAogI2luY2x1ZGUgImFjY2Vzcy9wYXJhbGxlbC5o
IgogI2luY2x1ZGUgImNhdGFsb2cvcGdfYXV0aGlkLmgiCisjaW5jbHVkZSAiY29tbW9uL3Bn
X3BybmcuaCIKICNpbmNsdWRlICJjb21tb24vaW50LmgiCiAjaW5jbHVkZSAiZXhlY3V0b3Iv
aW5zdHJ1bWVudC5oIgogI2luY2x1ZGUgImZ1bmNhcGkuaCIKQEAgLTI5NCwxMiArMjk1LDE2
IEBAIHN0YXRpYyBib29sIHBnc3NfdHJhY2tfdXRpbGl0eSA9IHRydWU7CS8qIHdoZXRoZXIg
dG8gdHJhY2sgdXRpbGl0eSBjb21tYW5kcyAqLwogc3RhdGljIGJvb2wgcGdzc190cmFja19w
bGFubmluZyA9IGZhbHNlOwkvKiB3aGV0aGVyIHRvIHRyYWNrIHBsYW5uaW5nCiAJCQkJCQkJ
CQkJCSAqIGR1cmF0aW9uICovCiBzdGF0aWMgYm9vbCBwZ3NzX3NhdmUgPSB0cnVlOwkvKiB3
aGV0aGVyIHRvIHNhdmUgc3RhdHMgYWNyb3NzIHNodXRkb3duICovCitzdGF0aWMgZG91Ymxl
IHBnc3Nfc2FtcGxlX3JhdGUgPSAxOwogCisvKiBJcyB0aGUgY3VycmVudCB0b3AtbGV2ZWwg
cXVlcnkgdG8gYmUgc2FtcGxlZD8gKi8KK3N0YXRpYyBib29sIGN1cnJlbnRfcXVlcnlfc2Ft
cGxlZCA9IGZhbHNlOwogCiAjZGVmaW5lIHBnc3NfZW5hYmxlZChsZXZlbCkgXAogCSghSXNQ
YXJhbGxlbFdvcmtlcigpICYmIFwKIAkocGdzc190cmFjayA9PSBQR1NTX1RSQUNLX0FMTCB8
fCBcCi0JKHBnc3NfdHJhY2sgPT0gUEdTU19UUkFDS19UT1AgJiYgKGxldmVsKSA9PSAwKSkp
CisJKHBnc3NfdHJhY2sgPT0gUEdTU19UUkFDS19UT1AgJiYgKGxldmVsKSA9PSAwKSkgJiYg
XAorCWN1cnJlbnRfcXVlcnlfc2FtcGxlZCkKIAogI2RlZmluZSByZWNvcmRfZ2NfcXRleHRz
KCkgXAogCWRvIHsgXApAQCAtNDE0LDYgKzQxOSwxOSBAQCBfUEdfaW5pdCh2b2lkKQogCQkJ
CQkJCU5VTEwsCiAJCQkJCQkJTlVMTCk7CiAKKwlEZWZpbmVDdXN0b21SZWFsVmFyaWFibGUo
InBnX3N0YXRfc3RhdGVtZW50cy5zYW1wbGVfcmF0ZSIsCisJCQkJCQkJICJGcmFjdGlvbiBv
ZiBxdWVyaWVzIHRvIHByb2Nlc3MuIiwKKwkJCQkJCQkgTlVMTCwKKwkJCQkJCQkgJnBnc3Nf
c2FtcGxlX3JhdGUsCisJCQkJCQkJIDEuMCwKKwkJCQkJCQkgMC4wLAorCQkJCQkJCSAxLjAs
CisJCQkJCQkJIFBHQ19TVVNFVCwKKwkJCQkJCQkgMCwKKwkJCQkJCQkgTlVMTCwKKwkJCQkJ
CQkgTlVMTCwKKwkJCQkJCQkgTlVMTCk7CisKIAlEZWZpbmVDdXN0b21FbnVtVmFyaWFibGUo
InBnX3N0YXRfc3RhdGVtZW50cy50cmFjayIsCiAJCQkJCQkJICJTZWxlY3RzIHdoaWNoIHN0
YXRlbWVudHMgYXJlIHRyYWNrZWQgYnkgcGdfc3RhdF9zdGF0ZW1lbnRzLiIsCiAJCQkJCQkJ
IE5VTEwsCkBAIC04MzUsNiArODUzLDggQEAgcGdzc19wb3N0X3BhcnNlX2FuYWx5emUoUGFy
c2VTdGF0ZSAqcHN0YXRlLCBRdWVyeSAqcXVlcnksIEp1bWJsZVN0YXRlICpqc3RhdGUpCiAJ
aWYgKHByZXZfcG9zdF9wYXJzZV9hbmFseXplX2hvb2spCiAJCXByZXZfcG9zdF9wYXJzZV9h
bmFseXplX2hvb2socHN0YXRlLCBxdWVyeSwganN0YXRlKTsKIAorCWN1cnJlbnRfcXVlcnlf
c2FtcGxlZCA9IChwZ19wcm5nX2RvdWJsZSgmcGdfZ2xvYmFsX3Bybmdfc3RhdGUpIDwgcGdz
c19zYW1wbGVfcmF0ZSk7CisKIAkvKiBTYWZldHkgY2hlY2suLi4gKi8KIAlpZiAoIXBnc3Mg
fHwgIXBnc3NfaGFzaCB8fCAhcGdzc19lbmFibGVkKG5lc3RpbmdfbGV2ZWwpKQogCQlyZXR1
cm47CmRpZmYgLS1naXQgYS9kb2Mvc3JjL3NnbWwvcGdzdGF0c3RhdGVtZW50cy5zZ21sIGIv
ZG9jL3NyYy9zZ21sL3Bnc3RhdHN0YXRlbWVudHMuc2dtbAppbmRleCA1MDFiNDY4ZTlhLi4x
ZTI1MzNhODAyIDEwMDY0NAotLS0gYS9kb2Mvc3JjL3NnbWwvcGdzdGF0c3RhdGVtZW50cy5z
Z21sCisrKyBiL2RvYy9zcmMvc2dtbC9wZ3N0YXRzdGF0ZW1lbnRzLnNnbWwKQEAgLTkzNiw2
ICs5MzYsMjMgQEAKICAgICA8L2xpc3RpdGVtPgogICAgPC92YXJsaXN0ZW50cnk+CiAKKyAg
IDx2YXJsaXN0ZW50cnk+CisgICAgPHRlcm0+CisgICAgIDx2YXJuYW1lPnBnX3N0YXRfc3Rh
dGVtZW50cy5zYW1wbGVfcmF0ZTwvdmFybmFtZT4gKDx0eXBlPnJlYWw8L3R5cGU+KQorICAg
ICA8aW5kZXh0ZXJtPgorICAgICAgPHByaW1hcnk+PHZhcm5hbWU+cGdfc3RhdF9zdGF0ZW1l
bnRzLnNhbXBsZV9yYXRlPC92YXJuYW1lPiBjb25maWd1cmF0aW9uIHBhcmFtZXRlcjwvcHJp
bWFyeT4KKyAgICAgPC9pbmRleHRlcm0+CisgICAgPC90ZXJtPgorCisgICAgPGxpc3RpdGVt
PgorICAgICA8cGFyYT4KKyAgICAgIDx2YXJuYW1lPnBnX3N0YXRfc3RhdGVtZW50cy5zYW1w
bGVfcmF0ZTwvdmFybmFtZT4gY2F1c2VzIHBnX3N0YXRfc3RhdGVtZW50cyB0byBvbmx5Cisg
ICAgICBjb250cm9sIGEgZnJhY3Rpb24gb2YgdGhlIHN0YXRlbWVudHMgaW4gZWFjaCBzZXNz
aW9uLiAgVGhlIGRlZmF1bHQgaXMgMSwKKyAgICAgIG1lYW5pbmcgY29udHJvbCBhbGwgdGhl
IHF1ZXJpZXMuIE9ubHkgc3VwZXJ1c2VycyBjYW4gY2hhbmdlIHRoaXMgc2V0dGluZy4KKyAg
ICAgPC9wYXJhPgorICAgIDwvbGlzdGl0ZW0+CisgICA8L3Zhcmxpc3RlbnRyeT4KKwogICAg
PHZhcmxpc3RlbnRyeT4KICAgICA8dGVybT4KICAgICAgPHZhcm5hbWU+cGdfc3RhdF9zdGF0
ZW1lbnRzLnNhdmU8L3Zhcm5hbWU+ICg8dHlwZT5ib29sZWFuPC90eXBlPikKLS0gCjIuMzQu
MQoK

--------------QGR02bJISSJrqkO0hXgWyEnQ--