Skip to content

Commit 59ee5a7

Browse files
jhnwkmnmarkus456
jhnwkmn
authored and
markus456
committed
MXS-969: Report user var modifications properly
User variable modifications are now reported as QUERY_TYPE_USERVAR_WRITE and not as QUERY_TYPE_GSYSVAR_WRITE as earlier.
1 parent b594bdc commit 59ee5a7

File tree

9 files changed

+280
-95
lines changed

9 files changed

+280
-95
lines changed

‎query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc

+124-10
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ typedef struct parsing_info_st
8787
static THD* get_or_create_thd_for_parsing(MYSQL* mysql, char* query_str);
8888
static unsigned long set_client_flags(MYSQL* mysql);
8989
static bool create_parse_tree(THD* thd);
90-
static uint32_t resolve_query_type(THD* thd);
90+
static uint32_t resolve_query_type(parsing_info_t*, THD* thd);
9191
static bool skygw_stmt_causes_implicit_commit(LEX* lex, int* autocommit_stmt);
9292

9393
static int is_autocommit_stmt(LEX* lex);
@@ -167,7 +167,7 @@ uint32_t qc_get_type(GWBUF* querybuf)
167167
/** Find out the query type */
168168
if (mysql != NULL)
169169
{
170-
qtype = resolve_query_type((THD *) mysql->thd);
170+
qtype = resolve_query_type(pi, (THD *) mysql->thd);
171171
}
172172
}
173173
}
@@ -435,9 +435,102 @@ static bool create_parse_tree(THD* thd)
435435
return failp;
436436
}
437437

