From: "v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org Cc: Nikita Pettik <korablev@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine Date: Thu, 22 Mar 2018 16:57:45 +0300 [thread overview] Message-ID: <33D8E3FE-EA0B-4426-8CC0-BCCB448AD239@tarantool.org> (raw) In-Reply-To: <71a9b8ac842931885bb91551c2466c670f238d10.1521583434.git.korablev@tarantool.org> See below 4 comments. > 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org> написал(а): > > As far as new opcodes to operate on system spaces have been introduced, > there is no more need to open cursors on such tables, when it comes to > insert/delete operations. In addition, it allows to get rid of system > spaces lookups from SQL data dictionary. Moreover, previously DDL > relied on nested parse to make deletions from system space. But during > nested parse it is impossible to convert space id to space pointer (i.e. > emit OP_SIDtoPtr opcode) without any overhead or "hacks". So, nested > parse has been replaced with hardcoded sequences of opcodes, > implementing the same logic. > > Closes #3252 > --- > src/box/sql/build.c | 322 ++++++++++++++++++++++--------------------- > src/box/sql/sqliteInt.h | 5 + > src/box/sql/trigger.c | 27 ++-- > test/sql/transition.result | 11 +- > test/sql/transition.test.lua | 8 +- > 5 files changed, 190 insertions(+), 183 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 229c8b4d5..02d27dec6 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2238,9 +2182,11 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) 1. sqlite3CodeDropTable now is used in build.c only, and can be declared as static (and removed from sqliteInt.h). > v = sqlite3GetVdbe(pParse); > assert(v != 0); > sqlite3BeginWriteOperation(pParse, 1); > - > - /* Drop all triggers associated with the table being dropped. Code > - * is generated to remove entries from _trigger space. > + /* > + * Drop all triggers associated with the table being > + * dropped. Code is generated to remove entries from > + * _trigger. OP_DropTrigger will remove it from internal > + * SQL structures. > */ > pTrigger = pTab->pTrigger; > /* Do not account triggers deletion - they will be accounted > @@ -2254,58 +2200,115 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) > - > - /* Drop all _space and _index entries that refer to the > - * table. The program loops through the _index & _space tables and deletes > - * every row that refers to a table. > - * Triggers are handled separately because a trigger can be > - * created in the temp database that refers to a table in another > - * database. > + /* Delete entry by from _space_sequence. */ > + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, > + idx_rec_reg); > + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID, > + idx_rec_reg); > + VdbeComment((v, "Delete entry from _space_sequence")); > + /* > + * Program below implement analogue of SQL statement: > + * "DELETE FROM BOX_SEQUENCE WHERE name = zName;" > + * However, we can't use nested parse for DDL, > + * since pointers to spaces will expire. > + */ 2. Why do you delete by name, if you have struct space.sequence->def->id? > > + /* > + * Drop all _space and _index entries that refer to the > + * table. > */ > - int space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum); > if (!isView) { > - if (pTab->pIndex && pTab->pIndex->pNext) { > - /* Remove all indexes, except for primary. > - Tarantool won't allow remove primary when secondary exist. */ > - sqlite3NestedParse(pParse, > - "DELETE FROM \"%s\" WHERE \"id\"=%d AND \"iid\">0", > - TARANTOOL_SYS_INDEX_NAME, space_id); > + struct space *space = space_by_id(space_id); > + assert(space != NULL); > + uint32_t index_count = space->index_count; > + if (index_count > 1) { > + /* > + * Remove all indexes, except for primary. > + * Tarantool won't allow remove primary when > + * secondary exist. > + */ > + uint32_t *iids = > + (uint32_t *) sqlite3Malloc(sizeof(uint32_t) * > + (index_count - 1)); 3.1. Check for OOM; 3.2. For such short lived allocations you can use region_alloc - it allows to avoid multiple free() calls on each alloc, and free all temp allocations at once after a transaction is finished. > > @@ -2603,16 +2606,18 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) > sqlite3VdbeJumpHere(v, addr1); > if (memRootPage < 0) > sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); > - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); > - assert(space != NULL); > int space_ptr_reg = ++pParse->nMem; > - sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, > - ((int64_t) space)); > + /* > + * OP_Clear can use truncate optimization > + * (i.e. insert record into _truncate) and schema > + * may change. Thus, use dynamic conversion from > + * space id to ptr. > + */ > + sqlite3VdbeAddOp2(v, OP_SIDtoPtr, SQLITE_PAGENO_TO_SPACEID(tnum), > + space_ptr_reg); > sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg, > (char *)pKey, P4_KEYINFO); > - sqlite3VdbeChangeP5(v, > - OPFLAG_BULKCSR | ((memRootPage >= 0) ? > - OPFLAG_P2ISREG : 0)); > + sqlite3VdbeChangeP5(v, OPFLAG_FRESH_PTR); 4. OPFLAG_FRESH_PTR seems to be unused. Why do you need it? >
next prev parent reply other threads:[~2018-03-22 13:57 UTC|newest] Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Nikita Pettik 2018-03-21 13:14 ` [tarantool-patches] " Kirill Yukhin 2018-03-22 10:07 ` n.pettik 2018-03-22 11:04 ` v.shpilevoy 2018-03-23 16:01 ` n.pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 2/4] sql: introduce opcodes to operate on system spaces Nikita Pettik 2018-03-22 11:42 ` [tarantool-patches] " v.shpilevoy 2018-03-22 12:23 ` n.pettik 2018-03-22 13:09 ` v.shpilevoy 2018-03-23 16:20 ` n.pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 3/4] sql: rework code generation for DDL routine Nikita Pettik 2018-03-22 13:57 ` v.shpilevoy [this message] 2018-03-23 16:33 ` [tarantool-patches] " n.pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead Nikita Pettik 2018-03-22 14:11 ` [tarantool-patches] " v.shpilevoy 2018-03-23 16:39 ` n.pettik 2018-03-24 12:37 ` [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime v.shpilevoy 2018-03-27 16:28 ` n.pettik
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=33D8E3FE-EA0B-4426-8CC0-BCCB448AD239@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine' \ /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