On 22 Mar 2018, at 16:57, v.shpilevoy@tarantool.org wrote:

See below 4 comments.

21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org> написал(а):

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).

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?

+ /*
+  * 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.

@@ -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?