Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org,
	Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence
Date: Wed, 13 Jun 2018 21:53:14 +0300	[thread overview]
Message-ID: <6b41fbc2-4bae-7acd-e79c-5b0128396322@tarantool.org> (raw)
In-Reply-To: <34f76794d133bf7ae6ff076494b1d3cb3edcf841.1528535873.git.kshcherbatov@tarantool.org>

Thanks for the patch! See 12 comments below.

On 09/06/2018 12:32, Kirill Shcherbatov wrote:
> Trigger presence in system should be tested on each VDBE
> execution attempt, not on Parser iteration.
> 
> Part of #3435, #3273
> ---
>   src/box/sql.c           | 35 +++++++++++++++++++++++++++++++++++
>   src/box/sql/main.c      | 13 +++++++++++--
>   src/box/sql/sqliteInt.h | 20 ++++++++++++++++++++
>   src/box/sql/trigger.c   | 20 +++++++++-----------
>   src/box/sql/vdbe.c      | 14 ++++++++++----
>   src/box/sql/vdbeapi.c   |  5 +++--
>   6 files changed, 88 insertions(+), 19 deletions(-)
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 57211fd..f539085 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1836,3 +1836,38 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
>   	}
>   	return 0;
>   }
> +
> +int
> +vdbe_abort_execution_on_exists(Parse *parser, int space_id, int index_id,
> +			       const char *name, int tarantool_error_code,
> +			       bool no_error)

1. If this function is method of parser then its prefix must be 'parser_'.
And here you do not an abortion. You emit the code of abortion if needed.
So this function can be named like this:

	parser_emit_execution_halt_on_exists

2. struct Parse

3. Why is the function stored in sql.c? As far I remember sql.c/.h is a
link between sqlite and Tarantool code, but vdbe_abort_execution_on_exists
is used in sqlite only.

