> On 22 Mar 2018, at 16:57, v.shpilevoy@tarantool.org wrote: > > See below 4 comments. > >> 21 марта 2018 г., в 2:48, Nikita Pettik > написал(а): >> >> 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). Done: --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2306,7 +2250,7 @@ sqlite3ClearStatTables(Parse * pParse, /* The parsing context */ /* * Generate code to drop a table. */ -void +static void sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3007,7 +3012,6 @@ int sqlite3ViewGetColumnNames(Parse *, Table *); int sqlite3DbMaskAllZero(yDbMask); #endif void sqlite3DropTable(Parse *, SrcList *, int, int); -void sqlite3CodeDropTable(Parse *, Table *, int); > >> 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? To be honest, I forgot about such opportunity. Fetching id directly from sequence allows to significantly reduce number of opcodes and simplify query logic: + /* Delete entry by id from _sequence. */ + assert(space->sequence != NULL); + int sequence_id_reg = ++pParse->nMem; + sqlite3VdbeAddOp2(v, OP_Integer, space->sequence->def->id, + sequence_id_reg); + sqlite3VdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1, + idx_rec_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg); + VdbeComment((v, "Delete entry from _sequence")); + } > >> >> + /* >> + * 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. Ok, substituted with region_alloc: + size_t size = sizeof(uint32_t) * (index_count - 1); + uint32_t *iids = + (uint32_t *) region_alloc(&fiber()->gc, size); + if (iids == NULL) { + diag_set(OutOfMemory, size, "region_alloc", + "iids"); + return; + } >> >> @@ -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? I need it to indicate that pointer can’t become expired. Its usage is introduced in the last patch.