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 C868229C95 for ; Thu, 22 Mar 2018 09:57:50 -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 ih_CobNv3kBh for ; Thu, 22 Mar 2018 09:57:50 -0400 (EDT) Received: from smtp2.mail.ru (smtp2.mail.ru [94.100.179.91]) (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 84AD929C89 for ; Thu, 22 Mar 2018 09:57:50 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: [tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine From: "v.shpilevoy@tarantool.org" In-Reply-To: <71a9b8ac842931885bb91551c2466c670f238d10.1521583434.git.korablev@tarantool.org> Date: Thu, 22 Mar 2018 16:57:45 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <33D8E3FE-EA0B-4426-8CC0-BCCB448AD239@tarantool.org> References: <71a9b8ac842931885bb91551c2466c670f238d10.1521583434.git.korablev@tarantool.org> 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: Nikita Pettik See below 4 comments. > 21 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 2:48, Nikita = Pettik =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= (=D0=B0): >=20 > 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. >=20 > 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(-) >=20 > 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 =3D sqlite3GetVdbe(pParse); > assert(v !=3D 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 =3D 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 =3D 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? >=20 > + /* > + * Drop all _space and _index entries that refer to the > + * table. > */ > - int space_id =3D 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\"=3D%d AND \"iid\">0", > - TARANTOOL_SYS_INDEX_NAME, = space_id); > + struct space *space =3D space_by_id(space_id); > + assert(space !=3D NULL); > + uint32_t index_count =3D 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 =3D > + (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. >=20 > @@ -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 =3D = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); > - assert(space !=3D NULL); > int space_ptr_reg =3D ++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 >=3D 0) ? > - OPFLAG_P2ISREG : 0)); > + sqlite3VdbeChangeP5(v, OPFLAG_FRESH_PTR); 4. OPFLAG_FRESH_PTR seems to be unused. Why do you need it? >=20