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

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

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 = sqlite3ExprAlloc(pParse->db, TK_INTEGER,
> -							&sqlite3IntTokens[1],
> -							0);
> +			if (pSel->pLimit == NULL) {
> +				pSel->pLimit =
> +					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 |= SF_SingleRow;
> 			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 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 = sqlite3GetTempReg(pParse);
> +				sqlite3VdbeAddOp2(v, OP_Integer, 1, r1);
> +				int no_err = sqlite3VdbeMakeLabel(v);
> +				sqlite3VdbeAddOp3(v, OP_Eq, iLimit, no_err, r1);
> +				const char *error =
> +					"SELECT subquery could be only limited "
> +					"with 1”;

Lets remove ’SELECT’ word: subquery implies it.

“Expression subquery could be limited only with 1.” -
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 &= ~SF_SingleRow;
> +			}
> +		}
> 		if (p->pOffset) {
> 			p->iOffset = iOffset = ++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)
> 	}
> }
> 

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 != 0);
> +	struct Vdbe *v = sqlite3GetVdbe(parser);
> +	assert(v != NULL);
> +
> +	int r1 = sqlite3GetTempReg(parser);
> +	sqlite3VdbeAddOp2(v, OP_Integer, 0, r1);
> +	sqlite3VdbeAddOp3(v, OP_Ne, r1, end_mark, limit_reg);
> +	const char *error = "SELECT subquery returned more than 1 row”;

Again: most DBs use next variant:

“Expression subquery returned more than 1 row” OR
“More than one row returned by a subquery used as an expression”

Btw, last variant is used in PostgeSQL 

> +	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 = multiSelect(pParse, p, pDest);
> 		pParse->iSelectId = iRestoreSelectId;
> +
> +		int end = sqlite3VdbeMakeLabel(v);
> +		if (p->selFlags & SF_SingleRow && p->iLimit != 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);
> 	}
> 
> -	/* Jump here to skip this query
> -	 */
> +	/* Generate code that prevent returning multiple rows. */
> +	if (p->selFlags & SF_SingleRow && p->iLimit != 0)
> +		vdbe_code_raise_on_multiple_rows(pParse, p->iLimit, iEnd);
> +	/* 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 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
> 
> /*
>  * 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: ‘contains’.

> 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 = require("sqltester")
> -test:plan(103)
> +test:plan(101)
> 
> --!./tcltestrunner.lua
> -- 2001 September 15
> @@ -1379,26 +1379,6 @@ test:do_execsql_test(
>     })
> 
> test:do_execsql_test(
> -    "select4-14.10",
> -    [[
> -        SELECT (VALUES(1),(2),(3),(4))
> -    ]], {
> -        -- <select4-14.10>
> -        1
> -        -- </select4-14.10>
> -    })

I wouldn’t 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’t 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 = require('test_run')
> +test_run = env.new()
> +
> +--
> +-- gh-2366: SQL subquery with =
> +--
> +
> +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;”)

Lets use upper-case for all SQL statements.

> +box.sql.execute("SELECT a FROM t1 WHERE b=6 LIMIT (select b from t1 where a =1);")
> +box.sql.execute("SELECT b from t1 where a = (SELECT a FROM t1 WHERE b=6);")
> +box.sql.execute("SELECT b from t1 where a = (SELECT a FROM t1 WHERE b=6 LIMIT (select b-1 from t1 where a =1));")
> +box.sql.execute("SELECT b from t1 where a = (SELECT a FROM t1 WHERE b=6 LIMIT (select b from t1 where a =1));")
> +
> +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.