[tarantool-patches] Re: [PATCH v1 1/1] sql: disallow returning many rows from subselect
n.pettik
korablev at tarantool.org
Tue Jun 19 20:56:39 MSK 2018
> On 18 Jun 2018, at 14:55, Kirill Shcherbatov <kshcherbatov at tarantool.org> wrote:
>
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-2366-whith-select-subquery
> Issue: https://github.com/tarantool/tarantool/issues/2366
>
> 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 ’some’, ’special’ 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:
From 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.
>
> Resolves #2366
> —
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(-)
>
> 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 = sqlite3ExprAlloc(pParse->db, TK_INTEGER,
> - &sqlite3IntTokens[1],
> - 0);
> + if (pSel->pLimit == NULL ||
> + !(pSel->pLimit->flags & EP_IntValue) ||
> + pSel->pLimit->u.iValue != 1) {
I found that queries like
select a from t1 where a = (select a from t1 limit (select 1));
select a from t1 where a = (select a from t1 limit (2*1 - 1));
select a from t1 where a = (select a from t1 limit -1);
select a from t1 where a = (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:
<LIMIT must not be negative>
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 <type mismatch> error.
As for the first two example, it seems to be correct:
SELECT 1; returns number literal 1.
2*1 - 1 == 1
> + sql_expr_delete(pParse->db, pSel->pLimit, false);
> + pSel->pLimit = sqlite3ExprAlloc(pParse->db, TK_INTEGER,
It doesn’t fit into 80 chars.
> + &sqlite3IntTokens[1],
> + 0);
> + pSel->selFlags |= SF_SingleRow;
> + } else {
> + /*
> + * Customer LIMIT 1 in subquery
> + * should be kept.
Do you need this empty ‘else’ clause only for comment?
You can put it above ‘if’ clause.
Lets use ‘user defined’ instead of ‘customer’.
> + */
> + }
> pSel->iLimit = 0;
> pSel->selFlags &= ~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) == 0) {
> p->nSelectRow = 320; /* 4 billion rows */
> }
> + bool single_row_assert =
> + (p->pLimit != 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’t understood why we can’t make LIMIT be
LIMIT 2 right in sqlite3CodeSubselect()…
Examples above show that no simplifications under LIMIT clause occur:
select a from t1 where a = (select a from t1 limit (2*1 - 1));
If you mean that LIMIT clause may disappear, it also doesn’t seem to be true.
Consider this example:
select a from t1 where a = (select a from t1 where 1 != 1 limit 3);
Obviously, subquery always returns 0 rows, but LIMIT is coded anyway:
SQL: [select a from t1 where a = (select a from t1 where 1 != 1 limit 3);]
VDBE Program Listing:
101> 0 Init 0 25 0 00 Start at 25
101> 1 LoadPtr 0 1 0 space<name=T1> 00 r[1] = space<name=T1>
101> 2 OpenWrite 2 524288 1 k(1,B) 00 index id = 524288, space ptr = 1; sqlite_autoindex_T1_1
101> 3 Explain 0 0 0 SCAN TABLE T1 00
101> 4 Rewind 2 24 2 0 00
101> 5 Column 2 1 2 00 r[2]=T1.A
101> 6 Once 0 20 0 00
101> 7 Null 0 4 4 00 r[4..4]=NULL; Init subquery result
101> 8 Integer 2 5 0 00 r[5]=2; LIMIT counter
101> 9 Eq 6 17 6 (binary) 51 if r[6]==r[6] goto 17
101> 10 LoadPtr 0 7 0 space<name=T1> 00 r[7] = space<name=T1>
101> 11 OpenWrite 3 524288 7 k(1,B) 00 index id = 524288, space ptr = 7; sqlite_autoindex_T1_1
101> 12 Explain 1 0 0 SCAN TABLE T1 00
101> 13 Rewind 3 17 8 0 00
101> 14 Column 3 1 4 00 r[4]=T1.A
101> 15 DecrJumpZero 5 17 0 00 if (--r[5])==0 goto 17
101> 16 Next 3 14 0 01
101> 17 Integer 0 8 0 00 r[8]=0
101> 18 Ne 8 20 5 00 if r[5]!=r[8] goto 20
101> 19 Halt 22 3 0 SELECT subquery returned more than 1 row 00
101> 20 Ne 4 23 2 (binary) 51 if r[2]!=r[4] goto 23
101> 21 Column 2 1 9 00 r[9]=T1.A
101> 22 ResultRow 9 1 0 00 output=r[9]
101> 23 Next 2 5 0 01
101> 24 Halt 0 0 0 01
101> 25 Integer 1 6 0 00 r[6]=1
101> 26 Goto 0 1 0 00
> + assert(p->pLimit->flags & EP_IntValue);
> + p->pLimit->u.iValue = 2;
> + }
> computeLimitRegisters(pParse, p, iEnd);
> if (p->iLimit == 0 && sSort.addrSortIndex >= 0) {
> sqlite3VdbeChangeOpcode(v, sSort.addrSortIndex, OP_SorterOpen);
> @@ -6220,8 +6235,20 @@ sqlite3Select(Parse * pParse, /* The parser context */
> generateSortTail(pParse, p, &sSort, pEList->nExpr, pDest);
> }
>
> - /* Jump here to skip this query
> - */
> + /* Generate code that prevent returning multiple rows. */
Prevent —> prevents from
> + assert(single_row_assert ==
> + (p->pLimit != NULL && p->selFlags & SF_SingleRow));
> + if (single_row_assert) {
> + int r1 = sqlite3GetTempReg(pParse);
> + sqlite3VdbeAddOp2(v, OP_Integer, 0, r1);
> + sqlite3VdbeAddOp3(v, OP_Ne, r1, iEnd, p->iLimit);
> + const char *error = "SELECT subquery returned more than 1 row”;
I guess, subquery term assumes ’SELECT’ word in it, so lets drop it.
Also, I would mention, that it is 'subquery used as expression’.
> + 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);
>
> /* 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 —> contains
More information about the Tarantool-patches
mailing list