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 0587E252A0 for ; Tue, 19 Jun 2018 11:12:19 -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 sMke4m3PTA6X for ; Tue, 19 Jun 2018 11:12:18 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 896812527D for ; Tue, 19 Jun 2018 11:12:18 -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> <04bda6c6-247c-0ad4-b78d-701d1d34c974@tarantool.org> <79bd10d4-579b-92b3-e1ed-755d33febb8b@tarantool.org> <4ed9ea53-a3b1-a633-9174-f7db97f3e40c@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Tue, 19 Jun 2018 18:12:16 +0300 MIME-Version: 1.0 In-Reply-To: <4ed9ea53-a3b1-a633-9174-f7db97f3e40c@tarantool.org> Content-Type: text/plain; charset=utf-8 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: tarantool-patches@freelists.org, "v.shpilevoy@tarantool.org" Trigger presence in system should be tested on each VDBE execution attempt, not on Parser iteration. 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 | 20 +++++++++++++------- test/sql-tap/view.test.lua | 8 ++++---- test/sql/view.result | 4 ++-- 7 files changed, 106 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, + 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"); + 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"); + 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); + sqlite3VdbeChangeP5(v, tarantool_error_code); + } + sqlite3VdbeAddOp1(v, OP_Close, cursor); + return 0; +} #endif /* !defined(SQLITE_OMIT_CTE) */ 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); + 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..831faf0 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. + * + * @param parser Parsing context. + * @param space_id Space to lookup identifier. + * @param index_id Index identifier containing string primary key. + * @param name Of object to test existence. + * @param tarantool_error_code to set on halt. + * @param error_src string to notify on halt. + * @param no_error Do not raise error flag. + * + * @retval -1 on memory allocation error. + * @retval 0 on success. + */ +int +vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id, + int index_id, const char *name, + int tarantool_error_code, + const char *error_src, bool no_error); + #endif /* SQLITEINT_H */ diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index e4c936d..8ccb646 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -122,18 +122,6 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK) goto trigger_cleanup; - if (!pParse->parse_only && - sqlite3HashFind(&db->pSchema->trigHash, zName) != NULL) { - if (!noErr) { - diag_set(ClientError, ER_TRIGGER_EXISTS, zName); - pParse->rc = SQL_TARANTOOL_ERROR; - pParse->nErr++; - } else { - assert(!db->init.busy); - } - goto trigger_cleanup; - } - const char *table_name = pTableName->a[0].zName; uint32_t space_id; if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name), @@ -179,6 +167,19 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s pParse->nErr++; goto trigger_cleanup; } + if (!pParse->parse_only) { + const char *error_msg = + tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS), zName); + if (vdbe_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID, + 0, zName, + ER_TRIGGER_EXISTS, + error_msg, + (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 c1df880..9f7f50d 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. * * 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,10 @@ 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); + box_error_set(__FILE__, __LINE__, ER_DROP_SPACE, + tnt_errcode_desc(ER_DROP_SPACE), + space->def->name, + "other views depend on this space"); rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; } diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua index 5dc9503..ac1a27d 100755 --- a/test/sql-tap/view.test.lua +++ b/test/sql-tap/view.test.lua @@ -127,7 +127,7 @@ test:do_catchsql_test( DROP TABLE t1; ]], { -- - 1, "Can't drop table T1: other views depend on this space" + 1, "Can't drop space 'T1': other views depend on this space" -- }) @@ -1185,7 +1185,7 @@ test:do_catchsql_test( DROP TABLE t11; ]], { -- - 1, "Can't drop table T11: other views depend on this space" + 1, "Can't drop space 'T11': other views depend on this space" -- }) @@ -1195,7 +1195,7 @@ test:do_catchsql_test( DROP TABLE t12; ]], { -- - 1, "Can't drop table T12: other views depend on this space" + 1, "Can't drop space 'T12': other views depend on this space" -- }) @@ -1205,7 +1205,7 @@ test:do_catchsql_test( DROP TABLE t13; ]], { -- - 1, "Can't drop table T13: other views depend on this space" + 1, "Can't drop space 'T13': other views depend on this space" -- }) diff --git a/test/sql/view.result b/test/sql/view.result index 0b9dc55..b033f19 100644 --- a/test/sql/view.result +++ b/test/sql/view.result @@ -124,7 +124,7 @@ box.sql.execute("CREATE VIEW v2 AS SELECT * FROM t2;"); ... box.sql.execute("DROP TABLE t2;"); --- -- error: 'Can''t drop table T2: other views depend on this space' +- error: 'Can''t drop space ''T2'': other views depend on this space' ... sp = box.space._space:get{box.space.T2.id}; --- @@ -134,7 +134,7 @@ sp = box.space._space:replace(sp); ... box.sql.execute("DROP TABLE t2;"); --- -- error: 'Can''t drop table T2: other views depend on this space' +- error: 'Can''t drop space ''T2'': other views depend on this space' ... box.sql.execute("DROP VIEW v2;"); --- -- 2.7.4