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 4EA2726BE2 for ; Tue, 19 Jun 2018 13:56:45 -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 3ncEd2ENEiN3 for ; Tue, 19 Jun 2018 13:56:45 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 8950526BAB for ; Tue, 19 Jun 2018 13:56:44 -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 v1 1/1] sql: disallow returning many rows from subselect From: "n.pettik" In-Reply-To: Date: Tue, 19 Jun 2018 20:56:39 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: 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 > On 18 Jun 2018, at 14:55, Kirill Shcherbatov = wrote: >=20 > Branch: = http://github.com/tarantool/tarantool/tree/kshch/gh-2366-whith-select-subq= uery > Issue: https://github.com/tarantool/tarantool/issues/2366 >=20 > To follow ANSI SQL standard we should dissallow returning > multiple rows from subselects. This is not exactly correct. We disallow returning multiple rows only when subselect is used in form of expression. In other words, you are still able to use such subqueries with multiple rows/columns: SELECT * FROM (SELECT * FROM t1); It is also related to commit subject. > To achieve this goal we have > introduced some special Lets throw away words like =E2=80=99some=E2=80=99, =E2=80=99special=E2=80=99= etc.: they just contaminate commit message. > Select SF_SingleRow flag that indicates > the case of subselect having no client-defined LIMIT 1 to patch > system implicit LIMIT 1 to be LIMIT 2 and generate some extra > bytecode to HALT execution on reaching this restrict. > The place of patching LIMIT expression iValue is a very special: =46rom context it is not clear what you mean by iValue. > this showld be done after simplifying epression tree(as this Typo: should. > restrict could become useless), but before generating related > bytecode. >=20 > Resolves #2366 > =E2=80=94 Links to the branch and issue must be here, not at the start of commit message. > src/box/sql/expr.c | 18 ++++++++++++++---- > src/box/sql/select.c | 31 +++++++++++++++++++++++++++++-- > src/box/sql/sqliteInt.h | 2 ++ > test/sql-tap/aggnested.test.lua | 2 +- > test/sql-tap/e_expr.test.lua | 38 = +++++++++++++++++++------------------- > test/sql-tap/misc5.test.lua | 15 ++++++--------- > test/sql-tap/selectA.test.lua | 4 ++-- > test/sql-tap/subquery.test.lua | 14 +++++++------- > test/sql-tap/subquery2.test.lua | 4 ++-- > test/sql-tap/subselect.test.lua | 32 ++++++++++++++++++++++++++++---- > test/sql-tap/with1.test.lua | 2 +- > 11 files changed, 111 insertions(+), 51 deletions(-) >=20 > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index a69d38b..a22548c 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -2890,10 +2890,20 @@ 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->flags & EP_IntValue) || > + pSel->pLimit->u.iValue !=3D 1) { I found that queries like select a from t1 where a =3D (select a from t1 limit (select 1)); select a from t1 where a =3D (select a from t1 limit (2*1 - 1)); select a from t1 where a =3D (select a from t1 limit -1); select a from t1 where a =3D (select a from t1 limit 0.5) Also throw <- error: SELECT subquery returned more than 1 row> In the third case error should be sort of: I guess SQLite allows negative values under LIMIT clause, but it must be banned. I have opened issue for that: https://github.com/tarantool/tarantool/issues/3467 In the fourth case we should raise error. As for the first two example, it seems to be correct: SELECT 1; returns number literal 1. 2*1 - 1 =3D=3D 1 > + sql_expr_delete(pParse->db, = pSel->pLimit, false); > + pSel->pLimit =3D = sqlite3ExprAlloc(pParse->db, TK_INTEGER, It doesn=E2=80=99t fit into 80 chars. > + = &sqlite3IntTokens[1], > + 0); > + pSel->selFlags |=3D SF_SingleRow; > + } else { > + /* > + * Customer LIMIT 1 in subquery > + * should be kept. Do you need this empty =E2=80=98else=E2=80=99 clause only for comment? You can put it above =E2=80=98if=E2=80=99 clause. Lets use =E2=80=98user defined=E2=80=99 instead of =E2=80=98customer=E2=80= =99. > + */ > + } > 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 4b5ba4d..7b0c335 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -5665,6 +5665,21 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > if ((p->selFlags & SF_FixedLimit) =3D=3D 0) { > p->nSelectRow =3D 320; /* 4 billion rows */ > } > + bool single_row_assert =3D > + (p->pLimit !=3D NULL && p->selFlags & SF_SingleRow); > + if (single_row_assert) { > + /* > + * We have to change synthetic LIMIT constraint > + * to 2 here, because it should be set before > + * computeLimitRegisters call and operations > + * on tree, but after simplifying tree a bit > + * earlier. > + * Checks bytecode is generated in the end of > + * this function. > + */ To be honest, I haven=E2=80=99t understood why we can=E2=80=99t make = LIMIT be LIMIT 2 right in sqlite3CodeSubselect()=E2=80=A6 Examples above show that no simplifications under LIMIT clause occur: select a from t1 where a =3D (select a from t1 limit (2*1 - 1)); If you mean that LIMIT clause may disappear, it also doesn=E2=80=99t = seem to be true. Consider this example: select a from t1 where a =3D (select a from t1 where 1 !=3D 1 limit 3); Obviously, subquery always returns 0 rows, but LIMIT is coded anyway: SQL: [select a from t1 where a =3D (select a from t1 where 1 !=3D 1 = limit 3);] VDBE Program Listing: 101> 0 Init 0 25 0 00 Start at 25 101> 1 LoadPtr 0 1 0 space 00 r[1] =3D = space 101> 2 OpenWrite 2 524288 1 k(1,B) 00 index id =3D = 524288, space ptr =3D 1; sqlite_autoindex_T1_1 101> 3 Explain 0 0 0 SCAN TABLE T1 00=20 101> 4 Rewind 2 24 2 0 00=20 101> 5 Column 2 1 2 00 r[2]=3DT1.A 101> 6 Once 0 20 0 00=20 101> 7 Null 0 4 4 00 r[4..4]=3DNULL; = Init subquery result 101> 8 Integer 2 5 0 00 r[5]=3D2; LIMIT = counter 101> 9 Eq 6 17 6 (binary) 51 if r[6]=3D=3Dr[6]= goto 17 101> 10 LoadPtr 0 7 0 space 00 r[7] =3D = space 101> 11 OpenWrite 3 524288 7 k(1,B) 00 index id =3D = 524288, space ptr =3D 7; sqlite_autoindex_T1_1 101> 12 Explain 1 0 0 SCAN TABLE T1 00=20 101> 13 Rewind 3 17 8 0 00=20 101> 14 Column 3 1 4 00 r[4]=3DT1.A 101> 15 DecrJumpZero 5 17 0 00 if (--r[5])=3D=3D= 0 goto 17 101> 16 Next 3 14 0 01=20 101> 17 Integer 0 8 0 00 r[8]=3D0 101> 18 Ne 8 20 5 00 if r[5]!=3Dr[8] = goto 20 101> 19 Halt 22 3 0 SELECT subquery returned more = than 1 row 00=20 101> 20 Ne 4 23 2 (binary) 51 if r[2]!=3Dr[4] = goto 23 101> 21 Column 2 1 9 00 r[9]=3DT1.A 101> 22 ResultRow 9 1 0 00 output=3Dr[9] 101> 23 Next 2 5 0 01=20 101> 24 Halt 0 0 0 01=20 101> 25 Integer 1 6 0 00 r[6]=3D1 101> 26 Goto 0 1 0 00=20 > + assert(p->pLimit->flags & EP_IntValue); > + p->pLimit->u.iValue =3D 2; > + } > computeLimitRegisters(pParse, p, iEnd); > if (p->iLimit =3D=3D 0 && sSort.addrSortIndex >=3D 0) { > sqlite3VdbeChangeOpcode(v, sSort.addrSortIndex, = OP_SorterOpen); > @@ -6220,8 +6235,20 @@ 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. */ Prevent =E2=80=94> prevents from > + assert(single_row_assert =3D=3D > + (p->pLimit !=3D NULL && p->selFlags & SF_SingleRow)); > + if (single_row_assert) { > + int r1 =3D sqlite3GetTempReg(pParse); > + sqlite3VdbeAddOp2(v, OP_Integer, 0, r1); > + sqlite3VdbeAddOp3(v, OP_Ne, r1, iEnd, p->iLimit); > + const char *error =3D "SELECT subquery returned more = than 1 row=E2=80=9D; I guess, subquery term assumes =E2=80=99SELECT=E2=80=99 word in it, so = lets drop it. Also, I would mention, that it is 'subquery used as expression=E2=80=99. > + sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, > + ON_CONFLICT_ACTION_FAIL, 0, > + error, P4_STATIC); You may fit last two arguments in previous line, tho. > + sqlite3ReleaseTempReg(pParse, r1); > + } > + /* 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 fc6b858..01642a4 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2725,6 +2725,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. */ Contain =E2=80=94> contains