[tarantool-patches] Re: [PATCH v4 6/6] sql: VDBE tests for trigger existence

["n.pettik" <korablev@tarantool.org>]

> On 20 Jun 2018, at 13:46, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> Trigger presence in system should be

Why? If you didn’t check it, you simply would get ‘duplicate key error’.
So, you did it to display more informative error..?

> tested on each VDBE
> execution attempt, not on Parser iteration.

And? After this sentence you should explain changes which you patch provides.

> 
> Part of #3435, #3273
> ---
> src/box/sql/build.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
> src/box/sql/main.c         | 11 ++++++++---
> src/box/sql/sqliteInt.h    | 24 ++++++++++++++++++++++++
> src/box/sql/trigger.c      | 25 +++++++++++++------------
> src/box/sql/vdbe.c         | 18 +++++++++++-------
> test/sql-tap/view.test.lua |  8 ++++----
> test/sql/view.result       |  4 ++--
> 7 files changed, 104 insertions(+), 28 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index c8bfad7..fff7c19 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3971,4 +3971,46 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
> 		sqlite3DbFree(db, pWith);
> 	}
> }
> +
> +int
> +vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,

I would call it vdbe_emit_halt_if_exists().
Or vdbe_emit_halt_on_duplication()...

> +				   int index_id, const char *name_src,
> +				   int tarantool_error_code,
> +				   const char *error_src, bool no_error)
> +{
> +	struct Vdbe *v = sqlite3GetVdbe(parser);
> +	assert(v != NULL);
> +
> +	struct sqlite3 *db = parser->db;
> +	char *name = sqlite3DbStrDup(db, name_src);
> +	if (name == NULL) {
> +		size_t size = strlen(name_src) + 1;
> +		diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name”);

In previous patch, you used sqlite3DbMalloc() function without using diag_set(),
since in case of fail it would call sqlite3OomFault(db);
Lets handle all usages of sqlite3DbMalloc() in the same way.

> +		return -1;
> +	}
> +	char *error = sqlite3DbStrDup(db, error_src);
> +	if (error == NULL) {
> +		sqlite3DbFree(db, name);
> +		size_t size = strlen(error_src) + 1;
> +		diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error”);

The same is here.

> +		return -1;
> +	}
> +
> +	int cursor = parser->nTab++;
> +	vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
> +
> +	int name_reg = parser->nMem++;
> +	int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
> +				      P4_DYNAMIC);
> +	sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
> +	if (no_error) {
> +		sqlite3VdbeAddOp0(v, OP_Halt);
> +	} else {
> +		sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> +				  ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);

Why do you set ON_CONFLICT_ACTION_FAIL? You can just skip this arg with 0.

> +		sqlite3VdbeChangeP5(v, tarantool_error_code);
> +	}

You may refactor code this way (if you like it):

+++ b/src/box/sql/build.c
@@ -4003,11 +4003,10 @@ vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
        int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
                                      P4_DYNAMIC);
        sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
-       if (no_error) {
-               sqlite3VdbeAddOp0(v, OP_Halt);
-       } else {
-               sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
-                                 ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
+       int op_halt_addr = sqlite3VdbeAddOp0(v, OP_Halt);
+       if (!no_error) {
+               sqlite3VdbeChangeP1(v, op_halt_addr, SQL_TARANTOOL_ERROR);
+               sqlite3VdbeChangeP4(v, op_halt_addr, error, P4_DYNAMIC);
                sqlite3VdbeChangeP5(v, tarantool_error_code);
        }

> +	sqlite3VdbeAddOp1(v, OP_Close, cursor);
> +	return 0;
> +}
> #endif				/* !defined(SQLITE_OMIT_CTE) */

Why this func is under SQLITE_OMIT_CTE guard?

> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index 0acf7bc..e435c01 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -1454,11 +1454,16 @@ sqlite3_errmsg(sqlite3 * db)
> 		z = sqlite3ErrStr(SQLITE_NOMEM_BKPT);
> 	} else {
> 		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) {
> +			assert(!db->mallocFailed);

Why have you duplicated this assert? It occurs 2 lines above.

> +			z = (char *)sqlite3_value_text(db->pErr);
> +			if (z == NULL)
> +				z = sqlite3ErrStr(db->errCode);
> +		} else {
> +			z = diag_last_error(diag_get())->errmsg;
> 		}
> +		assert(z != NULL);
> 	}
> 	return z;
> }
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 72803fa..acda23d 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -4627,4 +4627,28 @@ 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.
> + * The function does not begin to hold the passed error pointer
> + * to memory.

Rephrase last sentence - I can’t parse it..

> + *
> + * @param parser Parsing context.
> + * @param space_id Space to lookup identifier.
> + * @param index_id Index identifier containing string primary key.

How can index id contain ’string primary key’?

> + * @param name_src Of object to test existence.

Name of object to test on existence.

> + * @param tarantool_error_code to set on halt.
> + * @param error_src string to notify on halt.

Error message to display on VDBE halt.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c1df880..487b026 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -960,7 +960,9 @@ case OP_HaltIfNull: {      /* in3 */
>  *
>  * If P4 is not null then it is an error message string.
>  *
> - * P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
> + * If P1 is SQL_TARANTOOL_ERROR then P5 is a ClientError code and
> + * P4 is error message to set. Else P5 is a value between 0 and 4,
> + * inclusive, that modifies the P4 string.
>  *
>  *    0:  (no change)
>  *    1:  NOT NULL contraint failed: P4
> @@ -968,8 +970,8 @@ case OP_HaltIfNull: {      /* in3 */
>  *    3:  CHECK constraint failed: P4
>  *    4:  FOREIGN KEY constraint failed: P4
>  *
> - * If P5 is not zero and P4 is NULL, then everything after the ":" is
> - * omitted.
> + * If P5 is not zero and  P4 is  NULL, then everything after the
> + * ":" is omitted.

Looks like redundant diff. If you wanted to make comments fit into 66 chars,
you would better do it for the whole comment.

>  *
>  * There is an implied "Halt 0 0 0" instruction inserted at the very end of
>  * every program.  So a jump past the last instruction of the program
> @@ -1005,9 +1007,11 @@ 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 (p->rc == SQL_TARANTOOL_ERROR) {
> +			assert(pOp->p4.z != NULL);
> +			box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
> +		} else if (pOp->p5 != 0) {
> 			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
> 							       "FOREIGN KEY" };
> 			testcase( pOp->p5==1);
> @@ -2999,8 +3003,8 @@ case OP_CheckViewReferences: {
> 	struct space *space = space_by_id(space_id);
> 	assert(space != NULL);
> 	if (space->def->view_ref_count > 0) {
> -		sqlite3VdbeError(p,"Can't drop table %s: other views depend "
> -				 "on this space",  space->def->name);
> +		diag_set(ClientError, ER_DROP_SPACE, space->def->name,
> +			 "other views depend on this space”);

Why did you provide this diff? AFAIK in this particular case these two calls
are *almost* equivalent.