SQL: Skip PL/SQL selection directives and add sanity check for inquiry directive size #3654

techee · 2023-03-02T18:17:40Z

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. In addition, as the double-quoted
string is getting bigger and bigger and tested against
inquiry directives by

lookupCaseKeyword (vStringValue (string), Lang_sql)

it takes huge amount of time to perform the repeated
lookups in the hash table.

This patch skips all the code between $IF and $END because
we 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
$IF and $END as these may be conditional variables in the
$IF block.

Another issue is that
$$ typed anywhere in the code, e.g. by accident, makes the rest of the
code appear to the parser as a dollar-quoted string which can be thousands
of bytes long. In this case lookupCaseKeyword() is called repeatedly on
this 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 for
strings 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.

codecov · 2023-03-02T18:23:08Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (6f4a48a) 82.82% compared to head (b8d6a4a) 82.87%.

❗ Current head b8d6a4a differs from pull request most recent head 78a7342. Consider uploading reports for the commit 78a7342 to get more accurate results

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

Impacted Files	Coverage Δ
parsers/sql.c	`76.29% <100.00%> (+0.58%)`	⬆️

... 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

b4n · 2023-03-12T21:05:53Z

This patch skips all the code between $IF and $END because
we can't know which of the branches to take and this is the
easiest way to avoid invalid code.

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.

techee · 2023-03-12T22:58:50Z

Shouldn't it rather take one branch, any branch?

Yeah, you are right, I'll have a look at it when I have time.

techee · 2023-03-13T22:45:02Z

@b4n Done.

techee · 2023-03-14T19:18:37Z

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.

techee · 2023-03-14T19:21:34Z

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.

masatake · 2023-03-14T19:23:08Z

(I'm catching up now.)

masatake · 2023-03-20T14:37:42Z

parsers/sql.c

@@ -294,6 +300,7 @@ static const keywordTable SqlKeywordTable [] = {
 	{ "drop",							KEYWORD_drop			      },
 	{ "else",							KEYWORD_else			      },
 	{ "elseif",							KEYWORD_elseif			      },
+	{ "elsif",							KEYWORD_elsif			      },


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. */ ...

Done, thanks for noticing. I'll then squash the commits when you think there's not more work to do.

Yes, please. Could you remove c3b0324 and 9f532d3?

masatake · 2023-03-21T00:30:17Z

parsers/sql.c

@@ -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),


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!

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.

masatake · 2023-03-21T16:27:18Z

Thank you!

masatake self-assigned this Mar 3, 2023

masatake mentioned this pull request Mar 3, 2023

Verilog: all text macro map to new kindDefinition:define #3653

Merged

techee mentioned this pull request Mar 11, 2023

Find window loses focus geany/geany-osx#42

Closed

techee force-pushed the sql_preproc branch from 7d8742b to c3b0324 Compare March 13, 2023 22:16

masatake requested changes Mar 20, 2023

View reviewed changes

masatake reviewed Mar 21, 2023

View reviewed changes

techee force-pushed the sql_preproc branch from b8d6a4a to 78a7342 Compare March 21, 2023 14:37

masatake merged commit 1b8c941 into universal-ctags:master Mar 21, 2023

techee mentioned this pull request Mar 22, 2023

ctags: Add quick path for looking up too long strings in the keyword table geany/geany#3433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Skip PL/SQL selection directives and add sanity check for inquiry directive size #3654

SQL: Skip PL/SQL selection directives and add sanity check for inquiry directive size #3654

techee commented Mar 2, 2023

codecov bot commented Mar 2, 2023 •

edited

Loading

b4n commented Mar 12, 2023

techee commented Mar 12, 2023

techee commented Mar 13, 2023

techee commented Mar 14, 2023

techee commented Mar 14, 2023

masatake commented Mar 14, 2023

masatake Mar 20, 2023 •

edited

Loading

techee Mar 21, 2023

masatake Mar 21, 2023

techee Mar 21, 2023

masatake Mar 21, 2023

techee Mar 21, 2023

techee Mar 21, 2023

masatake commented Mar 21, 2023

SQL: Skip PL/SQL selection directives and add sanity check for inquiry directive size #3654

SQL: Skip PL/SQL selection directives and add sanity check for inquiry directive size #3654

Conversation

techee commented Mar 2, 2023

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

b4n commented Mar 12, 2023

techee commented Mar 12, 2023

techee commented Mar 13, 2023

techee commented Mar 14, 2023

techee commented Mar 14, 2023

masatake commented Mar 14, 2023

masatake Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

techee Mar 21, 2023

Choose a reason for hiding this comment

masatake Mar 21, 2023

Choose a reason for hiding this comment

techee Mar 21, 2023

Choose a reason for hiding this comment

masatake Mar 21, 2023

Choose a reason for hiding this comment

techee Mar 21, 2023

Choose a reason for hiding this comment

techee Mar 21, 2023

Choose a reason for hiding this comment

masatake commented Mar 21, 2023

codecov bot commented Mar 2, 2023 •

edited

Loading

masatake Mar 20, 2023 •

edited

Loading