[tarantool-patches] Re: [PATCH 9/9] sql: make <search condition> accept only boolean

["n.pettik" <korablev@tarantool.org>]

> On 24 Apr 2019, at 00:06, Vladislav Shpilevoy <v.shpilevoy@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)
                                                         &regFree1);
                                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;