From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E8E0524284 for ; Mon, 18 Jun 2018 11:42:57 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zw1tVAoe76kQ for ; Mon, 18 Jun 2018 11:42:57 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id A5F8B24270 for ; Mon, 18 Jun 2018 11:42:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence References: <2dc4c354dc72123c3447831a6ac48038eaf95f48.1528997527.git.kshcherbatov@tarantool.org> <05ff2cb5-d224-7b04-5aa8-d7d57b7c0031@tarantool.org> From: Vladislav Shpilevoy Message-ID: <04bda6c6-247c-0ad4-b78d-701d1d34c974@tarantool.org> Date: Mon, 18 Jun 2018 18:42:55 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Shcherbatov , tarantool-patches@freelists.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; > }