[tarantool-patches] Re: [PATCH 9/9] sql: make <search condition> accept only boolean
n.pettik
korablev at tarantool.org
Wed Apr 24 01:01:05 MSK 2019
> On 24 Apr 2019, at 00:06, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> On 23/04/2019 22:59, n.pettik wrote:
>>
>>> 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;
>>
>> One branching worth nothing. Below you suggest to split opcode
>> into two (fix me if I’m wrong), which in turn affects performance way much more.
>
> The places which I proposed to split are called only once per request.
> For example, OP_IfNot with iLimit was used for initial check that it is
> not zero. All the next work with iLimit was being done via special
> opcodes OP_DecrJumpZero and OP_IfNotZero.
>
> On the other hand, if we branch inside OP_IfNot, we branch repeatedly,
> in cycles, many times per request.
>>
>>> 3) It adds one more flag SQL_BOOLREQ, which looks very crutchy.
>>
>> IMHO it is matter of taste. Anyway, removed this flag.
>
> You are right, it would have been, if OP_IfNot/OP_If had already
> had some other flags. But you proposed to change the opcodes
> dramatically, to add a new argument just for some minor cases.
> And as a result it hid a bug with CASE-WHEN search condition.
>
>>> It violates the standard. "Information technology —
>>> Database languages — SQL — Part 2: Foundation (SQL/Foundation)",
>>> 2011, page 230.
>>>
>>> '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.
>>
>> I’ve fixed that.
>
> But you still set OPFLAG_BOOLREQ and SQL_BOOLREQ. Why?
> Also the commit message still describes this flag as a
> key change of the patch.
OMG, sorry, I forgot to apply stashed changes and merge them.
Haste makes waste… I’ve pushed these updates:
(commit message fixed as well)
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 86fc28606..31b724e23 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -727,12 +727,10 @@ codeVectorCompare(Parse * pParse, /* Code generator context */
}
if (opx == TK_EQ) {
sqlVdbeAddOp2(v, OP_IfNot, dest, addrDone);
- sqlVdbeChangeP5(v, OPFLAG_BOOLREQ);
VdbeCoverage(v);
p5 |= SQL_KEEPNULL;
} else if (opx == TK_NE) {
sqlVdbeAddOp2(v, OP_If, dest, addrDone);
- sqlVdbeChangeP5(v, OPFLAG_BOOLREQ);
VdbeCoverage(v);
p5 |= SQL_KEEPNULL;
} else {
@@ -4718,7 +4716,7 @@ exprCodeBetween(Parse * pParse, /* Parsing and code generating context */
* continues straight thru if the expression is false.
*
* If the expression evaluates to NULL (neither true nor false), then
- * take the jump if the flag is SQL_JUMPIFNULL.
+ * take the jump if the jumpIfNull flag is SQL_JUMPIFNULL.
*
* This code depends on the fact that certain token values (ex: TK_EQ)
* are the same as opcode values (ex: OP_Eq) that implement the corresponding
@@ -4727,14 +4725,14 @@ exprCodeBetween(Parse * pParse, /* Parsing and code generating context */
* below verify that the numbers are aligned correctly.
*/
void
-sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int flags)
+sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
{
Vdbe *v = pParse->pVdbe;
int op = 0;
int regFree1 = 0;
int regFree2 = 0;
int r1, r2;
- int jumpIfNull = flags & SQL_JUMPIFNULL;
+
assert(jumpIfNull == SQL_JUMPIFNULL || jumpIfNull == 0);
if (NEVER(v == 0))
return; /* Existence of VDBE checked by caller */
@@ -4870,22 +4868,18 @@ sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int flags)
* continues straight thru if the expression is true.
*
* If the expression evaluates to NULL (neither true nor false) then
- * jump if flags contains SQL_JUMPIFNULL or fall through if it doesn't.
- *
- * IF flags contains SQL_BOOLREQ then OP_If(Not) is supplied with
- * flag OPFLAG_BOOLREQ which forces additional verification of
- * its arguments. It is required to make sure that searching
- * condition is boolean (to disallow queries like ... WHERE 1+1;).
+ * jump if jumpIfNull is SQL_JUMPIFNULL or fall through if jumpIfNull
+ * is 0.
*/
void
-sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int flags)
+sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
{
Vdbe *v = pParse->pVdbe;
int op = 0;
int regFree1 = 0;
int regFree2 = 0;
int r1, r2;
- int jumpIfNull = flags & SQL_JUMPIFNULL;
+
assert(jumpIfNull == SQL_JUMPIFNULL || jumpIfNull == 0);
if (NEVER(v == 0))
return; /* Existence of VDBE checked by caller */
@@ -5050,12 +5044,6 @@ sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int flags)
®Free1);
sqlVdbeAddOp3(v, OP_IfNot, r1, dest,
jumpIfNull != 0);
- /*
- * Make sure that search condition
- * under WHERE clause returns boolean.
- */
- if ((flags & SQL_BOOLREQ) != 0)
- sqlVdbeChangeP5(v, OPFLAG_BOOLREQ);
VdbeCoverage(v);
testcase(regFree1 == 0);
testcase(jumpIfNull == 0);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index b8a1d5f54..07c887bb4 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1785,7 +1785,6 @@ struct Savepoint {
#define SQL_KEEPNULL 0x40 /* Used by vector == or <> */
#define SQL_NULLEQ 0x80 /* NULL=NULL */
#define SQL_NOTNULL 0x90 /* Assert that operands are never NULL */
-#define SQL_BOOLREQ 0x100 /* Argument passed to OP_If must be boolean */
/**
* Return logarithm of tuple count in space.
@@ -2766,7 +2765,6 @@ struct Parse {
#define OPFLAG_SYSTEMSP 0x20 /* OP_Open**: set if space pointer
* points to system space.
*/
-#define OPFLAG_BOOLREQ 0x1000 /* OP_IF(Not): operand must be boolean. */
/**
* Prepare vdbe P5 flags for OP_{IdxInsert, IdxReplace, Update}
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 23a1bda7d..1bac59587 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2537,21 +2537,14 @@ case OP_Once: { /* jump */
break;
}
-/* Opcode: If P1 P2 P3 * P5
+/* Opcode: If P1 P2 P3 * *
*
- * Jump to P2 if the value in register P1 is true. The value
- * is considered true if it is numeric and non-zero. If the value
+ * Jump to P2 if the value in register P1 is true. If the value
* in P1 is NULL then take the jump if and only if P3 is non-zero.
- *
- * In case P5 contains BOOLREQ flag, then argument is supposed
- * to be BOOLEAN. Otherwise, an error is raised. Such check is
- * required to restrict <search condition> used in WHERE and
- * JOIN clauses allowing only boolean values.
*/
-/* Opcode: IfNot P1 P2 P3 * P5
+/* Opcode: IfNot P1 P2 P3 * *
*
- * Jump to P2 if the value in register P1 is False. The value
- * is considered false if it has a numeric value of zero. If the value
+ * Jump to P2 if the value in register P1 is False. If the value
* in P1 is NULL then take the jump if and only if P3 is non-zero.
*/
case OP_If: /* jump, in1 */
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 93020b148..19ee2d03a 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -4349,8 +4349,7 @@ sqlWhereBegin(Parse * pParse, /* The parser context */
if (nTabList == 0
|| sqlExprIsConstantNotJoin(sWLB.pWC->a[ii].pExpr)) {
sqlExprIfFalse(pParse, sWLB.pWC->a[ii].pExpr,
- pWInfo->iBreak,
- SQL_JUMPIFNULL | SQL_BOOLREQ);
+ pWInfo->iBreak, SQL_JUMPIFNULL);
sWLB.pWC->a[ii].wtFlags |= TERM_CODED;
}
}
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 5ee2efce7..a453fe979 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1613,8 +1613,7 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W
*/
continue;
}
- sqlExprIfFalse(pParse, pE, addrCont,
- SQL_JUMPIFNULL | SQL_BOOLREQ);
+ sqlExprIfFalse(pParse, pE, addrCont, SQL_JUMPIFNULL);
if (skipLikeAddr)
sqlVdbeJumpHere(v, skipLikeAddr);
pTerm->wtFlags |= TERM_CODED;
More information about the Tarantool-patches
mailing list