> On 3 Apr 2018, at 21:27, Vladislav Shpilevoy wrote: > > See 6 comments below. > > 03.04.2018 19:14, Nikita Pettik пишет: >> As a part of SQL data dictionary integration, 'DROP INDEX' and >> 'DROP TABLE' statements proccessing has been refactored in order >> to avoid using SQL specific internal structures. >> However, triggers still aren't transfered to server, so to drop them >> it is required to lookup SQL table and emit apporpriate opcodes. >> Also, added comments and fixed codestyle for functions processing >> 'DROP' routine. >> >> Part of #3222. >> --- >> src/box/sql/build.c | 241 ++++++++++++++++++++++++++---------------------- >> src/box/sql/parse.c | 6 +- >> src/box/sql/parse.y | 6 +- >> src/box/sql/sqliteInt.h | 6 +- >> 4 files changed, 140 insertions(+), 119 deletions(-) >> >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 61194e06b..219cc974b 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -46,6 +46,7 @@ >> #include "sqliteInt.h" >> #include "vdbeInt.h" >> #include "tarantoolInt.h" >> +#include "box/box.h" >> #include "box/sequence.h" >> #include "box/session.h" >> #include "box/identifier.h" >> @@ -2152,48 +2153,50 @@ sql_clear_stat_spaces(Parse * pParse, const char *zType, const char *zName) >> zType, zName); >> } >> -/* >> +/** >> * Generate code to drop a table. >> + * This routine includes dropping triggers, sequences, >> + * all indexes and entry from _space space. >> + * >> + * @param parse_context Current parsing context. >> + * @param space Space to be dropped. >> + * @param is_view True, if space is >> */ >> static void >> -sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) >> +sql_code_drop_table(struct Parse *parse_context, struct space *space, >> + bool is_view) >> { >> - Vdbe *v; >> - sqlite3 *db = pParse->db; >> - Trigger *pTrigger; >> - >> - v = sqlite3GetVdbe(pParse); >> >> + struct sqlite3 *db = parse_context->db; >> + struct Vdbe *v = sqlite3GetVdbe(parse_context); >> assert(v != 0); > 1. Lets in all new code use explicit ==/!= NULL. Fixed: - assert(v != 0); + assert(v != NULL); >> /* >> * 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 >> + * >> + * Do not account triggers deletion - they will be accounted > 2. Out of 66. Fixed: - * Do not account triggers deletion - they will be accounted - * in DELETE from _space below. + * Do not account triggers deletion - they will be + * accounted in DELETE from _space below. >> @@ -2214,7 +2216,7 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) >> * Drop all _space and _index entries that refer to the >> * table. >> */ >> - if (!isView) { >> + if (!is_view) { > 3. I looked at the indexes deletion more carefully and noticed, that you can avoid any allocations > here. Why can not you just walk through space->index array and generate OP_SDelete opcodes for > each index, starting from 1 (to skip primary)? AFAIK the entire index array is not changed until you > reach VdbeExec, so you can do not save index identifiers on region to generate opcodes. IDK why I didn’t do it in this way (probably due to my ‘paranoia’). Reworked: - uint32_t *iids = - (uint32_t *) region_alloc(&fiber()->gc, - sizeof(uint32_t) * - (index_count - 1)); - /* Save index ids to be deleted except for PK. */ for (uint32_t i = 1; i < index_count; ++i) { - iids[i - 1] = space->index[i]->def->iid; - } - for (uint32_t i = 0; i < index_count - 1; ++i) { - sqlite3VdbeAddOp2(v, OP_Integer, iids[i], + sqlite3VdbeAddOp2(v, OP_Integer, + space->index[i]->def->iid, space_id_reg + 1); sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, idx_rec_reg); @@ -2233,7 +2226,7 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space, idx_rec_reg); VdbeComment((v, "Remove secondary index iid = %u", - iids[i])); + space->index[i]->def->iid)); >> uint32_t index_count = space->index_count; >> if (index_count > 1) { >> /* >> @@ -2257,71 +2259,78 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) >> sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_ID, idx_rec_reg); >> sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); >> VdbeComment((v, "Delete entry from _space")); >> - /* Remove the table entry from SQLite's internal schema and modify >> - * the schema cookie. >> + /* >> + * Remove the table entry from SQLite's internal schema >> + * and modify the schema cookie. >> */ > 4. But you have deleted cookie updates in the previous commit. Source code of OP_DropTable and all > called there functions does not contain cookie changes. Removed obsolete comment: - /* - * Remove the table entry from SQLite's internal schema - * and modify the schema cookie. - */ + /* Remove the table entry from SQLite's internal schema. */ >> + sqlite3VdbeAddOp4(v, OP_DropTable, 0, 0, 0, space->def->name, 0); >> - sqlite3VdbeAddOp4(v, OP_DropTable, 0, 0, 0, pTab->zName, 0); >> - >> - /* FIXME: DDL is impossible inside a transaction so far. >> + /* >> * Replace to _space/_index will fail if active >> * transaction. So, don't pretend, that we are able to >> * anything back. To be fixed when transactions >> * DDL are enabled. >> */ >> - /* sqlite3ChangeCookie(pParse); */ >> sqliteViewResetAll(db); >> } >> -/* >> +/** >> * This routine is called to do the work of a DROP TABLE statement. >> - * pName is the name of the table to be dropped. >> + * >> + * @param parse_context Current parsing context. >> + * @param table_name_list List containing table name. >> + * @param is_view True, if statement is really 'DROP VIEW'. >> + * @param if_exists True, if statement contains 'IF EXISTS' clause. >> */ >> void >> -sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr) >> +sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list, >> + bool is_view, bool if_exists) >> { >> - Table *pTab; >> - Vdbe *v = sqlite3GetVdbe(pParse); >> - sqlite3 *db = pParse->db; >> - >> + struct Vdbe *v = sqlite3GetVdbe(parse_context); >> + struct sqlite3 *db = parse_context->db; >> if (v == NULL || db->mallocFailed) { >> goto exit_drop_table; >> } >> - /* Activate changes counting here to account >> + /* >> + * Activate changes counting here to account >> * DROP TABLE IF NOT EXISTS, if the table really does not >> * exist. >> */ >> - if (!pParse->nested) >> + if (!parse_context->nested) > 5. Please, use explicit != 0. Why you use !int, the variable seems to be boolean. It slightly confuses. This this not != check, but vice versa == 0 check. ! In this case would better fit, wouldn’t it? (Since any number except for 0 would be converted to false) >> sqlite3VdbeCountChanges(v); >> - assert(pParse->nErr == 0); >> - assert(pName->nSrc == 1); >> - assert(db->pSchema != NULL); >> - if (noErr) >> - db->suppressErr++; >> - assert(isView == 0 || isView == LOCATE_VIEW); >> - pTab = sqlite3LocateTable(pParse, isView, pName->a[0].zName); >> - if (noErr) >> - db->suppressErr--; >> - >> - if (pTab == 0) >> + assert(parse_context->nErr == 0); >> + assert(table_name_list->nSrc == 1); >> + assert(is_view == 0 || is_view == LOCATE_VIEW); >> + const char *space_name = table_name_list->a[0].zName; >> + uint32_t space_id = box_space_id_by_name(space_name, >> + strlen(space_name)); >> + if (space_id == BOX_ID_NIL) { >> + if (!is_view && !if_exists) >> + sqlite3ErrorMsg(parse_context, "no such table: %s", >> + space_name); >> + if (is_view && !if_exists) >> + sqlite3ErrorMsg(parse_context, "no such view: %s", >> + space_name); >> goto exit_drop_table; >> -#ifndef SQLITE_OMIT_VIEW >> - /* Ensure DROP TABLE is not used on a view, and DROP VIEW is not used >> - * on a table. >> + } >> + struct space *space = space_by_id(space_id); >> + assert(space != NULL); >> + /* >> + * Ensure DROP TABLE is not used on a view, >> + * and DROP VIEW is not used on a table. >> */ >> - if (isView && !space_is_view(pTab)) { >> - sqlite3ErrorMsg(pParse, "use DROP TABLE to delete table %s", >> - pTab->zName); >> + if (is_view && !space->def->opts.is_view) { >> + sqlite3ErrorMsg(parse_context, "use DROP TABLE to delete table %s", >> + space_name); >> goto exit_drop_table; >> } >> - if (!isView && space_is_view(pTab)) { >> - sqlite3ErrorMsg(pParse, "use DROP VIEW to delete view %s", >> - pTab->zName); >> + if (!is_view && space->def->opts.is_view) { >> + sqlite3ErrorMsg(parse_context, "use DROP VIEW to delete view %s", >> + space_name); >> goto exit_drop_table; >> } >> -#endif >> - >> - /* Generate code to remove the table from Tarantool and internal SQL >> + /* >> + * Generate code to remove the table from Tarantool and internal SQL > 6. Out of 66. Fixed whole comment (to fit into 66 chars): - * Generate code to remove the table from Tarantool and internal SQL - * tables. Basically, it consists from 3 stages: - * 1. Delete statistics from _stat1 and _stat4 tables (if any exist) - * 2. In case of presence of FK constraints, i.e. current table is child - * or parent, then start new transaction and erase from table - * all data row by row. On each deletion check whether any FK - * violations have occurred. If ones take place, then rollback - * transaction and halt VDBE. Otherwise, commit transaction. - * 3. Drop table by truncating (if step 2 was skipped), removing - * indexes from _index table and eventually tuple with corresponding - * space_id from _space. + * Generate code to remove the table from Tarantool + * and internal SQL tables. Basically, it consists + * from 3 stages: + * 1. Delete statistics from _stat1 and _stat4 tables. + * 2. In case of presence of FK constraints, i.e. current + * table is child or parent, then start new transaction + * and erase from table all data row by row. On each + * deletion check whether any FK violations have + * occurred. If ones take place, then rollback + * transaction and halt VDBE. + * 3. Drop table by truncating (if step 2 was skipped), + * removing indexes from _index space and eventually + * tuple with corresponding space_id from _space. */