[tarantool-patches] Re: [PATCH 2/2] sql: rework 'DROP INDEX' and 'DROP TABLE' handling

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx, Nikita Pettik <korablev@xxxxxxxxxxxxx>
  • Date: Tue, 3 Apr 2018 21:27:39 +0300

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.



Other related posts: