From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 640BE294B5 for ; Mon, 22 Apr 2019 14:02:20 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id DhHtozjjx9bL for ; Mon, 22 Apr 2019 14:02:20 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5CFE3293D4 for ; Mon, 22 Apr 2019 14:02:19 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 9/9] sql: make accept only boolean References: <103cef3d31c59d1b869a7675a01ed2e6279a47ef.1555252410.git.korablev@tarantool.org> <6DC565F1-23AA-49CA-BB2C-AA60EE9E3593@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 22 Apr 2019 21:02:16 +0300 MIME-Version: 1.0 In-Reply-To: <6DC565F1-23AA-49CA-BB2C-AA60EE9E3593@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "n.pettik" , tarantool-patches@freelists.org Thanks for the fixes! On 18/04/2019 20:55, n.pettik wrote: > >>> is a predicate used as a part of WHERE and >>> JOIN clauses. ANSI SQL states that 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 , >> >> 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. ::= | ::= NULLIF | COALESCE { }... ::= | ::= CASE ... [ ] END ::= CASE ... [ ] END ::= WHEN THEN ::= WHEN THEN 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) {