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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 6 comments below.

And I have pushed more minor fixes on the branch. Please,
squash.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 70e134f21..23cee593f 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) {

1. How can it be != NULL?

> +				pSel->pLimit =
> +					sqlite3ExprAlloc(pParse->db, TK_INTEGER,
> +							 &sqlite3IntTokens[1],
> +							 0);
> +				ExprSetProperty(pSel->pLimit, EP_System);

2. ExprSerProperty does not check for pSel->pLimit == NULL, so it crashes on
OOM.

> +			}
> +			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 54f78a9de..daec802da 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2120,6 +2120,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 =
> +					"Expression subquery could be limited "
> +					"only with 1.";
> +				sqlite3VdbeAddOp4(v, OP_Halt,
> +						  SQL_TARANTOOL_ERROR,
> +						  0, 0, error, P4_STATIC);

3. I do not see where do you set an appropriate error code (p5).

> +				sqlite3VdbeResolveLabel(v, no_err);
> +				sqlite3ReleaseTempReg(pParse, r1);
> +
> +				/* Runtime checks are no longer needed. */
> +				p->selFlags &= ~SF_SingleRow;
> +			}
> +		}
> @@ -5398,6 +5430,31 @@ explain_simple_count(struct Parse *parse_context, const char *table_name)
>  	}
>  }
>  
> +/**
> + * Generate VDBE code that HALT program when subselect returned
> + * more than one row (determined as LIMIT 1 overflow).
> + * @param parser Current parsing context.
> + * @param limit_reg LIMIT register.
> + * @param end_mark mark to jump if select returned distinct one
> + *                 row as expected.
> + */
> +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 = "Expression subquery returned more than 1 row";
> +	sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> +			  ON_CONFLICT_ACTION_FAIL, 0,
> +			  error, P4_STATIC);

4. Same.

> +	sqlite3ReleaseTempReg(parser, r1);
> +}
> +
>  /*
>   * Generate code for the SELECT statement given in the p argument.
>   *
> @@ -6326,8 +6389,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);

5. Why do you have two runtime checks for the same auto limit?

> +	/* 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 e939663b6..bacf415df 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h> @@ -2702,6 +2704,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 contains more than one row. */
> +#define SF_SingleRow      0x40000

6. Why not 0x20000?