> +{
> +	Vdbe *v = sqlite3GetVdbe(parser);

4. struct Vdbe. Please check other places yourself.

> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index 0acf7bc..f2334cb 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -1456,8 +1456,17 @@ sqlite3_errmsg(sqlite3 * db)
>   		testcase(db->pErr == 0);
>   		z = (char *)sqlite3_value_text(db->pErr);
>   		assert(!db->mallocFailed);
> -		if (z == 0) {
> -			z = sqlite3ErrStr(db->errCode);
> +		if (db->errCode != SQL_TARANTOOL_ERROR) {
> +			testcase(db->pErr == 0);

5. What is 'testcase'? Why do you need it here?

> +			z = (char *) sqlite3_value_text(db->pErr);

6. It is assigned to this value few lines above.

> +			assert(!db->mallocFailed);
> +			if (z == NULL)
> +				z = sqlite3ErrStr(db->errCode);
> +		} else {
> +			struct diag *diag = diag_get();
> +			assert(diag != NULL);

7. diag is never NULL, because TX thread consists of fibers, and
each fiber has diag. Please, remove the assertion and just inline
diag_get() on the next line.

> +			struct error *error = diag_last_error(diag);
> +			z = error->errmsg;
>   		}
>   	}
>   	return z;
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 276f881..4f7dcbe 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -4537,4 +4537,24 @@ extern int sqlite3InitDatabase(sqlite3 * db);
>   enum on_conflict_action
>   table_column_nullable_action(struct Table *tab, uint32_t column);
>   
> +/**
> + * Generate VDBE code to halt execution with correct error if
> + * the object with specified name is already present in
> + * specified space.
> + *
> + * @param parser Parsing context.
> + * @param space_id Space to lookup identifier.
> + * @param index_id Index identifier containing string primary key.
> + * @param name Of object to test existency.

8. No such word 'existency'.

> + * @param tarantool_error_code Tarantool error to raise on object exists.

9. Out of 66.

> + * @param no_error Do not raise error flag.
> + *
> + * @retval -1 on memory allocation error.
> + * @retval 0 on success.
> + */
> +int
> +vdbe_abort_execution_on_exists(Parse *parser, int space_id, int index_id,
> +			       const char *name, int tarantool_error_code,
> +			       bool no_error);
> +
>   #endif				/* SQLITEINT_H */
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 1b569bc..92747f0 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -178,6 +167,15 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
>   		pParse->rc = SQL_TARANTOOL_ERROR;
>   		goto trigger_cleanup;
>   	}
> +	if (!pParse->parse_only) {
> +		if (vdbe_abort_execution_on_exists(pParse, BOX_TRIGGER_ID, 0,
> +						   zName,
> +						   ER_TRIGGER_EXISTS,
> +						   (noErr != 0)) != 0) {
> +			pParse->rc = SQL_TARANTOOL_ERROR;

10. pParse->nErr++. Or find a way to delete nErr. It is very annoying and
useless  attribute.

> +			goto trigger_cleanup;
> +		}
> +	}
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 2d1538e..0696cad 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -959,7 +959,7 @@ case OP_HaltIfNull: {      /* in3 */
>    * VDBE, but do not rollback the transaction.
>    *
>    * If P4 is not null then it is an error message string.
> - *
> + * If P1 is not SQL_TARANTOOL_ERROR,
>    * P5 is a value between 0 and 4, inclusive, that modifies the P4 string.

11. Why here you did not put empty line after 'if P1', but did in the
next hunk?

>    *
>    *    0:  (no change)
> @@ -968,6 +968,8 @@ case OP_HaltIfNull: {      /* in3 */
>    *    3:  CHECK constraint failed: P4
>    *    4:  FOREIGN KEY constraint failed: P4
>    *
> + * If P1 is SQL_TARANTOOL_ERROR, P5 contain ClientError code.
> + *
>    * If P5 is not zero and P4 is NULL, then everything after the ":" is
>    * omitted.
>    *
> @@ -1005,9 +1007,10 @@ case OP_Halt: {
>   	p->rc = pOp->p1;
>   	p->errorAction = (u8)pOp->p2;
>   	p->pc = pcx;
> -	assert(pOp->p5<=4);
>   	if (p->rc) {
> -		if (pOp->p5) {
> +		if (pOp->p5 != 0 && p->rc == SQL_TARANTOOL_ERROR) {
> +			diag_set(ClientError, pOp->p5, pOp->p4.z);

12. I am afraid of this way will not work, when a code has multiple
arguments. And when type is not ClientError. And when it has a single
non-string argument. And when we want to know where the error was
generated. Here diag_set always saves filename "vdbe.c" and line "1012".
I think we should find another way to emit an error. One possible way
is to create and error object during parsing and save it by pointer in
p4. When the error occurs, the error object is set into the diag object.
But maybe you will find another way.

> +		} else if (pOp->p5 != 0) {
>   			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
>   							       "FOREIGN KEY" };
>   			testcase( pOp->p5==1);

  reply	other threads:[~2018-06-13 18:53 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-09  9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov
2018-06-09  9:32 ` [tarantool-patches] [PATCH v2 10/11] sql: move " Kirill Shcherbatov
2018-06-13 18:53   ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-14 16:12     ` Kirill Shcherbatov
2018-06-28  7:18   ` Konstantin Osipov
2018-06-28  7:33     ` Kirill Shcherbatov
2018-06-09  9:32 ` [tarantool-patches] [PATCH v2 11/11] sql: VDBE tests for trigger existence Kirill Shcherbatov
2018-06-13 18:53   ` Vladislav Shpilevoy [this message]
2018-06-14 16:12     ` [tarantool-patches] " Kirill Shcherbatov
2018-06-09  9:32 ` [tarantool-patches] [PATCH v2 02/11] box: move db->pShchema init to sql_init Kirill Shcherbatov
2018-06-09  9:32 ` [tarantool-patches] [PATCH v2 04/11] sql: fix sql len in tarantoolSqlite3RenameTrigger Kirill Shcherbatov
2018-06-09  9:32 ` [tarantool-patches] [PATCH v2 06/11] sql: refactor sql_expr_compile to return AST Kirill Shcherbatov
2018-06-13 18:53   ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-14 16:12     ` Kirill Shcherbatov
2018-06-09  9:32 ` [tarantool-patches] [PATCH v2 07/11] sql: move sqlite3DeleteTrigger to sql.h Kirill Shcherbatov
2018-06-13 18:53   ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-14 16:12     ` Kirill Shcherbatov
2018-06-09  9:32 ` [tarantool-patches] [PATCH v2 08/11] box: sort error codes in misc.test Kirill Shcherbatov
2018-06-09  9:32 ` [tarantool-patches] [PATCH v2 09/11] sql: new _trigger space format with space_id Kirill Shcherbatov
2018-06-13 18:53   ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-14 16:12     ` Kirill Shcherbatov
2018-06-09  9:58 ` [tarantool-patches] [PATCH v2 01/11] box: remove migration from alpha 1.8.2 and 1.8.4 Kirill Shcherbatov
2018-06-09  9:58 ` [tarantool-patches] [PATCH v2 03/11] sql: fix leak on CREATE TABLE and resolve self ref Kirill Shcherbatov
2018-06-13 18:53   ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-14 16:12     ` Kirill Shcherbatov
2018-06-09  9:58 ` [tarantool-patches] [PATCH v2 05/11] box: port schema_find_id to C Kirill Shcherbatov
2018-06-13 18:53   ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-14 16:12     ` Kirill Shcherbatov

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=6b41fbc2-4bae-7acd-e79c-5b0128396322@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence' \
    /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