[tarantool-patches] Re: [PATCH v2 1/1] sql: disallow returning many rows from subselect
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Jul 2 16:30:24 MSK 2018
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?
More information about the Tarantool-patches
mailing list