[tarantool-patches] Re: [PATCH 9/9] sql: make <search condition> accept only boolean
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Apr 22 21:02:16 MSK 2019
Thanks for the fixes!
On 18/04/2019 20:55, n.pettik wrote:
>
>>> <search condition> is a predicate used as a part of WHERE and
>>> JOIN clauses. ANSI SQL states that <search condition> must
>>> accept only boolean arguments. In our SQL it is implemented as
>>> bytecode instruction OP_If which in turn carries out logic of
>>> conditional jump. Since it can be involved in executing other routines
>>> different from <search condition>,
>>
>> 1. Which other routines? What is a valid case of OP_If with non-boolean
>> value in check?
>
> For instance, to verify that register containing LIMIT value is > 0.
Yes, and this is almost the only case. What is more, it happens only once
per request, to check if LIMIT == 0 initially. Further it is decremented
and checked via OP_IfNotZero and OP_DecrJumpZero.
> It is quite hard to track values which come to this opcode, so we
> can’t be sure that it always accepts booleans.
It is hard, but without it
1) You can't be sure, that really all the search conditions
are checked to be booleans;
2) It makes OP_If/IfNot slower, and they are called repeatedly in
requests;
3) It adds one more flag SQL_BOOLREQ, which looks very crutchy.
I tried myself to disable non-bool for OP_If/IfNot, and I fixed
LIMIT cases. Then I've found CASE-WHEN also uses OP_If/IfNot in
this distorted way.
tarantool> box.execute("SELECT CASE WHEN 0 THEN 'zero' WHEN 1 THEN 'one' END;")
---
- metadata:
- name: CASE WHEN 0 THEN 'zero' WHEN 1 THEN 'one' END
type: string
rows:
- ['one']
...
It violates the standard. "Information technology —
Database languages — SQL — Part 2: Foundation (SQL/Foundation)",
2011, page 230.
<case expression> ::= <case abbreviation> | <case specification>
<case abbreviation> ::=
NULLIF <left paren> <value expression> <comma> <value expression> <right paren>
| COALESCE <left paren> <value expression>
{ <comma> <value expression> }... <right paren>
<case specification> ::= <simple case> | <searched case>
<simple case> ::= CASE <case operand> <simple when clause>... [ <else clause> ] END
<searched case> ::= CASE <searched when clause>... [ <else clause> ] END
<simple when clause> ::= WHEN <when operand list> THEN <result>
<searched when clause> ::= WHEN <search condition> THEN <result>
Our case is 'case expression' -> 'case specification' ->
'searched case' -> 'searched when clause' ->
'search condition' -> 'boolean'.
'WHEN' is a search condition, but I've used '1', not 'true'.
Also I tested it on PostgreSQL - they raise an error, so it is
both standard and practically used way.
Below are my fixes for LIMIT and a small obvious refactoring,
but they are *not on the branch* - not all the tests pass when I
start banning non-bools in OP_If/IfNot. And I do not like these
temporary registers. Probably we could somehow reuse OP_IfNotZero,
for example, via having it refactored to OP_CheckZero which would
be able to jump both if the target is zero and if not. For example,
it could look like this:
/**
* P1 - a register with value to check if it equals zero.
* P3 - if 1, then check r[P1] == 0, if 0, then check r[P1] != 0.
* If the check is successful, then jump to P2.
*
* if (r[P1] == 0) == P3 then jump p2
*/
But I do not know what to do with its current version decrementing
r[P1]. My ideas were about splitting 'decrement' operation from
zero-checking. Then we could do these replacements:
OP_IfNotZero -> OP_CheckZero, OP_Decrement
OP_DecrJumpZero -> OP_Decrement, OP_CheckZero
OP_IfNot -> OP_CheckZero (where it is used inappropriately)
And now we drop OP_IfNotZero and OP_DecrJumpZero, and forbid
non-bool in OP_IfNot/OP_If. Perhaps we will find more standard
violations.
==================================================================
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3c4cde2c8..86fc28606 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1094,24 +1094,16 @@ sqlPExprAddSelect(Parse * pParse, Expr * pExpr, Select * pSelect)
* LEFT JOIN, then we cannot determine at compile-time whether or not
* is it true or false, so always return 0.
*/
-static int
+static inline bool
exprAlwaysTrue(Expr * p)
{
- if (ExprHasProperty(p, EP_FromJoin))
- return 0;
- if (p->op == TK_TRUE)
- return 1;
- return 0;
+ return !ExprHasProperty(p, EP_FromJoin) && p->op == TK_TRUE;
}
-static int
+static inline bool
exprAlwaysFalse(Expr * p)
{
- if (ExprHasProperty(p, EP_FromJoin))
- return 0;
- if (p->op == TK_FALSE)
- return 1;
- return 0;
+ return !ExprHasProperty(p, EP_FromJoin) && p->op == TK_FALSE;
}
struct Expr *
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 428e3a92f..3680b6f2b 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -769,8 +769,10 @@ pushOntoSorter(Parse * pParse, /* Parser context */
pParse->nMem += pSort->nOBSat;
nKey = nExpr - pSort->nOBSat + bSeq;
if (bSeq) {
- addrFirst =
- sqlVdbeAddOp1(v, OP_IfNot, regBase + nExpr);
+ int r1 = sqlGetTempReg(pParse);
+ sqlVdbeAddOp2(v, OP_Integer, 0, r1);
+ addrFirst = sqlVdbeAddOp3(v, OP_Eq, r1, 0, regBase + nExpr);
+ sqlReleaseTempReg(pParse, r1);
} else {
addrFirst =
sqlVdbeAddOp1(v, OP_SequenceTest,
@@ -799,8 +801,10 @@ pushOntoSorter(Parse * pParse, /* Parser context */
pSort->labelBkOut);
sqlVdbeAddOp1(v, OP_ResetSorter, pSort->iECursor);
if (iLimit) {
- sqlVdbeAddOp2(v, OP_IfNot, iLimit,
- pSort->labelDone);
+ int r1 = sqlGetTempReg(pParse);
+ sqlVdbeAddOp2(v, OP_Integer, 0, r1);
+ sqlVdbeAddOp3(v, OP_Eq, r1, pSort->labelDone, iLimit);
+ sqlReleaseTempReg(pParse, r1);
VdbeCoverage(v);
}
sqlVdbeJumpHere(v, addrFirst);
@@ -2115,10 +2119,10 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
P4_STATIC);
sqlVdbeResolveLabel(v, positive_limit_label);
- sqlReleaseTempReg(pParse, r1);
VdbeCoverage(v);
VdbeComment((v, "LIMIT counter"));
- sqlVdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
+ sqlVdbeAddOp3(v, OP_Eq, r1, iBreak, iLimit);
+ sqlReleaseTempReg(pParse, r1);
VdbeCoverage(v);
if ((p->selFlags & SF_SingleRow) != 0) {
@@ -2673,10 +2677,11 @@ multiSelect(Parse * pParse, /* Parsing context */
p->iLimit = pPrior->iLimit;
p->iOffset = pPrior->iOffset;
if (p->iLimit) {
- addr =
- sqlVdbeAddOp1(v, OP_IfNot,
- p->iLimit);
+ int r1 = sqlGetTempReg(pParse);
+ sqlVdbeAddOp2(v, OP_Integer, 0, r1);
+ addr = sqlVdbeAddOp3(v, OP_Eq, r1, 0, p->iLimit);
VdbeCoverage(v);
+ sqlReleaseTempReg(pParse, r1);
VdbeComment((v,
"Jump ahead if LIMIT reached"));
if (p->iOffset) {
More information about the Tarantool-patches
mailing list