[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Jun 18 18:42:55 MSK 2018
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;
> }
More information about the Tarantool-patches
mailing list