SQL: Skip PL/SQL selection directives and add sanity check for inquiry directive size #3654
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3654 +/- ##
==========================================
+ Coverage 82.82% 82.87% +0.04%
==========================================
Files 223 223
Lines 54502 54561 +59
==========================================
+ Hits 45143 45219 +76
+ Misses 9359 9342 -17
... and 6 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Shouldn't it rather take one branch, any branch? I don't know the syntax (and have been too lazy to look it up any closely), but if it's anything similar to e.g. C preprocessor taking no branch is the best way to get invalid syntax. I'd then take the first branch (because it's likely the easiest solution), and if it's not what's wanted, well… at least it gives something. |
Yeah, you are right, I'll have a look at it when I have time. |
|
@b4n Done. |
|
I've created #3664 which might be a better and more universal fix for the original "slow parser" problem as it fixes it at the level of the keyword table so individual parsers don't have to worry about it any more. |
Just to be clear, it replaces the patch performing the string length check if ((string->length < 30 && KEYWORD_inquiry_directive == lookupCaseKeyword (vStringValue (string), Lang_sql))but the PL/SQL selection directive is still worth implementing. |
|
(I'm catching up now.) |
| @@ -294,6 +300,7 @@ static const keywordTable SqlKeywordTable [] = { | |||
| { "drop", KEYWORD_drop }, | |||
| { "else", KEYWORD_else }, | |||
| { "elseif", KEYWORD_elseif }, | |||
| { "elsif", KEYWORD_elsif }, | |||
There was a problem hiding this comment.
Could you add an entry for KEYWORD_elsif to
static struct SqlReservedWord SqlReservedWord [SQLKEYWORD_COUNT] = {
/*
* RESERVED_BIT: MYSQL & POSTGRESQL&SQL2016&SQL2011&SQL92 & ORACLE11g&PLSQL & SQLANYWERE
*
* { 0 } means we have not inspect whether the keyword is reserved or not.
*/
...
There was a problem hiding this comment.
Done, thanks for noticing. I'll then squash the commits when you think there's not more work to do.
parsers/sql.c
Outdated
| @@ -797,8 +797,8 @@ static tokenType parseDollarQuote (vString *const string, const int delimiter, i | |||
| { | |||
| vStringPut (string, c); | |||
| if (empty_tag | |||
| && (KEYWORD_inquiry_directive == lookupCaseKeyword (vStringValue (string), | |||
| Lang_sql) | |||
| && ((string->length < 30 && KEYWORD_inquiry_directive == lookupCaseKeyword (vStringValue (string), | |||
There was a problem hiding this comment.
Could you use '#define' for the 30 with a good name?
Finding a good name is not easy for me, but it may be easy for you!
There was a problem hiding this comment.
The patch checking string->length here can be dropped completely now. It is the reason why I created #3664 which is more universal and works for every parser automatically.
PLSQL selection directives are of the form $IF boolean_static_expression $THEN text [ $ELSIF boolean_static_expression $THEN text ]... [ $ELSE text ] $END In addition, boolean_static_expression conditions like $$MY_CUSTOM_VAR > 3 can appear which confuses the current parser because it thinks $$ is the start of dollar-quoted string and drops the rest of the code. This patch keeps only the $IF branch and skips all the code between $ELSE or $ELSIF and $END. In addition, this patch also drops $ from all identifiers starting with $$ behind $IF and $ELSIF as these may be conditional variables and not dollar-quoted strings.
|
Thank you! |
PLSQL selection directives are of the form
In addition, boolean_static_expression conditions like
can appear which confuses the current parser because it
thinks
$$is the start of dollar-quoted string and dropsthe rest of the code. In addition, as the double-quoted
string is getting bigger and bigger and tested against
inquiry directives by
it takes huge amount of time to perform the repeated
lookups in the hash table.
This patch skips all the code between
$IFand$ENDbecausewe can't know which of the branches to take and this is the
easiest way to avoid invalid code. In addition, this patch
also drops
$from all identifiers starting with$$between$IFand$ENDas these may be conditional variables in the$IFblock.Another issue is that
$$typed anywhere in the code, e.g. by accident, makes the rest of thecode appear to the parser as a dollar-quoted string which can be thousands
of bytes long. In this case
lookupCaseKeyword()is called repeatedly onthis ever increasing string which consumes a lot of time and makes the
parser appear completely unresponsive for large files.
This patch adds a sanity check to perform
lookupCaseKeyword()only forstrings of length smaller than 30 (currently the longest inquiry directive
keyword is 21 characters long so there should be some safe extra margin
even for longer keywords if added in the future).
Fixes #3647.