public inbox for [email protected]help / color / mirror / Atom feed
Cancel problems of query to pg_stat_statements 4+ messages / 3 participants [nested] [flat]
* Cancel problems of query to pg_stat_statements @ 2025-04-24 17:49 Roman Khapov <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Roman Khapov @ 2025-04-24 17:49 UTC (permalink / raw) To: pgsql-hackers ------==--bound.20.dbernar-corp-main-90.vla.yp-c.yandex.net Content-Transfer-Encoding: base64 Content-Type: text/html; charset=utf-8 PGRpdj5IaSE8L2Rpdj48ZGl2PsKgPC9kaXY+PGRpdj5SZWNlbnRseSB3ZSBmYWNlZCBhIHByb2Js ZW0gaW4gb3V0IHByb2R1Y3Rpb24gcHNxbCBpbnN0YWxsYXRpb24sIHdoaWNoIHdhcyB0aGF0IHdl IGhhZCB0byBjYW5jZWwgYWxsIHJlcXVlc3RzIHRvIHRoZSBkYiwgaW5jbHVkaW5nIHBlcmZvcm1h bmNlIG1vbml0b3JpbmcgcmVxdWVzdHMsIHRoYXQgdXNlcyBwc19zdGF0X3N0YXRlbWVudHMuIEJ1 dCB3ZSBjb3VsZCBub3QgY2FuY2VsIHRoZSByZXF1ZXN0IGluIHVzdWFsIHdheSwgYW5kIGhhZCB0 byBraWxsIC05IHRoZSBwZyBwcm9jZXNzIG9mIGl0LjwvZGl2PjxkaXY+wqA8L2Rpdj48ZGl2Pldl J3ZlIG5vdGljZWQgdGhhdCB0aGUgdGhlIHF1ZXJ5IGV4ZWN1dGlvbiBzdHVjayBvbiA8c3BhbiBz dHlsZT0iYmFja2dyb3VuZC1jb2xvcjp0cmFuc3BhcmVudCI+UEdTU19URVhUX0ZJTEUgZmlsZSBy ZWFkaW5nIDwvc3Bhbj5pbiBmdW5jdGlvbiBxdGV4dF9sb2FkX2ZpbGUsIHdoaWNoIGRvZXNuJ3Qg aGF2ZSA8c3BhbiBzdHlsZT0iYmFja2dyb3VuZC1jb2xvcjp0cmFuc3BhcmVudCI+Q0hFQ0tfRk9S X0lOVEVSUlVQVFMgaW4gdGhlIHJlYWQgY3ljbGUuIEluIGFkZGl0aW9uIHRvIG91ciBjYXNlIHdp dGggbGFyZ2UgUEdTU19URVhUX0ZJTEUgKGFuZCBtYXliZSB0aGUgcHJvYmxlbXMgd2l0aCB2aXJ0 dWFsIGRpc2sgaS9vKSB0aGF0IGNhbiBleHBsYWluIHVuY2FuY2VsbGFibGUgcGdfc3RhdF9zdGF0 ZW1lbnRzIHF1ZXJpZXMuPC9zcGFuPjwvZGl2PjxkaXY+wqA8L2Rpdj48ZGl2PjxzcGFuIHN0eWxl PSJiYWNrZ3JvdW5kLWNvbG9yOnRyYW5zcGFyZW50Ij5BbHNvLCB0aGUgcmVhZGluZyBibG9jayBz aXplIGNhbiBiZSByZWR1Y2VkIGZyb20gMUdCIHRvIDMyTUIgaW4gb3JkZXIgdG8gaW5jcmVhc2Ug dGhlIGZyZXF1ZW5jeSBvZiBDSEVDS19GT1JfSU5URVJSVVBUUyBjYWxscyB3aXRob3V0IDwvc3Bh bj5xdGV4dF9sb2FkX2ZpbGXCoHBlcmZvcm1hbmNlIGRlZ3JhZGF0aW9uPHNwYW4gc3R5bGU9ImJh Y2tncm91bmQtY29sb3I6dHJhbnNwYXJlbnQiPi4gVG8gY2hlY2sgdGhhdCBJIGRpZCBzb21lIGxp dHRsZSB0ZXN0aW5nIHdpdGggZmlvIGxpa2U6PC9zcGFuPjwvZGl2PjxkaXY+wqA8L2Rpdj48ZGl2 PmZpbyAtLW5hbWU9cmVhZHRlc3QgLS1maWxlbmFtZT0uL3JhbmRvbS1ieXRlcy1maWxlIC0tcnc9 cmVhZCAtLWJzPTMybSAtLXNpemU9MTBHIC0taW9lbmdpbmU9bGliYWlvIC0tZGlyZWN0PTEgLS1u dW1qb2JzPTE8L2Rpdj48ZGl2PsKgPC9kaXY+PGRpdj5TbywgSSBtYWRlIGEgc2ltcGxlIHBhdGNo IHRoYXQgYWRkcyBDSEVDS19GT1JfSU5URVJSVVBUUyBjYWxsIGluIHJlYWQgY3ljbGUgb2YgcXRl eHRfbG9hZF9maWxlwqBhbmQgY2hhbmdlIG1heCB2YWx1ZSBvZiB0b3JlYWQgZnJvbSAxR0IgdG8g MzJNQi48L2Rpdj48ZGl2PsKgPC9kaXY+PGRpdj48c3BhbiBzdHlsZT0id2hpdGUtc3BhY2U6cHJl LXdyYXAiPkkgd291bGQgYXBwcmVjaWF0ZSB5b3VyIGZlZWRiYWNrIG9uIHRoZXNlIGNoYW5nZXMu PC9zcGFuPjwvZGl2PjxkaXY+wqA8L2Rpdj48ZGl2PsKgPC9kaXY+ ------==--bound.20.dbernar-corp-main-90.vla.yp-c.yandex.net Content-Disposition: attachment; filename="cancelable-qtext_load_file.patch" Content-Transfer-Encoding: base64 Content-Type: text/x-diff; name="cancelable-qtext_load_file.patch" RnJvbSBlYWIwNDFmY2QxZjQ5NzNiMDNmNmJlYmQzN2JlZjMzOTVmZTY0YjRiIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBya2hhcG92IDxyLmtoYXBvdkB5YS5ydT4KRGF0ZTogVGh1LCAy NCBBcHIgMjAyNSAxNzowMTo1NCArMDAwMApTdWJqZWN0OiBbUEFUQ0hdIHBnX3N0YXRfc3RhdGVt ZW50cy5jOiBjYW5jZWxhYmxlIHF0ZXh0X2xvYWRfZmlsZQoKSW4gdGhlIGNhc2Ugb2YgYSBsYXJn ZSBQR1NTX1RFWFRfRklMRSwgdGhlIHdvcmsgdGltZSBvZiB0aGUgcXRleHRfbG9hZF9maWxlCmZ1 bmN0aW9uIHdpbGwgYmUgcXVpdGUgbG9uZywgYW5kIHRoZSBxdWVyeSB0byB0aGUgcGdfc3RhdF9z dGF0ZW1lbnRzIHRhYmxlCndpbGwgbm90IGJlIGNhbmNlbGxhYmxlLCBhcyB0aGVyZSBpcyBubyBD SEVDS19GT1JfSU5URVJSVVBUIGluIHRoZSBmdW5jdGlvbi4KCkFsc28sIHRoZSBhbW91bnQgb2Yg Ynl0ZXMgcmVhZCBjYW4gcmVhY2ggMSBHQiwgd2hpY2ggbGVhZHMgdG8gYSBzbG93IHJlYWQKc3lz dGVtIGNhbGwgdGhhdCBkb2VzIG5vdCBhbGxvdyBjYW5jZWxsYXRpb24gb2YgdGhlIHF1ZXJ5LiBU ZXN0aW5nIHRoZSBzcGVlZApvZiBzZXF1ZW50aWFsIHJlYWQgdXNpbmcgZmlvIHdpdGggZGlmZmVy ZW50IGJsb2NrIHNpemVzIHNob3dzIHRoYXQgdGhlcmUgaXMKbm8gc2lnbmlmaWNhbnQgZGlmZmVy ZW5jZSBiZXR3ZWVuIDE2IE1CIGJsb2NrcyBhbmQgMSBHQiBibG9ja3MuCgpUaGVyZWZvcmUsIHRo aXMgcGF0Y2ggY2hhbmdlcyB0aGUgbWF4aW11bSByZWFkIHZhbHVlIGZyb20gMSBHQiB0byAxNiBN QiBhbmQKYWRkcyBDSEVDS19GT1JfSU5URVJSVVBUSU9OIGluIHRoZSByZWFkIGxvb3Agb2YgcXRl eHRfbG9hZF9maWxlIHRvIG1ha2UgaXQgY2FuY2VsbGFibGUuCgpTaWduZWQtb2ZmLWJ5OiBya2hh cG92IDxyLmtoYXBvdkB5YS5ydT4KLS0tCiBjb250cmliL3BnX3N0YXRfc3RhdGVtZW50cy9wZ19z dGF0X3N0YXRlbWVudHMuYyB8IDQgKysrLQogMSBmaWxlIGNoYW5nZWQsIDMgaW5zZXJ0aW9ucygr KSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdpdCBhL2NvbnRyaWIvcGdfc3RhdF9zdGF0ZW1lbnRz L3BnX3N0YXRfc3RhdGVtZW50cy5jIGIvY29udHJpYi9wZ19zdGF0X3N0YXRlbWVudHMvcGdfc3Rh dF9zdGF0ZW1lbnRzLmMKaW5kZXggOTc3ODQwN2NiYTMuLmNkMzRmMWNlMjQ4IDEwMDY0NAotLS0g YS9jb250cmliL3BnX3N0YXRfc3RhdGVtZW50cy9wZ19zdGF0X3N0YXRlbWVudHMuYworKysgYi9j b250cmliL3BnX3N0YXRfc3RhdGVtZW50cy9wZ19zdGF0X3N0YXRlbWVudHMuYwpAQCAtMjM2NSw3 ICsyMzY1LDkgQEAgcXRleHRfbG9hZF9maWxlKFNpemUgKmJ1ZmZlcl9zaXplKQogCW5yZWFkID0g MDsKIAl3aGlsZSAobnJlYWQgPCBzdGF0LnN0X3NpemUpCiAJewotCQlpbnQJCQl0b3JlYWQgPSBN aW4oMTAyNCAqIDEwMjQgKiAxMDI0LCBzdGF0LnN0X3NpemUgLSBucmVhZCk7CisJCWludAkJCXRv cmVhZCA9IE1pbigzMiAqIDEwMjQgKiAxMDI0LCBzdGF0LnN0X3NpemUgLSBucmVhZCk7CisKKwkJ Q0hFQ0tfRk9SX0lOVEVSUlVQVFMoKTsKIAogCQkvKgogCQkgKiBJZiB3ZSBnZXQgYSBzaG9ydCBy ZWFkIGFuZCBlcnJubyBkb2Vzbid0IGdldCBzZXQsIHRoZSByZWFzb24gaXMKLS0gCjIuNDMuMAoK Cg== ------==--bound.20.dbernar-corp-main-90.vla.yp-c.yandex.net-- Attachments: [text/x-diff] cancelable-qtext_load_file.patch (1.6K, 2-cancelable-qtext_load_file.patch) download | inline diff: From eab041fcd1f4973b03f6bebd37bef3395fe64b4b Mon Sep 17 00:00:00 2001 From: rkhapov <[email protected]> Date: Thu, 24 Apr 2025 17:01:54 +0000 Subject: [PATCH] pg_stat_statements.c: cancelable qtext_load_file In the case of a large PGSS_TEXT_FILE, the work time of the qtext_load_file function will be quite long, and the query to the pg_stat_statements table will not be cancellable, as there is no CHECK_FOR_INTERRUPT in the function. Also, the amount of bytes read can reach 1 GB, which leads to a slow read system call that does not allow cancellation of the query. Testing the speed of sequential read using fio with different block sizes shows that there is no significant difference between 16 MB blocks and 1 GB blocks. Therefore, this patch changes the maximum read value from 1 GB to 16 MB and adds CHECK_FOR_INTERRUPTION in the read loop of qtext_load_file to make it cancellable. Signed-off-by: rkhapov <[email protected]> --- contrib/pg_stat_statements/pg_stat_statements.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 9778407cba3..cd34f1ce248 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -2365,7 +2365,9 @@ qtext_load_file(Size *buffer_size) nread = 0; while (nread < stat.st_size) { - int toread = Min(1024 * 1024 * 1024, stat.st_size - nread); + int toread = Min(32 * 1024 * 1024, stat.st_size - nread); + + CHECK_FOR_INTERRUPTS(); /* * If we get a short read and errno doesn't get set, the reason is -- 2.43.0 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Cancel problems of query to pg_stat_statements @ 2025-04-28 06:16 Andrey Borodin <[email protected]> parent: Roman Khapov <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Andrey Borodin @ 2025-04-28 06:16 UTC (permalink / raw) To: Roman Khapov <[email protected]>; +Cc: pgsql-hackers > On 24 Apr 2025, at 22:49, Roman Khapov <[email protected]> wrote: > > Hi! > Recently we faced a problem in out production psql installation, which was that we had to cancel all requests to the db, including performance monitoring requests, that uses ps_stat_statements. But we could not cancel the request in usual way, and had to kill -9 the pg process of it. Interesting problem, thanks for raising it! > We've noticed that the the query execution stuck on PGSS_TEXT_FILE file reading in function qtext_load_file, which doesn't have CHECK_FOR_INTERRUPTS in the read cycle. In addition to our case with large PGSS_TEXT_FILE (and maybe the problems with virtual disk i/o) that can explain uncancellable pg_stat_statements queries. I'm afraid it might be not so easy to add CHECK_FOR_INTERRUPTS there. Most probably you was holding a LWLockAcquire(pgss->lock, LW_SHARED) somewhere (do you have a backtrace?), which prevent interrupts anyway. Thanks! Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH v2] Re: Cancel problems of query to pg_stat_statements @ 2025-05-12 10:48 Roman Khapov <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Roman Khapov @ 2025-05-12 10:48 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: pgsql-hackers Hi! Thanks for the review! Its true that qtext_load_file() can be called with acquired lock in pg_stat_statements_internal()… So I perform INTERRUPTS_PENDING_CONDITION in v2 patch and call CHECK_FOR_INTERRUPTS later, after cycle ended and lock released. > On 28 Apr 2025, at 11:16, Andrey Borodin <[email protected]> wrote: > > > >> On 24 Apr 2025, at 22:49, Roman Khapov <[email protected]> wrote: >> >> Hi! >> Recently we faced a problem in out production psql installation, which was that we had to cancel all requests to the db, including performance monitoring requests, that uses ps_stat_statements. But we could not cancel the request in usual way, and had to kill -9 the pg process of it. > > Interesting problem, thanks for raising it! > >> We've noticed that the the query execution stuck on PGSS_TEXT_FILE file reading in function qtext_load_file, which doesn't have CHECK_FOR_INTERRUPTS in the read cycle. In addition to our case with large PGSS_TEXT_FILE (and maybe the problems with virtual disk i/o) that can explain uncancellable pg_stat_statements queries. > > I'm afraid it might be not so easy to add CHECK_FOR_INTERRUPTS there. Most probably you was holding a LWLockAcquire(pgss->lock, LW_SHARED) somewhere (do you have a backtrace?), which prevent interrupts anyway. > > Thanks! > > > Best regards, Andrey Borodin. Attachments: [application/octet-stream] v2-cancelable-qtext_load_file.patch (4.4K, 2-v2-cancelable-qtext_load_file.patch) download | inline diff: From f642c28124e1bd57764bd12296253b78fc9c083c Mon Sep 17 00:00:00 2001 From: rkhapov <[email protected]> Date: Mon, 12 May 2025 15:29:17 +0500 Subject: [PATCH v2] pg_stat_statements.c: cancelable qtext_load_file In the case of a large PGSS_TEXT_FILE, the work time of the qtext_load_file function will be quite long, and the query to the pg_stat_statements table will not be cancellable, as there is no CHECK_FOR_INTERRUPT in the function. Also, the amount of bytes read can reach 1 GB, which leads to a slow read system call that does not allow cancellation of the query. Testing the speed of sequential read using fio with different block sizes shows that there is no significant difference between 16 MB blocks and 1 GB blocks. Therefore, this patch changes the maximum read value from 1 GB to 16 MB and adds INTERRUPTS_PENDING_CONDITION() check in the read loop of qtext_load_file to make it cancellable. For now, only statement execution is cancellable (fail_on_interrupt is true only for calls from pg_stat_statements_internal) Signed-off-by: rkhapov <[email protected]> --- .../pg_stat_statements/pg_stat_statements.c | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 9778407cba3..587d49f4330 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -365,7 +365,7 @@ static pgssEntry *entry_alloc(pgssHashKey *key, Size query_offset, int query_len static void entry_dealloc(void); static bool qtext_store(const char *query, int query_len, Size *query_offset, int *gc_count); -static char *qtext_load_file(Size *buffer_size); +static char *qtext_load_file(Size *buffer_size, bool fail_on_interrupts); static char *qtext_fetch(Size query_offset, int query_len, char *buffer, Size buffer_size); static bool need_gc_qtexts(void); @@ -767,7 +767,7 @@ pgss_shmem_shutdown(int code, Datum arg) if (fwrite(&num_entries, sizeof(int32), 1, file) != 1) goto error; - qbuffer = qtext_load_file(&qbuffer_size); + qbuffer = qtext_load_file(&qbuffer_size, false /* fail_on_interrupts */ ); if (qbuffer == NULL) goto error; @@ -1767,7 +1767,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, /* No point in loading file now if there are active writers */ if (n_writers == 0) - qbuffer = qtext_load_file(&qbuffer_size); + qbuffer = qtext_load_file(&qbuffer_size, true /* fail_on_interrupts */ ); } /* @@ -1800,7 +1800,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, pgss->gc_count != gc_count) { free(qbuffer); - qbuffer = qtext_load_file(&qbuffer_size); + qbuffer = qtext_load_file(&qbuffer_size, true /* fail_on_interrupts */ ); } } @@ -1819,6 +1819,12 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, memset(values, 0, sizeof(values)); memset(nulls, 0, sizeof(nulls)); + /* Can't process interrupts here - pgss-lock is acquired */ + if (INTERRUPTS_PENDING_CONDITION()) + { + break; + } + values[i++] = ObjectIdGetDatum(entry->key.userid); values[i++] = ObjectIdGetDatum(entry->key.dbid); if (api_version >= PGSS_V1_9) @@ -2014,6 +2020,8 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, LWLockRelease(pgss->lock); + CHECK_FOR_INTERRUPTS(); + free(qbuffer); } @@ -2312,7 +2320,7 @@ error: * the caller is responsible for verifying that the result is sane. */ static char * -qtext_load_file(Size *buffer_size) +qtext_load_file(Size *buffer_size, bool fail_on_interrupts) { char *buf; int fd; @@ -2365,7 +2373,14 @@ qtext_load_file(Size *buffer_size) nread = 0; while (nread < stat.st_size) { - int toread = Min(1024 * 1024 * 1024, stat.st_size - nread); + int toread = Min(32 * 1024 * 1024, stat.st_size - nread); + + if (fail_on_interrupts && INTERRUPTS_PENDING_CONDITION()) + { + free(buf); + CloseTransientFile(fd); + return NULL; + } /* * If we get a short read and errno doesn't get set, the reason is @@ -2502,7 +2517,7 @@ gc_qtexts(void) * file is only going to get bigger; hoping for a future non-OOM result is * risky and can easily lead to complete denial of service. */ - qbuffer = qtext_load_file(&qbuffer_size); + qbuffer = qtext_load_file(&qbuffer_size, false /* fail_on_interrupts */ ); if (qbuffer == NULL) goto gc_fail; -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH v2] Re: Cancel problems of query to pg_stat_statements @ 2026-03-25 05:11 Michael Paquier <[email protected]> parent: Roman Khapov <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Michael Paquier @ 2026-03-25 05:11 UTC (permalink / raw) To: Roman Khapov <[email protected]>; +Cc: Andrey Borodin <[email protected]>; pgsql-hackers On Mon, May 12, 2025 at 03:48:29PM +0500, Roman Khapov wrote: > Its true that qtext_load_file() can be called with acquired lock in > pg_stat_statements_internal()... > So I perform INTERRUPTS_PENDING_CONDITION in v2 patch and call > CHECK_FOR_INTERRUPTS later, after cycle ended and lock released. This patch is only a workaround for a larger problem: the PGSS text file does not and will never scale once it reaches a large size and once we have a high turnover rate of the PGSS entries due to different query IDs. FWIW, Sami Imseih has mentioned to me a few days ago that the state of the pgstats API (that can be used for custom stats kinds as well) was basically in a shape good enough to move PGSS to pgstats as a custom kind, and we should be able to move the query text file to be entirely in-memory. This proposal will hopefully materialize at the beginning of the v20 cycle. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-03-25 05:11 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-04-24 17:49 Cancel problems of query to pg_stat_statements Roman Khapov <[email protected]> 2025-04-28 06:16 ` Andrey Borodin <[email protected]> 2025-05-12 10:48 ` Roman Khapov <[email protected]> 2026-03-25 05:11 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox