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 134AE26115 for ; Thu, 14 Jun 2018 12:12:32 -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 116A7TBSpnp7 for ; Thu, 14 Jun 2018 12:12:31 -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 9AEEE20341 for ; Thu, 14 Jun 2018 12:12:31 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence References: <34f76794d133bf7ae6ff076494b1d3cb3edcf841.1528535873.git.kshcherbatov@tarantool.org> <6b41fbc2-4bae-7acd-e79c-5b0128396322@tarantool.org> From: Kirill Shcherbatov Message-ID: <61adfbc6-167e-5487-45eb-76d74a0eb97a@tarantool.org> Date: Thu, 14 Jun 2018 19:12:29 +0300 MIME-Version: 1.0 In-Reply-To: <6b41fbc2-4bae-7acd-e79c-5b0128396322@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 Cc: "v.shpilevoy@tarantool.org" > 1. If this function is method of parser then its prefix must be 'parser_'. > And here you do not an abortion. You emit the code of abortion if needed. > So this function can be named like this: > 2. struct Parse > 3. Why is the function stored in sql.c? As far I remember sql.c/.h is a > link between sqlite and Tarantool code, but vdbe_abort_execution_on_exists > is used in sqlite only. Moved to build.c. +int +parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id, + int index_id, const char *name, + int tarantool_error_code, bool no_error) > 4. struct Vdbe. Please check other places yourself. + struct Vdbe *v = sqlite3GetVdbe(parser); + struct sqlite3 *db = parser->db; > 5. What is 'testcase'? Why do you need it here? > 6. It is assigned to this value few lines above. > 7. diag is never NULL, because TX thread consists of fibers, and > each fiber has diag. Please, remove the assertion and just inline > diag_get() on the next line. @@ -1454,19 +1454,14 @@ 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 (db->errCode != SQL_TARANTOOL_ERROR) { - testcase(db->pErr == 0); - z = (char *) sqlite3_value_text(db->pErr); assert(!db->mallocFailed); + z = (char *)sqlite3_value_text(db->pErr); if (z == NULL) z = sqlite3ErrStr(db->errCode); } else { - struct diag *diag = diag_get(); - assert(diag != NULL); - struct error *error = diag_last_error(diag); - z = error->errmsg; + z = diag_last_error(diag_get())->errmsg; } } >> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h >> index 276f881..4f7dcbe 100644 >> --- a/src/box/sql/sqliteInt.h >> +++ b/src/box/sql/sqliteInt.h >> @@ -4537,4 +4537,24 @@ 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. >> + * >> + * @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 existency. > > 8. No such word 'existency'. Songs good for me (https://en.oxforddictionaries.com/definition/existency): 1The fact or state of existing; continuance of being. 2Something that exists; a being, an entity; = "existence". - * @param name Of object to test existency. + * @param name Of object to test existence. > 9. Out of 66. + * @param tarantool_error_code error to raise on object exists. > 10. pParse->nErr++. Or find a way to delete nErr. It is very annoying and > useless attribute. + pParse->nErr++; > 11. Why here you did not put empty line after 'if P1', but did in the > next hunk? > 12. I am afraid of this way will not work, when a code has multiple > arguments. And when type is not ClientError. And when it has a single > non-string argument. And when we want to know where the error was > generated. Here diag_set always saves filename "vdbe.c" and line "1012". > I think we should find another way to emit an error. One possible way > is to create and error object during parsing and save it by pointer in > p4. When the error occurs, the error object is set into the diag object. > But maybe you will find another way. I've done this this way: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index c52c41d..0a731df 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -4175,8 +4175,9 @@ sqlite3WithDelete(sqlite3 * db, With * pWith) int parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id, int index_id, const char *name, - int tarantool_error_code, bool no_error) + struct error *error, bool no_error) { + error_ref(error); struct Vdbe *v = sqlite3GetVdbe(parser); assert(v != NULL); @@ -4197,11 +4198,12 @@ parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id, P4_DYNAMIC); sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1); if (no_error) { + error_unref(error); sqlite3VdbeAddOp0(v, OP_Halt); } else { sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, - ON_CONFLICT_ACTION_FAIL, 0, name, P4_STATIC); - sqlite3VdbeChangeP5(v, tarantool_error_code); + ON_CONFLICT_ACTION_FAIL, 0, (void *)error, + P4_ERROROBJ); } sqlite3VdbeAddOp1(v, OP_Close, cursor); return 0; diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 4f755e4..d86ace5 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -4546,7 +4546,7 @@ table_column_nullable_action(struct Table *tab, uint32_t column); * @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 error to raise on object exists. + * @param error object to raise on target exists. * @param no_error Do not raise error flag. * * @retval -1 on memory allocation error. @@ -4555,6 +4555,6 @@ table_column_nullable_action(struct Table *tab, uint32_t column); int parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id, int index_id, const char *name, - int tarantool_error_code, bool no_error); + struct error *error, bool no_error); #endif /* SQLITEINT_H */ diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index d7ecdf1..4cb89fe 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -173,13 +173,13 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s goto trigger_cleanup; } if (!pParse->parse_only) { + struct error *error = + error_object(ClientError, ER_TRIGGER_EXISTS, zName); if (parser_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID, - 0, - zName, - ER_TRIGGER_EXISTS, + 0, zName, error, (noErr != 0)) != 0) { pParse->rc = SQL_TARANTOOL_ERROR; pParse->nErr++; goto trigger_cleanup; } } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dfc7b71..c3109ab 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -959,7 +959,7 @@ case OP_HaltIfNull: { /* in3 */ * VDBE, but do not rollback the transaction. * * If P4 is not null then it is an error message string. - * If P1 is not SQL_TARANTOOL_ERROR, + * * P5 is a value between 0 and 4, inclusive, that modifies the P4 string. * * 0: (no change) @@ -968,8 +968,6 @@ case OP_HaltIfNull: { /* in3 */ * 3: CHECK constraint failed: P4 * 4: FOREIGN KEY constraint failed: P4 * - * If P1 is SQL_TARANTOOL_ERROR, P5 contain ClientError code. - * * If P5 is not zero and P4 is NULL, then everything after the ":" is * omitted. * @@ -1008,8 +1006,8 @@ case OP_Halt: { p->errorAction = (u8)pOp->p2; p->pc = pcx; if (p->rc) { - if (pOp->p5 != 0 && p->rc == SQL_TARANTOOL_ERROR) { - diag_set(ClientError, pOp->p5, pOp->p4.z); + if (p->rc == SQL_TARANTOOL_ERROR) { + diag_add_error(diag_get(), (struct error *)pOp->p4.z); } else if (pOp->p5 != 0) { static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK", "FOREIGN KEY" }; diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h index 77fa41a..66e6954 100644 --- a/src/box/sql/vdbe.h +++ b/src/box/sql/vdbe.h @@ -149,6 +149,8 @@ typedef struct VdbeOpList VdbeOpList; #define P4_PTR (-18) /* P4 is a generic pointer */ #define P4_KEYDEF (-19) /* P4 is a pointer to key_def structure. */ #define P4_SPACEPTR (-20) /* P4 is a space pointer */ +/** P4 is an error object. */ +#define P4_ERROROBJ (-21) /* Error message codes for OP_Halt */ #define P5_ConstraintNotNull 1 diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 7b3c13d..8d89894 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -981,6 +981,10 @@ freeP4(sqlite3 * db, int p4type, void *p4) } break; } + case P4_ERROROBJ: { + error_unref(p4); + break; + } } } diff --git a/src/diag.h b/src/diag.h index 0ccf86d..502b294 100644 --- a/src/diag.h +++ b/src/diag.h @@ -261,6 +261,9 @@ struct error * BuildSocketError(const char *file, unsigned line, const char *socketname, const char *format, ...); +#define error_object(class, ...) \ + Build##class(__FILE__, __LINE__, ##__VA_ARGS__); + #define diag_set(class, ...) do { \ /* Preserve the original errno. */ \ int save_errno = errno; \