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 38DF426115 for ; Thu, 14 Jun 2018 12:12:40 -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 ehbP4u357Uru for ; Thu, 14 Jun 2018 12:12:40 -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 BF24920341 for ; Thu, 14 Jun 2018 12:12:39 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server References: From: Kirill Shcherbatov Message-ID: Date: Thu, 14 Jun 2018 19:12:37 +0300 MIME-Version: 1.0 In-Reply-To: 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. On_replace_dd_trigger works when a single trigger is created, dropped > or updated. So 'triggers' here is incorrect. Lets name it > on_replace_trigger_rollback. And on_replace_trigger_commit the second > function. static void -triggers_task_rollback(struct trigger *trigger, void *event) +on_replace_trigger_rollback(struct trigger *trigger, void *event) static void -triggers_task_commit(struct trigger *trigger, void * /* event */) +on_replace_trigger_commit(struct trigger *trigger, void * /* event */) struct trigger *on_rollback = - txn_alter_trigger_new(triggers_task_rollback, NULL); + txn_alter_trigger_new(on_replace_trigger_rollback, NULL); struct trigger *on_commit = - txn_alter_trigger_new(triggers_task_commit, NULL); + txn_alter_trigger_new(on_replace_trigger_commit, NULL); > 2. Please, define separate enums for _trigger field numbers in > schema_def.h like it is done for other system spaces. I think, the > previous patch is the perfect place for it. --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -3160,7 +3160,8 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) /* DROP trigger. */ uint32_t trigger_name_len; const char *trigger_name_src = - tuple_field_str_xc(old_tuple, 0, &trigger_name_len); + tuple_field_str_xc(old_tuple, BOX_TRIGGER_FIELD_NAME, + &trigger_name_len); char *trigger_name = (char *)region_alloc_xc(&fiber()->gc, trigger_name_len + 1); @@ -3180,10 +3181,13 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) /* INSERT, REPLACE trigger. */ uint32_t trigger_name_len; const char *trigger_name_src = - tuple_field_str_xc(new_tuple, 0, &trigger_name_len); + tuple_field_str_xc(new_tuple, BOX_TRIGGER_FIELD_NAME, + &trigger_name_len); const char *space_opts = - tuple_field_with_type_xc(new_tuple, 2, MP_MAP); + tuple_field_with_type_xc(new_tuple, + BOX_TRIGGER_FIELD_OPTS, + MP_MAP); struct space_opts opts; struct region *region = &fiber()->gc; space_opts_decode(&opts, space_opts, region); @@ -3200,7 +3204,9 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) "tuple trigger name does not match extracted " "from SQL"); } - uint32_t space_id = tuple_field_u32_xc(new_tuple, 1); + uint32_t space_id = + tuple_field_u32_xc(new_tuple, + BOX_TRIGGER_FIELD_SPACE_ID); > 3. Incorrect alignment. int rc = sql_trigger_replace(sql_get(), trigger_name, NULL, - &old_trigger); + &old_trigger); > 4. When you worked on CHECKs, I found a bug with strncmp() usage. Please, > fix it here too. Strncmp considers two strings be equal even if their > lengths are different. - if (strncmp(trigger_name_src, trigger_name, - trigger_name_len) != 0) { + if (strlen(trigger_name) != trigger_name_len || + memcmp(trigger_name_src, trigger_name, + trigger_name_len) != 0) { > 5. What is the point of is_insert? If it is true, then old_trigger == NULL, > so the line below is the same as just on_commit->data = old_trigger. It is > not? Yes, you are right. on_commit->data = old_trigger; is enough. > 6. Why not just > > for _, t in _trigger.index.space_id:pairs({space_id}) do > _trigger:delete({t.name}) > end Accepted. - local triggers = _trigger.index["space_id"]:select({space_id}) - for i = #triggers, 1, -1 do - local v = triggers[i] - _trigger:delete{v[1]} + for _, t in _trigger.index.space_id:pairs({space_id}) do + _trigger:delete({t.name}) end > 7. What is schema:delete? - * initiated in LUA schema:delete. + * initiated in LUA part of deletion process. > 8. This and the previous diff are unnecessary. ok. > 9. Same as in the reviews for previous patches. Why not just if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK && parser.rc != SQL_TARANTOOL_ERROR) diag_set(ClientError, ER_SQL, sql_error); else trigger = parser.pNewTrigger; > 10. You can remove > from parser_destroy if here will nullify pNewTrigger like > sqlite3FinishTrigger. Parser_destroy is called much more > frequently, so lets optimize it to avoid branching when > possible. Not this way, but it is possible: diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index e43fbcb..1fe6deb 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser) } parser->disableLookaside = 0; sqlite3DbFree(db, parser->zErrMsg); + sql_trigger_delete(db, parser->pNewTrigger); region_destroy(&parser->region); } diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 16e2111..e06cc0a 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -534,12 +534,6 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg) if (pParse->pWithToFree) sqlite3WithDelete(db, pParse->pWithToFree); - /* - * Trigger is exported with pNewTrigger field when - * parse_only flag is set. - */ - if (!pParse->parse_only) - sql_trigger_delete(db, pParse->pNewTrigger); sqlite3DbFree(db, pParse->pVList); while (pParse->pZombieTab) { Table *p = pParse->pZombieTab; @@ -587,10 +581,12 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql) char *sql_error; struct Trigger *trigger = NULL; if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK && - parser.rc != SQL_TARANTOOL_ERROR) + parser.rc != SQL_TARANTOOL_ERROR) { diag_set(ClientError, ER_SQL, sql_error); - else + } else { trigger = parser.pNewTrigger; + parser.pNewTrigger = NULL; + } sql_parser_destroy(&parser); return trigger; } > 11. Forgot nErr++> 12. Same. Please, check in other places yourself. --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -127,6 +127,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s if (!noErr) { diag_set(ClientError, ER_TRIGGER_EXISTS, zName); pParse->rc = SQL_TARANTOOL_ERROR; + pParse->nErr++; } else { assert(!db->init.busy); } @@ -138,11 +139,13 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name), &space_id) != 0) { pParse->rc = SQL_TARANTOOL_ERROR; + pParse->nErr++; goto trigger_cleanup; } if (space_id == BOX_ID_NIL) { diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); pParse->rc = SQL_TARANTOOL_ERROR; + pParse->nErr++; goto trigger_cleanup; } @@ -168,6 +171,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s (tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER", table_name); diag_set(ClientError, ER_SQL, error); pParse->rc = SQL_TARANTOOL_ERROR; + pParse->nErr++; goto trigger_cleanup; } if (!space->def->opts.is_view && tr_tm == TK_INSTEAD) { @@ -177,6 +181,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s table_name); diag_set(ClientError, ER_SQL, error); pParse->rc = SQL_TARANTOOL_ERROR; + pParse->nErr++; goto trigger_cleanup; } > 13. Fits in one line. - struct space *space = - space_cache_find(src_trigger->space_id); + struct space *space = space_cache_find(src_trigger->space_id); > 14. Please, apply. - for (struct Trigger *p = trigger_list; p; p = p->pNext) { - if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) + for (struct Trigger *p = trigger_list; p != NULL; p = p->pNext) { + if (p->op == op && checkColumnOverlap(p->pColumns, + pChanges) != 0) mask |= p->tr_tm; } - if (pMask != 0) + if (pMask != NULL) *pMask = mask; - return (mask != 0) ? trigger_list : 0; + return (mask != 0) ? trigger_list : NULL; > 15. != NULL. Please, check in other places yourself. assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id); - assert(pTrigger->zName == 0 || pTab->def->id == pTrigger->space_id); + assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id); > 16. space is already got in this opcode above. No! That pointer become invalid with sqlite3UnlinkAndDeleteTable, sqlite3InitCallback calls. Now there is a new instance of space. > 17. Why not this? > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 2d1538e24..98b7d95a0 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4761,15 +4761,14 @@ case OP_RenameTable: { > assert(space != NULL); > > /* Rename all triggers created on this table. */ > - for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) { > - struct Trigger *next_trigger = trigger->pNext; > + for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; > + trigger = trigger->pNext) { > rc = tarantoolSqlite3RenameTrigger(trigger->zName, > zOldTableName, zNewTableName); > if (rc != SQLITE_OK) { > sqlite3ResetAllSchemasOfConnection(db); > goto abort_due_to_error; > } > - trigger = next_trigger; > } > I believe that to the moment of assignment next pointer source pointer could be already destructing and this value could be invalid. + /* Store pointer as trigger will be destructed. */ > 18. 'inmutable' does not exist. Maybe 'immutable'? > > In Lua you have tuple field access by name and table.insert() to append a value. So > lets use them to make the code more understandable: > > for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end - function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} end return r end + function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end > 19. Your tests looks failing. Or is it ok? Then why do you need 'immutable_part' here, > if :insert() fails before the call? Looks like rudimentary part of some old test, dropped.