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

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

Thanks for the patch! See 6 comments below.

On 15/06/2018 19:21, Kirill Shcherbatov wrote:
>> Unfortunately we have the problem with error objects creating. ClientError in
>> the constructor increments error counter in statistics visible to user. So this
>> still not happened error affects the public API. We need to find another way.
>>
>> I think, we may assume that this way of error setting will be used for
>> ClientErrors only. Indeed, OOM is set explicitly in the place where emerged. Other
>> errors are not used in VDBE. The only hindrance is that ClientErrors have
>> different argument count. But I have found function box_error_set() in error.h.
>> It is a public API to set ClientError with any message.
>>
>> So lets return to the previous implementation, but instead of passing
>> (error code; code arguments) lets pass (error code; the full error message) and
>> use box_error_set() instead of direct diag_set(ClientError, ...).
>>
>> To repeat the same error message format as from the code you can use
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0edbda1..8db9231 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -4145,4 +4145,51 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
>   		sqlite3DbFree(db, pWith);
>   	}
>   }
> +
> +int
> +parser_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)

1. Sorry, after discussion with Nikita we have decided to
use 'vdbe_' prefix for emitters even if they take parser in
first argument.

2. Mismatch of parameter names: error in header, error_src in source.

> +{
> +	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)
> +		error = sqlite3DbStrDup(db, error_src);
> +	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);

3. Please, reduce the mess. Separate these OOM handlers.

> +		sqlite3DbFree(db, name);
> +		sqlite3DbFree(db, error);
> +		return -1;
> +	}
> +
> +	int cursor = parser->nTab++;
> +	int entity_id =
> +		SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space_id, index_id);
> +	emit_open_cursor(parser, cursor, entity_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 {
> +		error = sqlite3DbStrDup(db, error);

4. This error is duplicated already above, it is not?

> +		sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> +				  ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
> +		sqlite3VdbeChangeP5(v, tarantool_error_code);
> +	}
> +	sqlite3VdbeAddOp1(v, OP_Close, cursor);
> +	return 0;
> +}
>   #endif				/* !defined(SQLITE_OMIT_CTE) */
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index ec420ea..eadef53 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -184,6 +172,20 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
>   		pParse->nErr++;
>   		goto trigger_cleanup;
>   	}
> +	if (!pParse->parse_only) {
> +		char error[DIAG_ERRMSG_MAX];
> +		snprintf(error, DIAG_ERRMSG_MAX,
> +			 tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);

5. Please, use tt_sprintf. Do not declare big arrays on the
stack when possible.

> +		if (parser_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
> +							 0, zName,
> +							 ER_TRIGGER_EXISTS,
> +							 error,
> +							 (noErr != 0)) != 0) {
> +			pParse->rc = SQL_TARANTOOL_ERROR;
> +			pParse->nErr++;
> +			goto trigger_cleanup;
> +		}
> +	}
>   
>   	/*
>   	 * INSTEAD OF triggers can only appear on views and BEFORE triggers
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c035ffe..5f88e67 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1029,7 +1033,10 @@ case OP_Halt: {
>   		p->rc = SQLITE_BUSY;
>   	} else {
>   		assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT);
> -		rc = p->rc ? SQLITE_ERROR : SQLITE_DONE;
> +		if (p->rc != SQL_TARANTOOL_ERROR)
> +			rc = (p->rc != SQLITE_OK) ? SQLITE_ERROR : SQLITE_DONE;
> +		else
> +			rc = SQL_TARANTOOL_ERROR;

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.

>   	}
>   	goto vdbe_return;
>   }