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 CE6DF2B78D for ; Tue, 23 Apr 2019 18:01:09 -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 WQjGwU-tx0YU for ; Tue, 23 Apr 2019 18:01:09 -0400 (EDT) Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 68FAB2B785 for ; Tue, 23 Apr 2019 18:01:09 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH 9/9] sql: make accept only boolean From: "n.pettik" In-Reply-To: <4978b03d-c40e-ed4d-8aac-8567327779c3@tarantool.org> Date: Wed, 24 Apr 2019 01:01:05 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <103cef3d31c59d1b869a7675a01ed2e6279a47ef.1555252410.git.korablev@tarantool.org> <6DC565F1-23AA-49CA-BB2C-AA60EE9E3593@tarantool.org> <09F94053-CCE0-49FD-8788-3F0749BF7ED8@tarantool.org> <4978b03d-c40e-ed4d-8aac-8567327779c3@tarantool.org> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > On 24 Apr 2019, at 00:06, Vladislav Shpilevoy = wrote: > On 23/04/2019 22:59, n.pettik wrote: >>=20 >>> On 18/04/2019 20:55, n.pettik wrote: >>>>=20 >>>>>> 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 ,=20 >>>>>=20 >>>>> 1. Which other routines? What is a valid case of OP_If with = non-boolean >>>>> value in check? >>>>=20 >>>> For instance, to verify that register containing LIMIT value is > = 0. >>>=20 >>> Yes, and this is almost the only case. What is more, it happens only = once >>> per request, to check if LIMIT =3D=3D 0 initially. Further it is = decremented >>> and checked via OP_IfNotZero and OP_DecrJumpZero. >>>=20 >>>> It is quite hard to track values which come to this opcode, so we >>>> can=E2=80=99t be sure that it always accepts booleans. >>>=20 >>> It is hard, but without it >>>=20 >>> 1) You can't be sure, that really all the search conditions >>> are checked to be booleans; >>>=20 >>> 2) It makes OP_If/IfNot slower, and they are called repeatedly in >>> requests; >>=20 >> One branching worth nothing. Below you suggest to split opcode >> into two (fix me if I=E2=80=99m wrong), which in turn affects = performance way much more. >=20 > 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. >=20 > On the other hand, if we branch inside OP_IfNot, we branch repeatedly, > in cycles, many times per request. >>=20 >>> 3) It adds one more flag SQL_BOOLREQ, which looks very crutchy. >>=20 >> IMHO it is matter of taste. Anyway, removed this flag. >=20 > 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. >=20 >>> It violates the standard. "Information technology =E2=80=94 >>> Database languages =E2=80=94 SQL =E2=80=94 Part 2: Foundation = (SQL/Foundation)", >>> 2011, page 230. >>>=20 >>> '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. >>>=20 >>> 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. >>=20 >> I=E2=80=99ve fixed that. >=20 > 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=E2=80=A6 I=E2=80=99ve 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 =3D=3D TK_EQ) { sqlVdbeAddOp2(v, OP_IfNot, dest, addrDone); - sqlVdbeChangeP5(v, OPFLAG_BOOLREQ); VdbeCoverage(v); p5 |=3D SQL_KEEPNULL; } else if (opx =3D=3D TK_NE) { sqlVdbeAddOp2(v, OP_If, dest, addrDone); - sqlVdbeChangeP5(v, OPFLAG_BOOLREQ); VdbeCoverage(v); p5 |=3D 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 =3D pParse->pVdbe; int op =3D 0; int regFree1 =3D 0; int regFree2 =3D 0; int r1, r2; - int jumpIfNull =3D flags & SQL_JUMPIFNULL; + assert(jumpIfNull =3D=3D SQL_JUMPIFNULL || jumpIfNull =3D=3D 0); if (NEVER(v =3D=3D 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 =3D pParse->pVdbe; int op =3D 0; int regFree1 =3D 0; int regFree2 =3D 0; int r1, r2; - int jumpIfNull =3D flags & SQL_JUMPIFNULL; + assert(jumpIfNull =3D=3D SQL_JUMPIFNULL || jumpIfNull =3D=3D 0); if (NEVER(v =3D=3D 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 !=3D 0); - /* - * Make sure that search condition - * under WHERE clause returns boolean. - */ - if ((flags & SQL_BOOLREQ) !=3D 0) - sqlVdbeChangeP5(v, = OPFLAG_BOOLREQ); VdbeCoverage(v); testcase(regFree1 =3D=3D 0); testcase(jumpIfNull =3D=3D 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 =3D=3D or <> */ #define SQL_NULLEQ 0x80 /* NULL=3DNULL */ #define SQL_NOTNULL 0x90 /* Assert that operands are never NULL = */ -#define SQL_BOOLREQ 0x100 /* Argument passed to OP_If must be = boolean */ =20 /** * 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. */ =20 /** * 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; } =20 -/* 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 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 =3D=3D 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 |=3D 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 |=3D TERM_CODED;