[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the fixes! See 4 comments below.

On 18/06/2018 22:22, Kirill Shcherbatov wrote:
>> 1. Sorry, after discussion with Nikita we have decided to
>> use 'vdbe_' prefix for emitters even if they take parser in
>> first argument.
> Tnx, squashed.
> 
>> 2. Mismatch of parameter names: error in header, error_src in source.
> Fixed.

1. Still is not fixed. Now mismatch of the parameter name in the
declaration and the name in the comment.

> 
>> 3. Please, reduce the mess. Separate these OOM handlers.
>          name = sqlite3DbStrDup(db, name_src);
> -       if (name == NULL)
> +       if (name == NULL) {
> +               size_t size = strlen(name_src) + 1;
> +               diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
>                  return -1;
> +       }
>          error = sqlite3DbStrDup(db, error_src);
>          if (error == NULL) {
>                  sqlite3DbFree(db, name);
> -               return -1;
> -       }
> -       if (name == NULL || error == NULL) {
> -               size_t size =
> -                       (name != NULL ? strlen(error_src) :
> -                        strlen(name_src)) + 1;
> -               const char *var_name = name != NULL ? "error" : "name";
> -               diag_set(OutOfMemory, size, "sqlite3DbStrDup", var_name);
> -               sqlite3DbFree(db, name);
> -               sqlite3DbFree(db, error);
> +               size_t size = strlen(error_src) + 1;
> +               diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
>                  return -1;
>          }
> 
> 
>> 6. Earlier this function could return either DONE or ERROR. Now
>> it can return SQL_TARANTOOL_ERROR, and I am not sure that the
>> behavior should be changed in such way. Please, explain why do you
>> need SQL_TARANTOOL_ERROR here.
> This is the sane behavior that parser has. I'd like to distinguish tarantool errors and out them from diag like Parser does.
> Take a look to the changes in sqlite3_errmsg.

2. I see the changes, but they are not linked with VdbeExec return value. The only
place where you have used SQL_TARANTOOL_ERROR is an assertion in sqlite3Step.

db->errCode finally is set not to VdbeExec return value, but to Vdbe->rc, that
is SQL_TARANTOOL_ERROR already.

I have reverted your changes at the end of OP_Halt:

@@ -1033,10 +1033,7 @@ case OP_Halt: {
  		p->rc = SQLITE_BUSY;
  	} else {
  		assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT);
-		if (p->rc != SQL_TARANTOOL_ERROR)
-			rc = p->rc != SQLITE_OK ? SQLITE_ERROR : SQLITE_DONE;
-		else
-			rc = SQL_TARANTOOL_ERROR;
+		rc = p->rc ? SQLITE_ERROR : SQLITE_DONE;
  	}
  	goto vdbe_return;

And nothing is changed. The tests passed ok.

Why?


3. Please, put a new patch version at the end of letter.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0edbda1d9..4588b1c6d 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -4145,4 +4145,50 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
>  		sqlite3DbFree(db, pWith);
>  	}
>  }
> +
> +int
> +vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
> +				   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 = NULL;
> +	char *error = NULL;
> +	name = sqlite3DbStrDup(db, name_src);
> +	if (name == NULL) {
> +		size_t size = strlen(name_src) + 1;
> +		diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
> +		return -1;
> +	}
> +	error = sqlite3DbStrDup(db, error_src);
> +	if (error == NULL) {
> +		sqlite3DbFree(db, name);
> +		size_t size = strlen(error_src) + 1;
> +		diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
> +		return -1;
> +	}

4. Why do you need to separate char *name and *error announcements from
assignments?