438+
/**
439+
* Sniff whether the statement is
440+
*
441+
* SET ROLE ...
442+
* SET NAMES ...
443+
* SET PASSWORD ...
444+
* SET CHARACTER ...
445+
*
446+
* Depending on what kind of SET statement it is, the parser of the embedded
447+
* library creates instances of set_var_user, set_var, set_var_password,
448+
* set_var_role, etc. that all are derived from set_var_base. However, there
449+
* is no type-information available in set_var_base, which is the type of the
450+
* instances when accessed from the lexer. Consequently, we cannot know what
451+
* kind of statment it is based on that, only whether it is a system variable
452+
* or not.
453+
*
454+
* Consequently, we just look at the string and deduce whether it is a
455+
* set [ROLE|NAMES|PASSWORD|CHARACTER] statement.
456+
*/
457+
bool is_set_specific(const char* s)
458+
{
459+
bool rv = false;
460+
461+
// Remove space from the beginning.
462+
while (isspace(*s))
463+
{
464+
++s;
465+
}
466+
467+
const char* token = s;
468+
469+
// Find next non-space character.
470+
while (!isspace(*s) && (*s != 0))
471+
{
472+
++s;
473+
}
474+
475+
if (s - token == 3) // Might be "set"
476+
{
477+
if (strncasecmp(token, "set", 3) == 0)
478+
{
479+
// YES it was!
480+
while (isspace(*s))
481+
{
482+
++s;
483+
}
484+
485+
token = s;
486+
487+
while (!isspace(*s) && (*s != 0) && (*s != '='))
488+
{
489+
++s;
490+
}
491+
492+
if (s - token == 4) // Might be "role"
493+
{
494+
if (strncasecmp(token, "role", 4) == 0)
495+
{
496+
// YES it was!
497+
rv = true;
498+
}
499+
}
500+
else if (s - token == 5) // Might be "names"
501+
{
502+
if (strncasecmp(token, "names", 5) == 0)
503+
{
504+
// YES it was!
505+
rv = true;
506+
}
507+
}
508+
else if (s - token == 8) // Might be "password
509+
{
510+
if (strncasecmp(token, "password", 8) == 0)
511+
{
512+
// YES it was!
513+
rv = true;
514+
}
515+
}
516+
else if (s - token == 9) // Might be "character"
517+
{
518+
if (strncasecmp(token, "character", 9) == 0)
519+
{
520+
// YES it was!
521+
rv = true;
522+
}
523+
}
524+
}
525+
}
526+
527+
return rv;
528+
}
529+
438530
/**
439531
* Detect query type by examining parsed representation of it.
440532
*
533+
* @param pi The parsing info.
441534
* @param thd MariaDB thread context.
442535
*
443536
* @return Copy of query type value.
@@ -449,7 +542,7 @@ static bool create_parse_tree(THD* thd)
449542
* the resulting type may be different.
450543
*
451544
*/
452-
static uint32_t resolve_query_type(THD* thd)
545+
static uint32_t resolve_query_type(parsing_info_t *pi, THD* thd)
453546
{
454547
qc_query_type_t qtype = QUERY_TYPE_UNKNOWN;
455548
uint32_t type = QUERY_TYPE_UNKNOWN;
@@ -565,7 +658,33 @@ static uint32_t resolve_query_type(THD* thd)
565658
else if (lex->sql_command == SQLCOM_SET_OPTION)
566659
{
567660
/** Either user- or system variable write */
568-
type |= QUERY_TYPE_GSYSVAR_WRITE;
661+
if (is_set_specific(pi->pi_query_plain_str))
662+
{
663+
type |= QUERY_TYPE_GSYSVAR_WRITE;
664+
}
665+
else
666+
{
667+
List_iterator<set_var_base> ilist(lex->var_list);
668+
size_t n = 0;
669+
670+
while (set_var_base *var = ilist++)
671+
{
672+
if (var->is_system())
673+
{
674+
type |= QUERY_TYPE_GSYSVAR_WRITE;
675+
}
676+
else
677+
{
678+
type |= QUERY_TYPE_USERVAR_WRITE;
679+
}
680+
++n;
681+
}
682+
683+
if (n == 0)
684+
{
685+
type |= QUERY_TYPE_GSYSVAR_WRITE;
686+
}
687+
}
569688
}
570689

571690
goto return_qtype;
@@ -812,12 +931,7 @@ static uint32_t resolve_query_type(THD* thd)
812931

813932
/** User-defined variable modification */
814933
case Item_func::SUSERVAR_FUNC:
815-
/**
816-
* Really it is user variable but we
817-
* don't separate sql variables atm.
818-
* 15.9.14
819-
*/
820-
func_qtype |= QUERY_TYPE_GSYSVAR_WRITE;
934+
func_qtype |= QUERY_TYPE_USERVAR_WRITE;
821935
MXS_DEBUG("%lu [resolve_query_type] "
822936
"functype SUSERVAR_FUNC, user "
823937
"variable write.",

‎query_classifier/qc_sqlite/qc_sqlite.c

+77-47
Original file line numberDiff line numberDiff line change
@@ -746,8 +746,7 @@ static void update_affected_fields(QC_SQLITE_INFO* info,
746746
{
747747
if ((prev_token == TK_EQ) && (pos == QC_TOKEN_LEFT))
748748
{
749-
// Yes, QUERY_TYPE_USERVAR_WRITE is currently not available.
750-
info->types |= QUERY_TYPE_GSYSVAR_WRITE;
749+
info->types |= QUERY_TYPE_USERVAR_WRITE;
751750
}
752751
else
753752
{
@@ -2036,6 +2035,7 @@ void maxscaleSet(Parse* pParse, int scope, mxs_set_t kind, ExprList* pList)
20362035
ss_dassert(info);
20372036

20382037
info->status = QC_QUERY_PARSED;
2038+
info->types = 0; // Reset what was set in maxscaleKeyword
20392039

20402040
switch (kind)
20412041
{
@@ -2053,10 +2053,6 @@ void maxscaleSet(Parse* pParse, int scope, mxs_set_t kind, ExprList* pList)
20532053

20542054
case MXS_SET_VARIABLES:
20552055
{
2056-
// TODO: qc_mysqlembedded sets this bit on, without checking what
2057-
// TODO: kind of variable it is.
2058-
info->types = QUERY_TYPE_GSYSVAR_WRITE;
2059-
20602056
for (int i = 0; i < pList->nExpr; ++i)
20612057
{
20622058
const struct ExprList_item* pItem = &pList->a[i];
@@ -2065,74 +2061,108 @@ void maxscaleSet(Parse* pParse, int scope, mxs_set_t kind, ExprList* pList)
20652061
{
20662062
case TK_CHARACTER:
20672063
case TK_NAMES:
2064+
info->types |= QUERY_TYPE_GSYSVAR_WRITE;
20682065
break;
20692066

20702067
case TK_EQ:
20712068
{
20722069
const Expr* pEq = pItem->pExpr;
2073-
const Expr* pVariable = pEq->pLeft;
2070+
const Expr* pVariable;
20742071
const Expr* pValue = pEq->pRight;
20752072

2076-
// pVariable is either TK_DOT, TK_VARIABLE or TK_ID. If it's TK_DOT,
2077-
// then pVariable->pLeft is either TK_VARIABLE or TK_ID and pVariable->pRight
2073+
// pEq->pLeft is either TK_DOT, TK_VARIABLE or TK_ID. If it's TK_DOT,
2074+
// then pEq->pLeft->pLeft is either TK_VARIABLE or TK_ID and pEq->pLeft->pRight
20782075
// is either TK_DOT, TK_VARIABLE or TK_ID.
20792076

2080-
// Set pVariable to point to the rightmost part of the name.
2077+
// Find the left-most part.
2078+
pVariable = pEq->pLeft;
20812079
while (pVariable->op == TK_DOT)
20822080
{
2083-
pVariable = pVariable->pRight;
2081+
pVariable = pVariable->pLeft;
2082+
ss_dassert(pVariable);
20842083
}
20852084

2086-
ss_dassert((pVariable->op == TK_VARIABLE) || (pVariable->op == TK_ID));
2087-
2085+
// Check what kind of variable it is.
2086+
size_t n_at = 0;
20882087
const char* zName = pVariable->u.zToken;
20892088

20902089
while (*zName == '@')
20912090
{
2091+
++n_at;
20922092
++zName;
20932093
}
20942094

2095-
// As pVariable points to the rightmost part, we'll catch both
2096-
// "autocommit" and "@@global.autocommit".
2097-
if (strcasecmp(zName, "autocommit") == 0)
2095+
if (n_at == 1)
20982096
{
2099-
int enable = -1;
2097+
info->types |= QUERY_TYPE_USERVAR_WRITE;
2098+
}
2099+
else
2100+
{
2101+
info->types |= QUERY_TYPE_GSYSVAR_WRITE;
2102+
}
21002103

2101-
switch (pValue->op)
2102-
{
2103-
case TK_INTEGER:
2104-
if (pValue->u.iValue == 1)
2105-
{
2106-
enable = 1;
2107-
}
2108-
else if (pValue->u.iValue == 0)
2109-
{
2110-
enable = 0;
2111-
}
2112-
break;
2104+
// Set pVariable to point to the rightmost part of the name.
2105+
pVariable = pEq->pLeft;
2106+
while (pVariable->op == TK_DOT)
2107+
{
2108+
pVariable = pVariable->pRight;
2109+
}
2110+
2111+
ss_dassert((pVariable->op == TK_VARIABLE) || (pVariable->op == TK_ID));
21132112

2114-
case TK_ID:
2115-
enable = string_to_truth(pValue->u.zToken);
2116-
break;
2113+
if (n_at != 1)
2114+
{
2115+
// If it's not a user-variable we need to check whether it might
2116+
// be 'autocommit'.
2117+
const char* zName = pVariable->u.zToken;
21172118

2118-
default:
2119-
break;
2119+
while (*zName == '@')
2120+
{
2121+
++zName;
21202122
}
21212123

2122-
switch (enable)
2124+
// As pVariable points to the rightmost part, we'll catch both
2125+
// "autocommit" and "@@global.autocommit".
2126+
if (strcasecmp(zName, "autocommit") == 0)
21232127
{
2124-
case 0:
2125-
info->types |= QUERY_TYPE_BEGIN_TRX;
2126-
info->types |= QUERY_TYPE_DISABLE_AUTOCOMMIT;
2127-
break;
2128-
2129-
case 1:
2130-
info->types |= QUERY_TYPE_ENABLE_AUTOCOMMIT;
2131-
info->types |= QUERY_TYPE_COMMIT;
2132-
break;
2133-
2134-
default:
2135-
break;
2128+
int enable = -1;
2129+
2130+
switch (pValue->op)
2131+
{
2132+
case TK_INTEGER:
2133+
if (pValue->u.iValue == 1)
2134+
{
2135+
enable = 1;
2136+
}
2137+
else if (pValue->u.iValue == 0)
2138+
{
2139+
enable = 0;
2140+
}
2141+
break;
2142+
2143+
case TK_ID:
2144+
enable = string_to_truth(pValue->u.zToken);
2145+
break;
2146+
2147+
default:
2148+
break;
2149+
}
2150+
2151+
switch (enable)
2152+
{
2153+
case 0:
2154+
info->types |= QUERY_TYPE_BEGIN_TRX;
2155+
info->types |= QUERY_TYPE_DISABLE_AUTOCOMMIT;
2156+
break;
2157+
2158+
case 1:
2159+
info->types |= QUERY_TYPE_ENABLE_AUTOCOMMIT;
2160+
info->types |= QUERY_TYPE_COMMIT;
2161+
break;
2162+
2163+
default:
2164+
break;
2165+
}
21362166
}
21372167
}
21382168

‎query_classifier/test/expected.sql

-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ QUERY_TYPE_WRITE
44
QUERY_TYPE_WRITE
55
QUERY_TYPE_WRITE|QUERY_TYPE_COMMIT
66
QUERY_TYPE_WRITE|QUERY_TYPE_CREATE_TMP_TABLE
7-
QUERY_TYPE_GSYSVAR_WRITE
87
QUERY_TYPE_READ|QUERY_TYPE_SYSVAR_READ
98
QUERY_TYPE_READ|QUERY_TYPE_USERVAR_READ
109
QUERY_TYPE_GSYSVAR_WRITE|QUERY_TYPE_ENABLE_AUTOCOMMIT|QUERY_TYPE_COMMIT

‎query_classifier/test/input.sql

-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ insert into tst values ("Jane","Doe"),("Daisy","Duck"),("Marie","Curie");
44
update tst set fname="Farmer", lname="McDonald" where lname="%Doe" and fname="John";
55
create table tmp as select * from t1;
66
create temporary table tmp as select * from t1;
7-
/*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */;
87
select @@server_id;
98
select @OLD_SQL_NOTES;
109
SET autocommit=1;

‎query_classifier/test/qc_sqlite_unsupported.test

+9
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,12 @@ SET @x:= (SELECT h FROM t1 WHERE (a,b,c,d,e,f,g)=(1,2,3,4,5,6,7));
2020
# REMOVE: expr(A) ::= LP(B) expr(X) RP(E). {A.pExpr = X.pExpr; spanSet(&A,&B,&E);}
2121
# REMOVE: expr(A) ::= LP expr(X) COMMA(OP) expr(Y) RP. {spanBinaryExpr(&A,pParse,@OP,&X,&Y);}
2222
# ADD : expr(A) ::= LP exprlist RP. { ... }
23+
24+
SET @`a b`='hello';
25+
set @`test`=1;
26+
set @"tEST"=3;
27+
set @`TeST`=4;
28+
# warning: qc_sqlite: Statement was classified only based on keywords
29+
# (Sqlite3 error: SQL logic error or missing database, unrecognized token: "@"): "set @=4"
30+
#
31+
# sqlite3GetToken needs to be modified to accept a quoted variable name.

0 commit comments

Comments
 (0)