Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org
Cc: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: disallow returning many rows from subselect
Date: Mon, 2 Jul 2018 16:30:24 +0300	[thread overview]
Message-ID: <7ba69010-2e21-9e4f-d923-4061ddecf4cf@tarantool.org> (raw)
In-Reply-To: <411BF67B-D49B-468C-80EF-AF31D520F0DF@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?

  reply	other threads:[~2018-07-02 13:30 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-18 11:55 [tarantool-patches] [PATCH v1 " Kirill Shcherbatov
2018-06-19 17:56 ` [tarantool-patches] " n.pettik
2018-06-27 15:30   ` [tarantool-patches] [PATCH v2 " Kirill Shcherbatov
2018-06-27 15:40   ` Kirill Shcherbatov
2018-06-28 13:04     ` [tarantool-patches] " n.pettik
2018-06-28 17:08       ` Kirill Shcherbatov
2018-06-29 13:15         ` n.pettik
2018-07-02  7:17           ` Kirill Shcherbatov
2018-07-02  8:45             ` n.pettik
2018-07-02 13:30               ` Vladislav Shpilevoy [this message]
2018-07-02 14:14                 ` Kirill Shcherbatov
2018-07-02 14:42                   ` Vladislav Shpilevoy
2018-07-02 14:52                     ` Kirill Shcherbatov
2018-07-02 17:58                       ` Vladislav Shpilevoy
2018-07-03  8:06 ` [tarantool-patches] Re: [PATCH v1 " Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7ba69010-2e21-9e4f-d923-4061ddecf4cf@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v2 1/1] sql: disallow returning many rows from subselect' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox