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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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) {