Missing CFI in iterate_word_similarity()
Hi,
For long strings, iterate_word_similarity() can run into long-running
tight-loops without honouring interrupts or statement_timeouts. For
example:
postgres=# set statement_timeout='1s';
SET
postgres=# select 1 where repeat('1.1',80000) %>> 'Lorem ipsum dolor sit amet';
?column?
----------
(0 rows)
Time: 29615.842 ms (00:29.616)
The associated perf report:
+ 99.98% 0.00% postgres postgres [.] ExecQual
+ 99.98% 0.00% postgres postgres [.] ExecEvalExprSwitchContext
+ 99.98% 0.00% postgres pg_trgm.so [.] strict_word_similarity_commutator_op
+ 99.98% 0.00% postgres pg_trgm.so [.] calc_word_similarity
+ 99.68% 99.47% postgres pg_trgm.so [.] iterate_word_similarity
0.21% 0.03% postgres postgres [.] pg_qsort
0.16% 0.00% postgres [kernel.kallsyms] [k] asm_sysvec_apic_timer_interrupt
0.16% 0.00% postgres [kernel.kallsyms] [k] sysvec_apic_timer_interrupt
0.16% 0.11% postgres [kernel.kallsyms] [k] __softirqentry_text_start
0.16% 0.00% postgres [kernel.kallsyms] [k] irq_exit_rcuAdding CHECK_FOR_INTERRUPTS() ensures that such queries respond to
statement_timeout & Ctrl-C signals. With the patch applied, the
above query will interrupt more quickly:
postgres=# select 1 where repeat('1.1',80000) %>> 'Lorem ipsum dolor sit amet';
ERROR: canceling statement due to statement timeout
Time: 1000.768 ms (00:01.001)
Please find the patch attached. The patch does not show any performance
regressions when run against the above use-case. Thanks to SQLSmith
for indirectly leading me to this scenario.
-
Robins Tharakan
Amazon Web Services
Attachments:
v1_cfi_iterate_word_similarity.patchapplication/octet-stream; name=v1_cfi_iterate_word_similarity.patchDownload
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 49b3609de9..2df573876d 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -7,6 +7,7 @@
#include "catalog/pg_type.h"
#include "lib/qunique.h"
+#include "miscadmin.h"
#include "trgm.h"
#include "tsearch/ts_locale.h"
#include "utils/lsyscache.h"
@@ -492,6 +493,8 @@ iterate_word_similarity(int *trg2indexes,
for (i = 0; i < len2; i++)
{
+ CHECK_FOR_INTERRUPTS();
+
/* Get index of next trigram */
int trgindex = trg2indexes[i];
On 2 Aug 2022, at 04:41, Robins Tharakan <tharakan@gmail.com> wrote:
For long strings, iterate_word_similarity() can run into long-running
tight-loops without honouring interrupts or statement_timeouts.
Adding CHECK_FOR_INTERRUPTS() ensures that such queries respond to
statement_timeout & Ctrl-C signals. With the patch applied, the
above query will interrupt more quickly:
Makes sense. While this might be a bit of a theoretical issue given the
lengths required, the fix is still sane and any such query should honor
statement timeouts (especially in a trusted extension).
Please find the patch attached. The patch does not show any performance
regressions when run against the above use-case.
I wasn't able to find one either.
+ CHECK_FOR_INTERRUPTS();
+
/* Get index of next trigram */
int trgindex = trg2indexes[i];Placing code before declarations will generate a compiler warning, so the check
must go after trgindex is declared. I've fixed that in the attached to get the
cfbot green. Marking this ready for committer in the meantime.
Looking at this I also noticed that commit be8a7a68662 changed the check_only
param to instead use a flag value but didn't update all comments. 0002 fixes
that while in there.
--
Daniel Gustafsson https://vmware.com/
Attachments:
v2-0002-Update-out-of-date-comments-in-pg_trgm.patchapplication/octet-stream; name=v2-0002-Update-out-of-date-comments-in-pg_trgm.patch; x-unix-mode=0644Download
From 268a6224d3f2e536308bb224e00509f9062464d2 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 2 Sep 2022 14:21:11 +0200
Subject: [PATCH v2 2/2] Update out of date comments in pg_trgm
Commit be8a7a68662 changed the check_only parameter to a flag array
but missed updating all comments. Update, and fix a related typo.
---
contrib/pg_trgm/trgm_op.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index eec06ef09c..668e872595 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -442,14 +442,15 @@ comp_ptrgm(const void *v1, const void *v2)
/*
* Iterative search function which calculates maximum similarity with word in
- * the string. But maximum similarity is calculated only if check_only == false.
+ * the string. Maximum similarity is only calculated only if the flag
+ * WORD_SIMILARITY_CHECK_ONLY isn't set.
*
* trg2indexes: array which stores indexes of the array "found".
* found: array which stores true of false values.
* ulen1: count of unique trigrams of array "trg1".
* len2: length of array "trg2" and array "trg2indexes".
* len: length of the array "found".
- * lags: set of boolean flags parameterizing similarity calculation.
+ * flags: set of boolean flags parameterizing similarity calculation.
* bounds: whether each trigram is left/right bound of word.
*
* Returns word similarity.
--
2.32.1 (Apple Git-133)
v2-0001-Check-for-interrupts-in-pg_trgm-word-similarity.patchapplication/octet-stream; name=v2-0001-Check-for-interrupts-in-pg_trgm-word-similarity.patch; x-unix-mode=0644Download
From 5c7f06a9cf72f80461302f741fddf5e018ac7575 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 2 Sep 2022 14:15:19 +0200
Subject: [PATCH v2 1/2] Check for interrupts in pg_trgm word similarity
Calculating similarity between large strings can be timesconsuming
and overrun configured statement timeouts. Check for interrupts in
the main loop to ensure query cancellation can be performed.
Author: Robins Tharakan <tharakan@gmail.com>
Discussion: https://postgr.es/m/CAEP4nAxvmfc_XWTz73bqXRhgjONi=1HaX4_NhsopA3L6UvnN1g@mail.gmail.com
---
contrib/pg_trgm/trgm_op.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 49b3609de9..eec06ef09c 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -7,6 +7,7 @@
#include "catalog/pg_type.h"
#include "lib/qunique.h"
+#include "miscadmin.h"
#include "trgm.h"
#include "tsearch/ts_locale.h"
#include "utils/lsyscache.h"
@@ -495,6 +496,8 @@ iterate_word_similarity(int *trg2indexes,
/* Get index of next trigram */
int trgindex = trg2indexes[i];
+ CHECK_FOR_INTERRUPTS();
+
/* Update last position of this trigram */
if (lower >= 0 || found[trgindex])
{
--
2.32.1 (Apple Git-133)
Daniel Gustafsson <daniel@yesql.se> writes:
Placing code before declarations will generate a compiler warning, so the check
must go after trgindex is declared. I've fixed that in the attached to get the
cfbot green. Marking this ready for committer in the meantime.
I noticed the same thing, but sticking the CFI immediately after the
declaration didn't read well either. I was considering moving it to
the bottom of the loop instead of that. A possible objection is that
if there's ever a "continue;" in the loop, those iterations would bypass
the CFI; but we don't necessarily need a CFI every time.
regards, tom lane
On 2 Sep 2022, at 14:57, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Placing code before declarations will generate a compiler warning, so the check
must go after trgindex is declared. I've fixed that in the attached to get the
cfbot green. Marking this ready for committer in the meantime.I noticed the same thing, but sticking the CFI immediately after the
declaration didn't read well either. I was considering moving it to
the bottom of the loop instead of that.
I was contemplating that too, but kept it at the top after seeing quite a few
examples of that in other contrib modules (like amcheck/verify_nbtree.c and
pg_visibility/pg_visibility.c). I don't have any strong feelings either way,
I'm happy to move it last.
A possible objection is that
if there's ever a "continue;" in the loop, those iterations would bypass
the CFI; but we don't necessarily need a CFI every time.
Yeah, I don't think we need to worry about that. If an added continue;
shortcuts the loop to the point where keeping the CFI last becomes a problem
then it's probably time to look at rewriting the loop.
--
Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
On 2 Sep 2022, at 14:57, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I noticed the same thing, but sticking the CFI immediately after the
declaration didn't read well either. I was considering moving it to
the bottom of the loop instead of that.
I was contemplating that too, but kept it at the top after seeing quite a few
examples of that in other contrib modules (like amcheck/verify_nbtree.c and
pg_visibility/pg_visibility.c). I don't have any strong feelings either way,
I'm happy to move it last.
You could keep it at the top, but then I'd be inclined to split up
the existing code:
int trgindex;
CHECK_FOR_INTERRUPTS();
/* Get index of next trigram */
trgindex = trg2indexes[i];
/* Update last position of this trigram */
...
What's annoying me about the one-liner fix is that it makes it
look like CFI is part of the "Get index" action.
regards, tom lane
On 2 Sep 2022, at 15:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
What's annoying me about the one-liner fix is that it makes it
look like CFI is part of the "Get index" action.
Thats a good point, I'll split the code up to make it clearer.
--
Daniel Gustafsson https://vmware.com/
On 2 Sep 2022, at 15:22, Daniel Gustafsson <daniel@yesql.se> wrote:
On 2 Sep 2022, at 15:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
What's annoying me about the one-liner fix is that it makes it
look like CFI is part of the "Get index" action.Thats a good point, I'll split the code up to make it clearer.
Done that way and pushed, thanks!
--
Daniel Gustafsson https://vmware.com/