[tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence

[Vladislav Shpilevoy <v.shpilevoy@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);