[tarantool-patches] Re: [PATCH v1 1/1] sql: disallow returning many rows from subselect

["n.pettik" <korablev@tarantool.org>]

> On 18 Jun 2018, at 14:55, Kirill Shcherbatov <kshcherbatov@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