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 531A72BD8A for ; Tue, 3 Apr 2018 14:27:43 -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 iM-yhnRblA7r for ; Tue, 3 Apr 2018 14:27:43 -0400 (EDT) Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 0DF5C24236 for ; Tue, 3 Apr 2018 14:27:42 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: rework 'DROP INDEX' and 'DROP TABLE' handling References: <2bccf40f38d3e3e3d2182c6c8111050770d69821.1522769126.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <69bcb5b2-34b5-5c64-31b3-016d9a699348@tarantool.org> Date: Tue, 3 Apr 2018 21:27:39 +0300 MIME-Version: 1.0 In-Reply-To: <2bccf40f38d3e3e3d2182c6c8111050770d69821.1522769126.git.korablev@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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, Nikita Pettik 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. > /* > * 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. > @@ -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. > 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. > + 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. > 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.