From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Kirill Shcherbatov <kshcherbatov@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence Date: Wed, 13 Jun 2018 21:53:14 +0300 [thread overview] Message-ID: <6b41fbc2-4bae-7acd-e79c-5b0128396322@tarantool.org> (raw) In-Reply-To: <34f76794d133bf7ae6ff076494b1d3cb3edcf841.1528535873.git.kshcherbatov@tarantool.org> Thanks for the patch! See 12 comments below. On 09/06/2018 12:32, Kirill Shcherbatov wrote: > Trigger presence in system should be tested on each VDBE > execution attempt, not on Parser iteration. > > Part of #3435, #3273 > --- > src/box/sql.c | 35 +++++++++++++++++++++++++++++++++++ > src/box/sql/main.c | 13 +++++++++++-- > src/box/sql/sqliteInt.h | 20 ++++++++++++++++++++ > src/box/sql/trigger.c | 20 +++++++++----------- > src/box/sql/vdbe.c | 14 ++++++++++---- > src/box/sql/vdbeapi.c | 5 +++-- > 6 files changed, 88 insertions(+), 19 deletions(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index 57211fd..f539085 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1836,3 +1836,38 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list, > } > return 0; > } > + > +int > +vdbe_abort_execution_on_exists(Parse *parser, int space_id, int index_id, > + const char *name, int tarantool_error_code, > + bool no_error) 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: parser_emit_execution_halt_on_exists 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. > +{ > + Vdbe *v = sqlite3GetVdbe(parser); 4. struct Vdbe. Please check other places yourself. > diff --git a/src/box/sql/main.c b/src/box/sql/main.c > index 0acf7bc..f2334cb 100644 > --- a/src/box/sql/main.c > +++ b/src/box/sql/main.c > @@ -1456,8 +1456,17 @@ sqlite3_errmsg(sqlite3 * db) > 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) { > + testcase(db->pErr == 0); 5. What is 'testcase'? Why do you need it here? > + z = (char *) sqlite3_value_text(db->pErr); 6. It is assigned to this value few lines above. > + assert(!db->mallocFailed); > + if (z == NULL) > + z = sqlite3ErrStr(db->errCode); > + } else { > + struct diag *diag = diag_get(); > + assert(diag != NULL); 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. > + struct error *error = diag_last_error(diag); > + z = error->errmsg; > } > } > return z; > 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'. > + * @param tarantool_error_code Tarantool error to raise on object exists. 9. Out of 66. > + * @param no_error Do not raise error flag. > + * > + * @retval -1 on memory allocation error. > + * @retval 0 on success. > + */ > +int > +vdbe_abort_execution_on_exists(Parse *parser, int space_id, int index_id, > + const char *name, int tarantool_error_code, > + bool no_error); > + > #endif /* SQLITEINT_H */ > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index 1b569bc..92747f0 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -178,6 +167,15 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s > pParse->rc = SQL_TARANTOOL_ERROR; > goto trigger_cleanup; > } > + if (!pParse->parse_only) { > + if (vdbe_abort_execution_on_exists(pParse, BOX_TRIGGER_ID, 0, > + zName, > + ER_TRIGGER_EXISTS, > + (noErr != 0)) != 0) { > + pParse->rc = SQL_TARANTOOL_ERROR; 10. pParse->nErr++. Or find a way to delete nErr. It is very annoying and useless attribute. > + goto trigger_cleanup; > + } > + } > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 2d1538e..0696cad 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. 11. Why here you did not put empty line after 'if P1', but did in the next hunk? > * > * 0: (no change) > @@ -968,6 +968,8 @@ 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. > * > @@ -1005,9 +1007,10 @@ 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 (pOp->p5 != 0 && p->rc == SQL_TARANTOOL_ERROR) { > + diag_set(ClientError, pOp->p5, pOp->p4.z); 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. > + } else if (pOp->p5 != 0) { > static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK", > "FOREIGN KEY" }; > testcase( pOp->p5==1);
next prev parent reply other threads:[~2018-06-13 18:53 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-06-09 9:58 [tarantool-patches] [PATCH v2 00/11] sql: remove Triggers to server Kirill Shcherbatov 2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 10/11] sql: move " Kirill Shcherbatov 2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-14 16:12 ` Kirill Shcherbatov 2018-06-28 7:18 ` Konstantin Osipov 2018-06-28 7:33 ` Kirill Shcherbatov 2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 11/11] sql: VDBE tests for trigger existence Kirill Shcherbatov 2018-06-13 18:53 ` Vladislav Shpilevoy [this message] 2018-06-14 16:12 ` [tarantool-patches] " Kirill Shcherbatov 2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 02/11] box: move db->pShchema init to sql_init Kirill Shcherbatov 2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 04/11] sql: fix sql len in tarantoolSqlite3RenameTrigger Kirill Shcherbatov 2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 06/11] sql: refactor sql_expr_compile to return AST Kirill Shcherbatov 2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-14 16:12 ` Kirill Shcherbatov 2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 07/11] sql: move sqlite3DeleteTrigger to sql.h Kirill Shcherbatov 2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-14 16:12 ` Kirill Shcherbatov 2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 08/11] box: sort error codes in misc.test Kirill Shcherbatov 2018-06-09 9:32 ` [tarantool-patches] [PATCH v2 09/11] sql: new _trigger space format with space_id Kirill Shcherbatov 2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-14 16:12 ` Kirill Shcherbatov 2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 01/11] box: remove migration from alpha 1.8.2 and 1.8.4 Kirill Shcherbatov 2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 03/11] sql: fix leak on CREATE TABLE and resolve self ref Kirill Shcherbatov 2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-14 16:12 ` Kirill Shcherbatov 2018-06-09 9:58 ` [tarantool-patches] [PATCH v2 05/11] box: port schema_find_id to C Kirill Shcherbatov 2018-06-13 18:53 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-14 16:12 ` Kirill Shcherbatov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6b41fbc2-4bae-7acd-e79c-5b0128396322@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=kshcherbatov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox