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 B398D2623B for ; Thu, 28 Jun 2018 09:04:30 -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 IoB0CF0Fzt_I for ; Thu, 28 Jun 2018 09:04:30 -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 4700626217 for ; Thu, 28 Jun 2018 09:04:30 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: disallow returning many rows from subselect From: "n.pettik" In-Reply-To: <7815ec2cb5cfb0d837478cff88fe4ba95503e8d7.1530113937.git.kshcherbatov@tarantool.org> Date: Thu, 28 Jun 2018 16:04:27 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <7815ec2cb5cfb0d837478cff88fe4ba95503e8d7.1530113937.git.kshcherbatov@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: Kirill Shcherbatov Thanks for the patch. I can provide only few minor comments. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 70e134f..23cee59 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -2874,10 +2874,14 @@ sqlite3CodeSubselect(Parse * pParse, /* = Parsing context */ > dest.iSDParm); > VdbeComment((v, "Init EXISTS result")); > } > - sql_expr_delete(pParse->db, pSel->pLimit, = false); > - pSel->pLimit =3D sqlite3ExprAlloc(pParse->db, = TK_INTEGER, > - = &sqlite3IntTokens[1], > - 0); > + if (pSel->pLimit =3D=3D NULL) { > + pSel->pLimit =3D > + sqlite3ExprAlloc(pParse->db, = TK_INTEGER, > + = &sqlite3IntTokens[1], > + 0); Should we test pLimit on OOM? AFAIS sqlite3ExprAlloc can return NULL = ptr. > + ExprSetProperty(pSel->pLimit, = EP_System); > + } > + pSel->selFlags |=3D SF_SingleRow; > pSel->iLimit =3D 0; > pSel->selFlags &=3D ~SF_MultiValue; > if (sqlite3Select(pParse, pSel, &dest)) { > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 4e61ec1..e904c2a 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -2119,6 +2119,38 @@ computeLimitRegisters(Parse * pParse, Select * = p, int iBreak) > sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak); > VdbeCoverage(v); > } > + if (p->selFlags & SF_SingleRow) { > + if (ExprHasProperty(p->pLimit, EP_System)) { > + /* > + * Indirect LIMIT 1 is allowed only for > + * requests returning only 1 row. > + * To test this, we change LIMIT 1 to > + * LIMIT 2 and will look up LIMIT 1 = overflow > + * at the sqlite3Select end. > + */ > + sqlite3VdbeAddOp2(v, OP_Integer, 2, = iLimit); > + } else { > + /* > + * User-defined complex limit for = subquery > + * could be only 1 as resulting value. > + */ > + int r1 =3D sqlite3GetTempReg(pParse); > + sqlite3VdbeAddOp2(v, OP_Integer, 1, r1); > + int no_err =3D sqlite3VdbeMakeLabel(v); > + sqlite3VdbeAddOp3(v, OP_Eq, iLimit, = no_err, r1); > + const char *error =3D > + "SELECT subquery could be only = limited " > + "with 1=E2=80=9D; Lets remove =E2=80=99SELECT=E2=80=99 word: subquery implies it. =E2=80=9CExpression subquery could be limited only with 1.=E2=80=9D - I suggest this variant. > + sqlite3VdbeAddOp4(v, OP_Halt, > + SQL_TARANTOOL_ERROR, > + 0, 0, error, = P4_STATIC); > + sqlite3VdbeResolveLabel(v, no_err); > + sqlite3ReleaseTempReg(pParse, r1); > + > + /* Runtime checks are no longer needed. = */ > + p->selFlags &=3D ~SF_SingleRow; > + } > + } > if (p->pOffset) { > p->iOffset =3D iOffset =3D ++pParse->nMem; > pParse->nMem++; /* Allocate an extra register = for limit+offset */ > @@ -5368,6 +5400,23 @@ explain_simple_count(struct Parse = *parse_context, const char *table_name) > } > } >=20 AFAIR Kostya and Vlad ask for adding comments even for static functions. > +static void > +vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, = int end_mark) > +{ > + assert(limit_reg !=3D 0); > + struct Vdbe *v =3D sqlite3GetVdbe(parser); > + assert(v !=3D NULL); > + > + int r1 =3D sqlite3GetTempReg(parser); > + sqlite3VdbeAddOp2(v, OP_Integer, 0, r1); > + sqlite3VdbeAddOp3(v, OP_Ne, r1, end_mark, limit_reg); > + const char *error =3D "SELECT subquery returned more than 1 = row=E2=80=9D; Again: most DBs use next variant: =E2=80=9CExpression subquery returned more than 1 row=E2=80=9D OR =E2=80=9CMore than one row returned by a subquery used as an = expression=E2=80=9D Btw, last variant is used in PostgeSQL=20 > + sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, > + ON_CONFLICT_ACTION_FAIL, 0, > + error, P4_STATIC); > + sqlite3ReleaseTempReg(parser, r1); > +} > + > /* > * Generate code for the SELECT statement given in the p argument. > * > @@ -5504,6 +5553,12 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > if (p->pPrior) { > rc =3D multiSelect(pParse, p, pDest); > pParse->iSelectId =3D iRestoreSelectId; > + > + int end =3D sqlite3VdbeMakeLabel(v); > + if (p->selFlags & SF_SingleRow && p->iLimit !=3D 0) > + vdbe_code_raise_on_multiple_rows(pParse, = p->iLimit, end); > + sqlite3VdbeResolveLabel(v, end); > + > #ifdef SELECTTRACE_ENABLED > SELECTTRACE(1, pParse, p, ("end compound-select = processing\n")); > pParse->nSelectIndent--; > @@ -6296,8 +6351,10 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > generateSortTail(pParse, p, &sSort, pEList->nExpr, = pDest); > } >=20 > - /* Jump here to skip this query > - */ > + /* Generate code that prevent returning multiple rows. */ > + if (p->selFlags & SF_SingleRow && p->iLimit !=3D 0) > + vdbe_code_raise_on_multiple_rows(pParse, p->iLimit, = iEnd); > + /* Jump here to skip this query. */ > sqlite3VdbeResolveLabel(v, iEnd); >=20 > /* The SELECT has been coded. If there is an error in the Parse = structure, > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 5f7a0f1..093aabb 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2370,6 +2370,8 @@ struct Expr { > #define EP_Subquery 0x200000 /* Tree contains a TK_SELECT operator */ > #define EP_Alias 0x400000 /* Is an alias for a result set column = */ > #define EP_Leaf 0x800000 /* Expr.pLeft, .pRight, .u.pSelect all = NULL */ > +/** Expression is system-defined. */ > +#define EP_System 0x1000000 >=20 > /* > * Combinations of two or more EP_* flags > @@ -2718,6 +2720,8 @@ struct Select { > #define SF_FixedLimit 0x04000 /* nSelectRow set by a constant = LIMIT */ > #define SF_MaybeConvert 0x08000 /* Need = convertCompoundSelectToSubquery() */ > #define SF_Converted 0x10000 /* By = convertCompoundSelectToSubquery() */ > +/** Abort subquery if its output contain more than one row. */ Typo: =E2=80=98contains=E2=80=99. > diff --git a/test/sql-tap/select4.test.lua = b/test/sql-tap/select4.test.lua > index 5f91b13..5b49d8b 100755 > --- a/test/sql-tap/select4.test.lua > +++ b/test/sql-tap/select4.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test =3D require("sqltester") > -test:plan(103) > +test:plan(101) >=20 > --!./tcltestrunner.lua > -- 2001 September 15 > @@ -1379,26 +1379,6 @@ test:do_execsql_test( > }) >=20 > test:do_execsql_test( > - "select4-14.10", > - [[ > - SELECT (VALUES(1),(2),(3),(4)) > - ]], { > - -- > - 1 > - -- > - }) I wouldn=E2=80=99t delete these tests: let them remain to check that expression subqueries tolerate only one row as result. So, simply change type of test to catchsql and fix its result. It also related to the rest of deleted tests (tkt-473 and so on). > diff --git a/test/sql/gh-2366-whith-select-subquery.test.lua = b/test/sql/gh-2366-whith-select-subquery.test.lua > new file mode 100644 > index 0000000..39116df > --- /dev/null > +++ b/test/sql/gh-2366-whith-select-subquery.test.lua I wouldn=E2=80=99t create separate sql/ test for this issue: there are a lot of sql-tap/ tests on subqueries - you are able to move these tests to one of them. > @@ -0,0 +1,23 @@ > +env =3D require('test_run') > +test_run =3D env.new() > + > +-- > +-- gh-2366: SQL subquery with =3D > +-- > + > +box.sql.execute("CREATE TABLE t1(a int primary key, b int);") > +box.sql.execute("INSERT INTO t1 VALUES(1,2);") > +box.sql.execute("INSERT INTO t1 VALUES(3,4);") > +box.sql.execute("INSERT INTO t1 VALUES(5,6);") > +box.sql.execute("INSERT INTO t1 VALUES(7,6);") > + > +box.sql.execute("select * from t1;=E2=80=9D) Lets use upper-case for all SQL statements. > +box.sql.execute("SELECT a FROM t1 WHERE b=3D6 LIMIT (select b from t1 = where a =3D1);") > +box.sql.execute("SELECT b from t1 where a =3D (SELECT a FROM t1 WHERE = b=3D6);") > +box.sql.execute("SELECT b from t1 where a =3D (SELECT a FROM t1 WHERE = b=3D6 LIMIT (select b-1 from t1 where a =3D1));") > +box.sql.execute("SELECT b from t1 where a =3D (SELECT a FROM t1 WHERE = b=3D6 LIMIT (select b from t1 where a =3D1));") > + > +box.sql.execute("SELECT (VALUES(1),(2),(3),(4));") > + > +-- clean-up > +box.sql.execute("DROP TABLE t1;") > \ No newline at end of file Put new line at end of file.