Normalize SQL IN(?, ?, ...) statements to "in(?)" to reduce cardinality of db.statement attribute #10564
Conversation
swar8080
force-pushed
the
sql-in-statement-normalization
branch
from
bf176d0
to
5723c1a
Compare
…ity of span metrics using db.statement as an attribute
swar8080
force-pushed
the
sql-in-statement-normalization
branch
from
5723c1a
to
c8dbb13
Compare
it's precompiled - and it doesn't use pathological patterns |
swar8080
changed the title
|
my bad, this is actually causing a stack overflow for an IN statement with 2000 values ill see if there's a different approach |
|
I switched to a simpler regular expression that a ReDos checker said is linear complexity. It no longer causes a stack overflow, which I added a test case for Also, I did some profiles to compare performance on an IN statement that has 10k values. Didn't see a noticeable difference for what it's worth Without the normalization: With the normalization: |
swar8080
changed the title
| @@ -52,6 +54,9 @@ WHITESPACE = [ \t\r\n]+ | |||
| // max length of the sanitized statement - SQLs longer than this will be trimmed | |||
| static final int LIMIT = 32 * 1024; | |||
|
|
|||
| private static final Pattern IN_STATEMENT_PATTERN = Pattern.compile("\\sin\\s*\\(\\s*\\?[\\s?,]*?\\)", Pattern.CASE_INSENSITIVE); | |||
There was a problem hiding this comment.
I think this also matches inputs like in (?,,,???) perhaps using "(\\sin\\s*)\\(\\s*\\?\\s*(,\\s*\\?\\s*)*\\)" and replacing with "$1(?)" would be better. These regular expressions are hard to parse, maybe we should try to document them to make them easier to understand?
There was a problem hiding this comment.
Unfortunately the (,\\s*\\?\\s*)* part of that pattern causes a stack overflow for IN statements with many values
Let me know if matching invalid syntax like in (?,,,???) is ok since it'd hide info helpful for debugging bad queries. I'd guess that info's available in most sql library stack traces though
Also, added some documentation
There was a problem hiding this comment.
Unfortunately the (,\s*\?\s*)* part of that pattern causes a stack overflow for IN statements with many values
using a possessive quantifier should fix this, try "(\\sin\\s*)\\(\\s*\\?\\s*(,\\s*\\?\\s*)*+\\)"
There was a problem hiding this comment.
thanks, that solves the problem
github-actions
bot
added
the
test native
laurit
removed
the
test native
|
closing and reopening to trigger checks |
| @@ -52,6 +54,10 @@ WHITESPACE = [ \t\r\n]+ | |||
| // max length of the sanitized statement - SQLs longer than this will be trimmed | |||
| static final int LIMIT = 32 * 1024; | |||
|
|
|||
| // Match on "IN(?, ?, ...)" | |||
| private static final Pattern IN_STATEMENT_PATTERN = Pattern.compile("(\\sin\\s*)\\(\\s*\\?\\s*(,\\s*\\?\\s*)*+\\)", Pattern.CASE_INSENSITIVE); | |||
| private static final String IN_STATEMENT_NORMALIZED = " in(?)"; | |||
There was a problem hiding this comment.
| private static final String IN_STATEMENT_NORMALIZED = " in(?)"; | |
| private static final String IN_STATEMENT_NORMALIZED = "$1(?)"; |
Sanitizer does not change case or remove whitespace from the original query. Lets keep in as it was in the original query. longInStatementDoesntCauseStackOverflow will break after this change as there is a space between in and ( that is currently removed.
There was a problem hiding this comment.
Got it, I updated it to preserve case and whitespace, and updated the test cases to check for that. I didn't add a test case for more than one space between IN and ( since strings like IN (?) get sanitized to IN (?)
Also switched to a non-capturing group for matching on the part in-between the brackets as a small optimization
swar8080
force-pushed
the
sql-in-statement-normalization
branch
from
f762e93
to
c52df11
Compare
|
HI @laurit, let me know if anything else should be done here Would be great to have this in 2.2.0 |
Went for a simple implementation, but let me know if any of the following would be preferred:
Closes #10442