On 3 Apr 2018, at 21:27, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> 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.
         */