[tarantool-patches] [PATCH v2 0/4] sql: get rid off sqlite3NestedParse

[tarantool-patches] [PATCH v2 0/4] sql: get rid off sqlite3NestedParse

[Kirill Shcherbatov]

As we are going to implement parser as separate library,
we should get rid off sqlite3NestedParse calls.   
Last usages in statistics update could be trivially rewrited
to do not start parsing. We manually build AST structures 
simmilar to parser did and manually call sql_table_delete_from. 
Finilly, I've removed all sqlite3NestedParse mentionings.
Also slightly refactored OP_Open{Write,Read} opcodes and 
vdbe_emit_open_cursor function to use index_id instead of tnum.

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3496-no-nested-parse
Issue: https://github.com/tarantool/tarantool/issues/3496

Kirill Shcherbatov (3):
  sql: get rid off sqlite3NestedParse in clean stats
  sql: remove usless sqlite3NestedParse function
  sql: refactor vdbe_emit_open_cursor calls

Vladislav Shpilevoy (1):
  sql: remove OP_LoadPtr

 src/box/sql/analyze.c   | 110 +++++++++++---------------
 src/box/sql/build.c     | 202 ++++++++++++++++++------------------------------
 src/box/sql/delete.c    |  32 ++------
 src/box/sql/expr.c      |   5 +-
 src/box/sql/fkey.c      |   3 +-
 src/box/sql/insert.c    | 120 ++++++++++++++--------------
 src/box/sql/parse.y     |  12 +--
 src/box/sql/pragma.c    |  36 +++++----
 src/box/sql/select.c    |   4 +-
 src/box/sql/sqliteInt.h |  39 +++++-----
 src/box/sql/tokenize.c  |   2 +-
 src/box/sql/trigger.c   |  45 ++++-------
 src/box/sql/update.c    |  15 ++--
 src/box/sql/vdbe.c      |  73 +++--------------
 src/box/sql/where.c     |  10 ++-
 15 files changed, 272 insertions(+), 436 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v2 1/4] sql: get rid off sqlite3NestedParse in clean stats

[Kirill Shcherbatov]

Now we manually generate AST structures to drop outdated
stats from _sql_stat1 and _sql_stat4 spaces instead of
starting sqlite3NestedParse. This function become totally
useless and could be removed.

Part of #3496.
---
 src/box/sql/analyze.c   | 102 ++++++++++++++++++++----------------------------
 src/box/sql/build.c     |  79 ++++++++++++++++++++++++++-----------
 src/box/sql/sqliteInt.h |  13 ++++++
 3 files changed, 113 insertions(+), 81 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 5f73f02..336d146 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -116,71 +116,53 @@
 #include "tarantoolInt.h"
 #include "vdbeInt.h"
 
-/*
- * This routine generates code that opens the sql_statN tables.
- * The _sql_stat1 table is always relevant. _sql_stat4 is only opened when
- * appropriate compile-time options are provided.
- *
- * If the sql_statN tables do not previously exist, it is created.
+/**
+ * This routine generates code that opens the sql_stat1/4 tables.
+ * If the sql_statN tables do not previously exist, they are
+ * created.
  *
- * Argument zWhere may be a pointer to a buffer containing a table name,
- * or it may be a NULL pointer. If it is not NULL, then all entries in
- * the sql_statN tables associated with the named table are deleted.
- * If zWhere==0, then code is generated to delete all stat table entries.
+ * @param parse Parsing context.
+ * @param stat_cursor Open the _sql_stat1 table on this cursor.
+ * @param index_name Delete records of this table if specified.
+ * @param table_name Delete records of this index if specified.
  */
 static void
-openStatTable(Parse * pParse,	/* Parsing context */
-	      int iStatCur,	/* Open the _sql_stat1 table on this cursor */
-	      const char *zWhere,	/* Delete entries for this table or index */
-	      const char *zWhereType	/* Either "tbl" or "idx" */
-    )
+vdbe_emit_stat_space_open(struct Parse *parse, int stat_cursor,
+			  const char *index_name, const char *table_name)
 {
-	const char *aTable[] = {
-		"_sql_stat1",
-		"_sql_stat4",
-		NULL};
-	int i;
-	sqlite3 *db = pParse->db;
-	Vdbe *v = sqlite3GetVdbe(pParse);
-	int aRoot[ArraySize(aTable)];
-	u8 aCreateTbl[ArraySize(aTable)];
+	const char *space_names[] = {"_sql_stat1", "_sql_stat4"};
+	const uint32_t space_ids[] = {BOX_SQL_STAT1_ID, BOX_SQL_STAT4_ID};
+	struct Vdbe *v = sqlite3GetVdbe(parse);
+	assert(v != NULL);
+	assert(sqlite3VdbeDb(v) == parse->db);
 
-	if (v == 0)
-		return;
-	assert(sqlite3VdbeDb(v) == db);
-
-	/* Create new statistic tables if they do not exist, or clear them
-	 * if they do already exist.
+	/*
+	 * Create new statistic tables if they do not exist, or
+	 * clear them if they do already exist.
 	 */
-	for (i = 0; aTable[i]; i++) {
-		const char *zTab = aTable[i];
-		Table *pStat;
-		/* The table already exists, because it is a system space */
-		pStat = sqlite3HashFind(&db->pSchema->tblHash, zTab);
-		assert(pStat != NULL);
-		aRoot[i] = pStat->tnum;
-		aCreateTbl[i] = 0;
-		if (zWhere) {
-			sqlite3NestedParse(pParse,
-					   "DELETE FROM \"%s\" WHERE \"%s\"=%Q",
-					   zTab, zWhereType, zWhere);
+	for (uint i = 0; i < lengthof(space_names); ++i) {
+		const char *space_name = space_names[i];
+		/*
+		 * The table already exists, because it is a
+		 * system space.
+		 */
+		assert(sqlite3HashFind(&parse->db->pSchema->tblHash,
+				       space_name) != NULL);
+		if (table_name != NULL || index_name != NULL) {
+			vdbe_emit_stat_space_clear(parse, space_name,
+						   index_name, table_name);
 		} else {
-			/*
-			 * The sql_stat[134] table already exists.
-			 * Delete all rows.
-			 */
-			sqlite3VdbeAddOp2(v, OP_Clear,
-					  SQLITE_PAGENO_TO_SPACEID(aRoot[i]), 0);
+			sqlite3VdbeAddOp2(v, OP_Clear, space_ids[i], 0);
 		}
 	}
 
-	/* Open the sql_stat[134] tables for writing. */
-	for (i = 0; aTable[i]; i++) {
-		struct space *space =
-			space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i]));
-		vdbe_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);
-		sqlite3VdbeChangeP5(v, aCreateTbl[i]);
-		VdbeComment((v, aTable[i]));
+	/* Open the sql_stat tables for writing. */
+	for (uint i = 0; i < lengthof(space_names); ++i) {
+		uint32_t id = space_ids[i];
+		int tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(id, 0);
+		vdbe_emit_open_cursor(parse, stat_cursor + i, tnum,
+				      space_by_id(id));
+		VdbeComment((v, space_names[i]));
 	}
 }
 
@@ -1117,7 +1099,7 @@ sql_analyze_database(Parse *parser)
 	sql_set_multi_write(parser, false);
 	int stat_cursor = parser->nTab;
 	parser->nTab += 3;
-	openStatTable(parser, stat_cursor, NULL, NULL);
+	vdbe_emit_stat_space_open(parser, stat_cursor, NULL, NULL);
 	int reg = parser->nMem + 1;
 	int tab_cursor = parser->nTab;
 	for (struct HashElem *k = sqliteHashFirst(&schema->tblHash); k != NULL;
@@ -1145,10 +1127,12 @@ analyzeTable(Parse * pParse, Table * pTab, Index * pOnlyIdx)
 	sql_set_multi_write(pParse, false);
 	iStatCur = pParse->nTab;
 	pParse->nTab += 3;
-	if (pOnlyIdx) {
-		openStatTable(pParse, iStatCur, pOnlyIdx->zName, "idx");
+	if (pOnlyIdx != NULL) {
+		vdbe_emit_stat_space_open(pParse, iStatCur, pOnlyIdx->zName,
+					  NULL);
 	} else {
-		openStatTable(pParse, iStatCur, pTab->def->name, "tbl");
+		vdbe_emit_stat_space_open(pParse, iStatCur, NULL,
+					  pTab->def->name);
 	}
 	analyzeOneTable(pParse, pTab, pOnlyIdx, iStatCur, pParse->nMem + 1,
 			pParse->nTab);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 0072f84..21791a4 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2050,6 +2050,60 @@ sql_store_select(struct Parse *parse_context, struct Select *select)
 }
 
 /**
+ * Create expression record "@col_name = '@col_value'".
+ *
+ * @param parse The parsing context.
+ * @param col_name Name of column.
+ * @param col_value Name of row.
+ * @retval not NULL on success.
+ * @retval NULL on failure.
+ */
+static struct Expr *
+sql_id_eq_str_expr(struct Parse *parse, const char *col_name,
+		   const char *col_value)
+{
+	struct sqlite3 *db = parse->db;
+
+	struct Expr *col_name_expr = sqlite3Expr(db, TK_ID, col_name);
+	if (col_name_expr == NULL)
+		return NULL;
+	struct Expr *col_value_expr = sqlite3Expr(db, TK_STRING, col_value);
+	if (col_value_expr == NULL) {
+		sql_expr_delete(db, col_name_expr, false);
+		return NULL;
+	}
+	return sqlite3PExpr(parse, TK_EQ, col_name_expr, col_value_expr);
+}
+
+void
+vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
+			   const char *idx_name, const char *table_name)
+{
+	assert(idx_name != NULL || table_name != NULL);
+	struct sqlite3 *db = parse->db;
+	assert(!db->mallocFailed);
+	struct SrcList *src_list = sql_alloc_src_list(db);
+	if (src_list != NULL)
+		src_list->a[0].zName = sqlite3DbStrDup(db, stat_table_name);
+	struct Expr *where = NULL;
+	if (idx_name != NULL) {
+		struct Expr *expr = sql_id_eq_str_expr(parse, "idx", idx_name);
+		if (expr != NULL)
+			where = sqlite3ExprAnd(db, expr, where);
+	}
+	if (table_name != NULL) {
+		struct Expr *expr = sql_id_eq_str_expr(parse, "tbl", table_name);
+		if (expr != NULL)
+			where = sqlite3ExprAnd(db, expr, where);
+	}
+	/**
+	 * On memory allocation error sql_table delete_from
+	 * releases memory for its own.
+	 */
+	sql_table_delete_from(parse, src_list, where);
+}
+
+/**
  * Remove entries from the _sql_stat1 and _sql_stat4
  * system spaces after a DROP INDEX or DROP TABLE command.
  *
@@ -2059,30 +2113,11 @@ sql_store_select(struct Parse *parse_context, struct Select *select)
  * @param idx_name   Index to be dropped.
  */
 static void
-sql_clear_stat_spaces(Parse *parse, const char *table_name,
+sql_clear_stat_spaces(struct Parse *parse, const char *table_name,
 		      const char *idx_name)
 {
-	if (idx_name != NULL) {
-		sqlite3NestedParse(parse,
-				   "DELETE FROM \"_sql_stat1\" "
-				   "WHERE (\"idx\"=%Q AND "
-				   "\"tbl\"=%Q)",
-				   idx_name, table_name);
-		sqlite3NestedParse(parse,
-				   "DELETE FROM \"_sql_stat4\" "
-				   "WHERE (\"idx\"=%Q AND "
-				   "\"tbl\"=%Q)",
-				   idx_name, table_name);
-	} else {
-		sqlite3NestedParse(parse,
-				   "DELETE FROM \"_sql_stat1\" "
-				   "WHERE \"tbl\"=%Q",
-				   table_name);
-		sqlite3NestedParse(parse,
-				   "DELETE FROM \"_sql_stat4\" "
-				   "WHERE \"tbl\"=%Q",
-				   table_name);
-	}
+	vdbe_emit_stat_space_clear(parse, "_sql_stat4", idx_name, table_name);
+	vdbe_emit_stat_space_clear(parse, "_sql_stat1", idx_name, table_name);
 }
 
 /**
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 8b75ae8..d76d173 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4847,4 +4847,17 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
 				  const char *error_src, bool no_error,
 				  int cond_opcode);
 
+/**
+ * Generate VDBE code to delete records from system _sql_stat1 or
+ * _sql_stat4 table.
+ *
+ * @param parse The parsing context.
+ * @param stat_table_name System stat table name.
+ * @param idx_name Index name.
+ * @param table_name Table name.
+ */
+void
+vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
+			   const char *idx_name, const char *table_name);
+
 #endif				/* SQLITEINT_H */
-- 
2.7.4

[tarantool-patches] [PATCH v2 2/4] sql: remove usless sqlite3NestedParse function

[Kirill Shcherbatov]

As sqlite3NestedParse become totaly useless, let's
remove unreacheble code and all mentioning.

Resolves #3496.
---
 src/box/sql/build.c     | 100 +++++-------------------------------------------
 src/box/sql/delete.c    |  13 ++-----
 src/box/sql/insert.c    |  18 +++------
 src/box/sql/parse.y     |  12 +-----
 src/box/sql/sqliteInt.h |  15 ++------
 src/box/sql/tokenize.c  |   2 +-
 src/box/sql/trigger.c   |  45 +++++++---------------
 src/box/sql/update.c    |  15 +++-----
 8 files changed, 45 insertions(+), 175 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 21791a4..1c00842 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -57,8 +57,6 @@
 void
 sql_finish_coding(struct Parse *parse_context)
 {
-	if (parse_context->nested)
-		return;
 	assert(parse_context->pToplevel == NULL);
 	struct sqlite3 *db = parse_context->db;
 	struct Vdbe *v = sqlite3GetVdbe(parse_context);
@@ -116,46 +114,6 @@ sql_finish_coding(struct Parse *parse_context)
 	}
 }
 
-/*
- * Run the parser and code generator recursively in order to generate
- * code for the SQL statement given onto the end of the pParse context
- * currently under construction.  When the parser is run recursively
- * this way, the final OP_Halt is not appended and other initialization
- * and finalization steps are omitted because those are handling by the
- * outermost parser.
- *
- * Not everything is nestable.  This facility is designed to perform
- * basic DDL operations.  Use care if you decide to try to use this routine
- * for some other purposes.
- */
-void
-sqlite3NestedParse(Parse * pParse, const char *zFormat, ...)
-{
-	va_list ap;
-	char *zSql;
-	char *zErrMsg = 0;
-	sqlite3 *db = pParse->db;
-	char saveBuf[PARSE_TAIL_SZ];
-
-	if (pParse->nErr)
-		return;
-	assert(pParse->nested < 10);	/* Nesting should only be of limited depth */
-	va_start(ap, zFormat);
-	zSql = sqlite3VMPrintf(db, zFormat, ap);
-	va_end(ap);
-	if (zSql == 0) {
-		return;		/* A malloc must have failed */
-	}
-	pParse->nested++;
-	memcpy(saveBuf, PARSE_TAIL(pParse), PARSE_TAIL_SZ);
-	memset(PARSE_TAIL(pParse), 0, PARSE_TAIL_SZ);
-	sqlite3RunParser(pParse, zSql, &zErrMsg);
-	sqlite3DbFree(db, zErrMsg);
-	sqlite3DbFree(db, zSql);
-	memcpy(PARSE_TAIL(pParse), saveBuf, PARSE_TAIL_SZ);
-	pParse->nested--;
-}
-
 /**
  * This is a function which should be called during execution
  * of sqlite3EndTable. It ensures that only PRIMARY KEY
@@ -532,17 +490,10 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
 	Table *pTable;
 	char *zName = 0;	/* The name of the new table */
 	sqlite3 *db = pParse->db;
-	Vdbe *v;
-
-	/* Do not account nested operations: the count of such
-	 * operations depends on Tarantool data dictionary internals,
-	 * such as data layout in system spaces.
-	 */
-	if (!pParse->nested) {
-		if ((v = sqlite3GetVdbe(pParse)) == NULL)
-			goto cleanup;
-		sqlite3VdbeCountChanges(v);
-	}
+	struct Vdbe *v = sqlite3GetVdbe(pParse);
+	if (v == NULL)
+		goto cleanup;
+	sqlite3VdbeCountChanges(v);
 
 	zName = sqlite3NameFromToken(db, pName);
 
@@ -1496,13 +1447,7 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
 			  SQL_SUBTYPE_MSGPACK,zParts, P4_STATIC);
 	sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 6, iRecord);
 	sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, iRecord);
-	/* Do not account nested operations: the count of such
-	 * operations depends on Tarantool data dictionary internals,
-	 * such as data layout in system spaces. Also do not account
-	 * autoindexes - they had been accounted as a part of
-	 * CREATE TABLE already.
-	 */
-	if (!pParse->nested && pIndex->idxType == SQLITE_IDXTYPE_APPDEF)
+	if (pIndex->idxType == SQLITE_IDXTYPE_APPDEF)
 		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
 }
 
@@ -1601,12 +1546,7 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
 			  SQL_SUBTYPE_MSGPACK, zFormat, P4_STATIC);
 	sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
 	sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);
-	/* Do not account nested operations: the count of such
-	 * operations depends on Tarantool data dictionary internals,
-	 * such as data layout in system spaces.
-	 */
-	if (!pParse->nested)
-		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+	sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
 }
 
 /*
@@ -2144,13 +2084,11 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	 * Do not account triggers deletion - they will be
 	 * accounted in DELETE from _space below.
 	 */
-	parse_context->nested++;
 	struct sql_trigger *trigger = space->sql_triggers;
 	while (trigger != NULL) {
-		vdbe_code_drop_trigger(parse_context, trigger->zName);
+		vdbe_code_drop_trigger(parse_context, trigger->zName, false);
 		trigger = trigger->next;
 	}
-	parse_context->nested--;
 	/*
 	 * Remove any entries of the _sequence and _space_sequence
 	 * space associated with the table being dropped. This is
@@ -2238,13 +2176,7 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
 	if (v == NULL || db->mallocFailed) {
 		goto exit_drop_table;
 	}
-	/*
-	 * Activate changes counting here to account
-	 * DROP TABLE IF NOT EXISTS, if the table really does not
-	 * exist.
-	 */
-	if (!parse_context->nested)
-		sqlite3VdbeCountChanges(v);
+	sqlite3VdbeCountChanges(v);
 	assert(parse_context->nErr == 0);
 	assert(table_name_list->nSrc == 1);
 	assert(is_view == 0 || is_view == LOCATE_VIEW);
@@ -2705,13 +2637,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	if (db->mallocFailed || parse->nErr > 0) {
 		goto exit_create_index;
 	}
-	/* Do not account nested operations: the count of such
-	 * operations depends on Tarantool data dictionary internals,
-	 * such as data layout in system spaces. Also do not account
-	 * PRIMARY KEY and UNIQUE constraint - they had been accounted
-	 * in CREATE TABLE already.
-	 */
-	if (!parse->nested && idx_type == SQLITE_IDXTYPE_APPDEF) {
+	if (idx_type == SQLITE_IDXTYPE_APPDEF) {
 		Vdbe *v = sqlite3GetVdbe(parse);
 		if (v == NULL)
 			goto exit_create_index;
@@ -3158,13 +3084,7 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
 	if (db->mallocFailed) {
 		goto exit_drop_index;
 	}
-	/*
-	 * Do not account nested operations: the count of such
-	 * operations depends on Tarantool data dictionary internals,
-	 * such as data layout in system spaces.
-	 */
-	if (!parse_context->nested)
-		sqlite3VdbeCountChanges(v);
+	sqlite3VdbeCountChanges(v);
 	assert(index_name_list->nSrc == 1);
 	assert(table_token->n > 0);
 	uint32_t space_id = box_space_id_by_name(table_name,
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 5a79971..66dc0fc 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -156,8 +156,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 	if (v == NULL)
 		goto delete_from_cleanup;
 
-	if (parse->nested == 0)
-		sqlite3VdbeCountChanges(v);
+	sqlite3VdbeCountChanges(v);
 	sql_set_multi_write(parse, true);
 
 	/* If we are trying to delete from a view, realize that
@@ -387,7 +386,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			idx_noseek = one_pass_cur[1];
 
 		sql_generate_row_delete(parse, table, trigger_list, tab_cursor,
-					reg_key, key_len, parse->nested == 0,
+					reg_key, key_len, true,
 					ON_CONFLICT_ACTION_DEFAULT, one_pass,
 					idx_noseek);
 
@@ -403,13 +402,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		}
 	}
 
-	/* Return the number of rows that were deleted. If this
-	 * routine is generating code because of a call to
-	 * sqlite3NestedParse(), do not invoke the callback
-	 * function.
-	 */
+	/* Return the number of rows that were deleted. */
 	if ((user_session->sql_flags & SQLITE_CountRows) != 0 &&
-	    parse->nested == 0 && parse->pTriggerTab != NULL) {
+	    parse->pTriggerTab != NULL) {
 		sqlite3VdbeAddOp2(v, OP_ResultRow, reg_count, 1);
 		sqlite3VdbeSetNumCols(v, 1);
 		sqlite3VdbeSetColName(v, 0, COLNAME_NAME, "rows deleted",
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index c12043b..58e159c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -413,13 +413,11 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		goto insert_cleanup;
 	}
 
-	/* Allocate a VDBE
-	 */
+	/* Allocate a VDBE. */
 	v = sqlite3GetVdbe(pParse);
-	if (v == 0)
+	if (v == NULL)
 		goto insert_cleanup;
-	if (pParse->nested == 0)
-		sqlite3VdbeCountChanges(v);
+	sqlite3VdbeCountChanges(v);
 	sql_set_multi_write(pParse, pSelect != NULL || trigger != NULL);
 
 #ifndef SQLITE_OMIT_XFER_OPT
@@ -904,13 +902,9 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 
  insert_end:
 
-	/*
-	 * Return the number of rows inserted. If this routine is
-	 * generating code because of a call to sqlite3NestedParse(), do not
-	 * invoke the callback function.
-	 */
-	if ((user_session->sql_flags & SQLITE_CountRows) && !pParse->nested
-	    && !pParse->pTriggerTab) {
+	/* Return the number of rows inserted. */
+	if ((user_session->sql_flags & SQLITE_CountRows) != 0 &&
+	    pParse->pTriggerTab == NULL) {
 		sqlite3VdbeAddOp2(v, OP_ResultRow, regRowCount, 1);
 		sqlite3VdbeSetNumCols(v, 1);
 		sqlite3VdbeSetColName(v, 0, COLNAME_NAME, "rows inserted",
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index b2940b7..ac935fd 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -877,18 +877,10 @@ expr(A) ::= VARIABLE(X).     {
     else
         sqlite3ExprAssignVarNumber(pParse, A.pExpr, n);
   }else{
-    /* When doing a nested parse, one can include terms in an expression
-    ** that look like this:   #1 #2 ...  These terms refer to registers
-    ** in the virtual machine.  #N is the N-th register. */
     assert( t.n>=2 );
     spanSet(&A, &t, &t);
-    if( pParse->nested==0 ){
-      sqlite3ErrorMsg(pParse, "near \"%T\": syntax error", &t);
-      A.pExpr = 0;
-    }else{
-      A.pExpr = sqlite3PExpr(pParse, TK_REGISTER, 0, 0);
-      if( A.pExpr ) sqlite3GetInt32(&t.z[1], &A.pExpr->iTable);
-    }
+    sqlite3ErrorMsg(pParse, "near \"%T\": syntax error", &t);
+    A.pExpr = NULL;
   }
 }
 expr(A) ::= expr(A) COLLATE id(C). {
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index d76d173..380c9c6 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2880,7 +2880,6 @@ struct Parse {
 	int rc;			/* Return code from execution */
 	u8 colNamesSet;		/* TRUE after OP_ColumnName has been issued to pVdbe */
 	u8 checkSchema;		/* Causes schema cookie check after an error */
-	u8 nested;		/* Number of nested calls to the parser/code generator */
 	u8 nTempReg;		/* Number of temporary registers in aTempReg[] */
 	u8 isMultiWrite;	/* True if statement may modify/insert multiple rows */
 	u8 mayAbort;		/* True if statement may throw an ABORT exception */
@@ -2976,14 +2975,6 @@ struct Parse {
 };
 
 /*
- * Sizes and pointers of various parts of the Parse object.
- */
-#define PARSE_HDR_SZ offsetof(Parse,aColCache)	/* Recursive part w/o aColCache */
-#define PARSE_RECURSE_SZ offsetof(Parse,sLastToken)	/* Recursive part */
-#define PARSE_TAIL_SZ (sizeof(Parse)-PARSE_RECURSE_SZ)	/* Non-recursive part */
-#define PARSE_TAIL(X) (((char*)(X))+PARSE_RECURSE_SZ)	/* Pointer to tail */
-
-/*
  * Bitfield flags for P5 value in various opcodes.
  *
  * Value constraints (enforced via assert()):
@@ -4129,9 +4120,12 @@ sql_drop_trigger(struct Parse *parser, struct SrcList *name, bool no_err);
  *
  * @param parser Parser context.
  * @param trigger_name The name of trigger to drop.
+ * @param account_changes Increase number of db changes made since
+ *        last reset.
  */
 void
-vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name);
+vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name,
+		       bool account_changes);
 
 /**
  * Return a list of all triggers on table pTab if there exists at
@@ -4449,7 +4443,6 @@ void sqlite3AlterRenameTable(Parse *, SrcList *, Token *);
 int
 sql_token(const char *z, int *type, bool *is_reserved);
 
-void sqlite3NestedParse(Parse *, const char *, ...);
 void sqlite3ExpirePreparedStatements(sqlite3 *);
 int sqlite3CodeSubselect(Parse *, Expr *, int);
 void sqlite3SelectPrep(Parse *, Select *, NameContext *);
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 7c3dabe..ce9ed84 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -526,7 +526,7 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
 		pParse->zErrMsg = 0;
 		nErr++;
 	}
-	if (pParse->pVdbe && pParse->nErr > 0 && pParse->nested == 0) {
+	if (pParse->pVdbe != NULL && pParse->nErr > 0) {
 		sqlite3VdbeDelete(pParse->pVdbe);
 		pParse->pVdbe = 0;
 	}
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 801013b..ec0bc98 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -75,16 +75,10 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
 	/* The name of the Trigger. */
 	char *trigger_name = NULL;
 
-	/*
-	 * Do not account nested operations: the count of such
-	 * operations depends on Tarantool data dictionary
-	 * internals, such as data layout in system spaces.
-	 */
-	if (!parse->nested) {
-		struct Vdbe *v = sqlite3GetVdbe(parse);
-		if (v != NULL)
-			sqlite3VdbeCountChanges(v);
-	}
+	struct Vdbe *v = sqlite3GetVdbe(parse);
+	if (v != NULL)
+		sqlite3VdbeCountChanges(v);
+
 	/* pName->z might be NULL, but not pName itself. */
 	assert(name != NULL);
 	assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE);
@@ -261,14 +255,8 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
 				  SQL_SUBTYPE_MSGPACK, opts_buff, P4_DYNAMIC);
 		sqlite3VdbeAddOp3(v, OP_MakeRecord, first_col, 3, record);
 		sqlite3VdbeAddOp2(v, OP_IdxInsert, cursor, record);
-		/*
-		 * Do not account nested operations: the count of
-		 * such operations depends on Tarantool data
-		 * dictionary internals, such as data layout in
-		 * system spaces.
-		 */
-		if (!parse->nested)
-			sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+
+		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
 		sqlite3VdbeAddOp1(v, OP_Close, cursor);
 
 		sql_set_multi_write(parse, false);
@@ -440,7 +428,8 @@ sql_trigger_delete(struct sqlite3 *db, struct sql_trigger *trigger)
 }
 
 void
-vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name)
+vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name,
+		       bool account_changes)
 {
 	sqlite3 *db = parser->db;
 	assert(db->pSchema != NULL);
@@ -459,7 +448,7 @@ vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name)
 			  record_to_delete);
 	sqlite3VdbeAddOp2(v, OP_SDelete, BOX_TRIGGER_ID,
 			  record_to_delete);
-	if (parser->nested == 0)
+	if (account_changes)
 		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
 	sqlite3ChangeCookie(parser);
 }
@@ -473,17 +462,9 @@ sql_drop_trigger(struct Parse *parser, struct SrcList *name, bool no_err)
 		goto drop_trigger_cleanup;
 	assert(db->pSchema != NULL);
 
-	/* Do not account nested operations: the count of such
-	 * operations depends on Tarantool data dictionary internals,
-	 * such as data layout in system spaces. Activate the counter
-	 * here to account DROP TRIGGER IF EXISTS case if the trigger
-	 * actually does not exist.
-	 */
-	if (!parser->nested) {
-		Vdbe *v = sqlite3GetVdbe(parser);
-		if (v != NULL)
-			sqlite3VdbeCountChanges(v);
-	}
+	struct Vdbe *v = sqlite3GetVdbe(parser);
+	if (v != NULL)
+		sqlite3VdbeCountChanges(v);
 
 	assert(name->nSrc == 1);
 	const char *trigger_name = name->a[0].zName;
@@ -495,7 +476,7 @@ sql_drop_trigger(struct Parse *parser, struct SrcList *name, bool no_err)
 					      error_msg, no_err, OP_Found) != 0)
 		goto drop_trigger_cleanup;
 
-	vdbe_code_drop_trigger(parser, trigger_name);
+	vdbe_code_drop_trigger(parser, trigger_name, true);
 
  drop_trigger_cleanup:
 	sqlite3SrcListDelete(db, name);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 212adbc..cbcca34 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -261,10 +261,9 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 
 	/* Begin generating code. */
 	v = sqlite3GetVdbe(pParse);
-	if (v == 0)
+	if (v == NULL)
 		goto update_cleanup;
-	if (pParse->nested == 0)
-		sqlite3VdbeCountChanges(v);
+	sqlite3VdbeCountChanges(v);
 	sql_set_multi_write(pParse, true);
 
 	/* Allocate required registers. */
@@ -625,13 +624,9 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	}
 	sqlite3VdbeResolveLabel(v, labelBreak);
 
-	/*
-	 * Return the number of rows that were changed. If this routine is
-	 * generating code because of a call to sqlite3NestedParse(), do not
-	 * invoke the callback function.
-	 */
-	if ((user_session->sql_flags & SQLITE_CountRows) &&
-	    !pParse->pTriggerTab && !pParse->nested) {
+	/* Return the number of rows that were changed. */
+	if (user_session->sql_flags & SQLITE_CountRows &&
+	    pParse->pTriggerTab == NULL) {
 		sqlite3VdbeAddOp2(v, OP_ResultRow, regRowCount, 1);
 		sqlite3VdbeSetNumCols(v, 1);
 		sqlite3VdbeSetColName(v, 0, COLNAME_NAME, "rows updated",
-- 
2.7.4

[tarantool-patches] [PATCH v2 3/4] sql: refactor vdbe_emit_open_cursor calls

[Kirill Shcherbatov]

Made vdbe_emit_open_cursor calls consistent:
now it uses index id everywhere.
This required to change a way to detect that
VDBE has openned Read cursor to specified table
in vdbe_has_table_read to write result of insert
in temp table if required.
---
 src/box/sql/analyze.c   |   6 +--
 src/box/sql/build.c     |   3 +-
 src/box/sql/delete.c    |   8 +---
 src/box/sql/expr.c      |   5 ++-
 src/box/sql/fkey.c      |   3 +-
 src/box/sql/insert.c    | 100 +++++++++++++++++++++++++++++-------------------
 src/box/sql/pragma.c    |   7 +++-
 src/box/sql/select.c    |   4 +-
 src/box/sql/sqliteInt.h |   6 +--
 src/box/sql/vdbe.c      |   5 +--
 src/box/sql/where.c     |  10 ++++-
 11 files changed, 92 insertions(+), 65 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 336d146..067a86e 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -159,8 +159,7 @@ vdbe_emit_stat_space_open(struct Parse *parse, int stat_cursor,
 	/* Open the sql_stat tables for writing. */
 	for (uint i = 0; i < lengthof(space_names); ++i) {
 		uint32_t id = space_ids[i];
-		int tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(id, 0);
-		vdbe_emit_open_cursor(parse, stat_cursor + i, tnum,
+		vdbe_emit_open_cursor(parse, stat_cursor + i, 0,
 				      space_by_id(id));
 		VdbeComment((v, space_names[i]));
 	}
@@ -893,10 +892,11 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
 		/* Open a read-only cursor on the index being analyzed. */
 		struct space *space =
 			space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+		int idx_id = SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
 		assert(space != NULL);
 		sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
 				  (void*)space, P4_SPACEPTR);
-		sqlite3VdbeAddOp3(v, OP_OpenRead, iIdxCur, pIdx->tnum,
+		sqlite3VdbeAddOp3(v, OP_OpenRead, iIdxCur, idx_id,
 				  space_ptr_reg);
 		sql_vdbe_set_p4_key_def(pParse, pIdx);
 		VdbeComment((v, "%s", pIdx->zName));
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 1c00842..5f7a35a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2448,7 +2448,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
 		sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum),
 				  0);
 	struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
-	vdbe_emit_open_cursor(pParse, iIdx, tnum, space);
+	vdbe_emit_open_cursor(pParse, iIdx, SQLITE_PAGENO_TO_INDEXID(tnum),
+			      space);
 	sqlite3VdbeChangeP5(v, memRootPage >= 0 ? OPFLAG_P2ISREG : 0);
 
 	addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0);
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 66dc0fc..d1c5935 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -340,18 +340,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
 					  (void *)space, P4_SPACEPTR);
 
-			int tnum =
-				SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space_id,
-								       0);
-			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
-					  tnum, space_ptr_reg);
+			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor, 0,
+					  space_ptr_reg);
 			struct key_def *def = key_def_dup(pk_def);
 			if (def == NULL) {
 				sqlite3OomFault(parse->db);
 				goto delete_from_cleanup;
 			}
 			sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
-
 			VdbeComment((v, "%s", space->index[0]->def->name));
 
 			if (one_pass == ONEPASS_MULTI)
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3183e3d..b1650cf 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2470,8 +2470,11 @@ sqlite3FindInIndex(Parse * pParse,	/* Parsing context */
 							  P4_DYNAMIC);
 					struct space *space =
 						space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+					uint32_t idx_id =
+						SQLITE_PAGENO_TO_INDEXID(pIdx->
+									 tnum);
 					vdbe_emit_open_cursor(pParse, iTab,
-							      pIdx->tnum, space);
+							      idx_id, space);
 					VdbeComment((v, "%s", pIdx->zName));
 					assert(IN_INDEX_INDEX_DESC ==
 					       IN_INDEX_INDEX_ASC + 1);
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 6c75c47..face9cb 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -441,7 +441,8 @@ fkLookupParent(Parse * pParse,	/* Parse context */
 			int regRec = sqlite3GetTempReg(pParse);
 			struct space *space =
 				space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
-			vdbe_emit_open_cursor(pParse, iCur, pIdx->tnum, space);
+			uint32_t idx_id = SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
+			vdbe_emit_open_cursor(pParse, iCur, idx_id, space);
 			for (i = 0; i < nCol; i++) {
 				sqlite3VdbeAddOp2(v, OP_Copy,
 						  aiCol[i] + 1 + regData,
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 58e159c..62b85d5 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -179,41 +179,51 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg)
 	}
 }
 
-/*
- * Return non-zero if the table pTab in database or any of its indices
- * have been opened at any point in the VDBE program. This is used to see if
- * a statement of the form  "INSERT INTO <pTab> SELECT ..." can
- * run for the results of the SELECT.
+/**
+ * This routine is used to see if a statement of the form
+ * "INSERT INTO <table> SELECT ..." can run for the results of the
+ * SELECT.
+ *
+ * @param parser Parse context.
+ * @param table Table AST object.
+ * @retval  true if the table table in database or any of its
+ *          indices have been opened at any point in the VDBE
+ *          program.
+ * @retval  false else.
  */
-static int
-readsTable(Parse * p, Table * pTab)
+static bool
+vdbe_has_table_read(struct Parse *parser, const struct Table *table)
 {
-	Vdbe *v = sqlite3GetVdbe(p);
-	int i;
-	int iEnd = sqlite3VdbeCurrentAddr(v);
-
-	for (i = 1; i < iEnd; i++) {
-		VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
-		assert(pOp != 0);
-		/* Currently, there is no difference between
-		 * Read and Write cursors.
+	struct Vdbe *v = sqlite3GetVdbe(parser);
+	int last_instr = sqlite3VdbeCurrentAddr(v);
+	for (int i = 1; i < last_instr; i++) {
+		struct VdbeOp *op = sqlite3VdbeGetOp(v, i);
+		assert(op != NULL);
+		/*
+		 * Currently, there is no difference between Read
+		 * and Write cursors.
 		 */
-		if (pOp->opcode == OP_OpenRead ||
-		    pOp->opcode == OP_OpenWrite) {
-			Index *pIndex;
-			int tnum = pOp->p2;
-			if (tnum == pTab->tnum) {
-				return 1;
-			}
-			for (pIndex = pTab->pIndex; pIndex;
-			     pIndex = pIndex->pNext) {
-				if (tnum == pIndex->tnum) {
-					return 1;
-				}
+		if (op->opcode == OP_OpenRead || op->opcode == OP_OpenWrite) {
+			assert(i > 1);
+			struct VdbeOp *space_var_op =
+				sqlite3VdbeGetOp(v, i - 1);
+			assert(space_var_op != NULL);
+			assert(space_var_op->opcode == OP_LoadPtr);
+			struct space *space = space_var_op->p4.space;
+
+			if (space->def->id == table->def->id)
+				return true;
+
+			int idx_id = op->p2;
+			for (struct Index *pIndex = table->pIndex;
+				pIndex != NULL; pIndex = pIndex->pNext) {
+				if (idx_id ==
+				    SQLITE_PAGENO_TO_INDEXID(pIndex->tnum))
+					return true;
 			}
 		}
 	}
-	return 0;
+	return false;
 }
 
 
@@ -526,16 +536,20 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		assert(pSelect->pEList);
 		nColumn = pSelect->pEList->nExpr;
 
-		/* Set useTempTable to TRUE if the result of the SELECT statement
-		 * should be written into a temporary table (template 4).  Set to
-		 * FALSE if each output row of the SELECT can be written directly into
-		 * the destination table (template 3).
+		/*
+		 * Set useTempTable to TRUE if the result of the
+		 * SELECT statement should be written into a
+		 * temporary table (template 4). Set to FALSE if
+		 * each output row of the SELECT can be written
+		 * directly into the destination table
+		 * (template 3).
 		 *
-		 * A temp table must be used if the table being updated is also one
-		 * of the tables being read by the SELECT statement.  Also use a
-		 * temp table in the case of row triggers.
+		 * A temp table must be used if the table being
+		 * updated is also one of the tables being read by
+		 * the SELECT statement. Also use a temp table in
+		 * the case of row triggers.
 		 */
-		if (trigger != NULL || readsTable(pParse, pTab))
+		if (trigger != NULL || vdbe_has_table_read(pParse, pTab))
 			useTempTable = 1;
 
 		if (useTempTable) {
@@ -1617,7 +1631,9 @@ sqlite3OpenTableAndIndices(Parse * pParse,	/* Parsing context */
 				p5 = 0;
 			}
 			if (aToOpen == 0 || aToOpen[i + 1]) {
-				sqlite3VdbeAddOp3(v, op, iIdxCur, pIdx->tnum,
+				int idx_id =
+					SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
+				sqlite3VdbeAddOp3(v, op, iIdxCur, idx_id,
 						  space_ptr_reg);
 				sql_vdbe_set_p4_key_def(pParse, pIdx);
 				sqlite3VdbeChangeP5(v, p5);
@@ -1934,11 +1950,15 @@ xferOptimization(Parse * pParse,	/* Parser context */
 		assert(pSrcIdx);
 		struct space *src_space =
 			space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
-		vdbe_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
+		vdbe_emit_open_cursor(pParse, iSrc,
+				      SQLITE_PAGENO_TO_INDEXID(pSrcIdx->tnum),
+				      src_space);
 		VdbeComment((v, "%s", pSrcIdx->zName));
 		struct space *dest_space =
 			space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
-		vdbe_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
+		vdbe_emit_open_cursor(pParse, iDest,
+				      SQLITE_PAGENO_TO_INDEXID(pDestIdx->tnum),
+				      dest_space);
 		VdbeComment((v, "%s", pDestIdx->zName));
 		addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
 		VdbeCoverage(v);
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 31581b1..ca119e0 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -685,11 +685,14 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 									 pParent,
 									 OP_OpenRead);
 						} else {
+							int idx_id =
+								SQLITE_PAGENO_TO_INDEXID(
+									pIdx->
+									tnum);
 							sqlite3VdbeAddOp3(v,
 									  OP_OpenRead,
 									  i,
-									  pIdx->
-									  tnum,
+									  idx_id,
 									  0);
 							sql_vdbe_set_p4_key_def(pParse,
 										pIdx);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 52b3fdd..ceb7e34 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -6279,9 +6279,7 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 				 * Open the cursor, execute the OP_Count,
 				 * close the cursor.
 				 */
-				vdbe_emit_open_cursor(pParse, cursor,
-						      space->def->id << 10,
-						      space);
+				vdbe_emit_open_cursor(pParse, cursor, 0, space);
 				sqlite3VdbeAddOp2(v, OP_Count, cursor,
 						  sAggInfo.aFunc[0].iMem);
 				sqlite3VdbeAddOp1(v, OP_Close, cursor);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 380c9c6..4a1eef0 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3583,9 +3583,9 @@ void sqlite3EndTable(Parse *, Token *, Token *, Select *);
  *
  * @param parse_context Parse context.
  * @param cursor Number of cursor to be created.
- * @param index_id Encoded index id (encoding is void actually, so
- *        pas it as is). In future will be replaced with pointer
- *        to struct index.
+ * @param index_id index id. In future will be replaced with
+ *        pointer to struct index.
+ * @param space Pointer to space object.
  * @retval address of last opcode.
  */
 int
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7a4d376..a0eb41b 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3185,9 +3185,8 @@ case OP_ReopenIdx: {
 	pIn3 = &aMem[pOp->p3];
 	assert(pIn3->flags & MEM_Ptr);
 	if (pCur && pCur->uc.pCursor->space == (struct space *) pIn3->u.p &&
-	    pCur->uc.pCursor->index->def->iid == SQLITE_PAGENO_TO_INDEXID(p2)) {
+	    pCur->uc.pCursor->index->def->iid == (uint32_t)p2)
 		goto open_cursor_set_hints;
-	}
 	/* If the cursor is not currently open or is open on a different
 	 * index, then fall through into OP_OpenRead to force a reopen
 	 */
@@ -3213,7 +3212,7 @@ case OP_OpenWrite:
 	assert(pIn3->flags & MEM_Ptr);
 	struct space *space = ((struct space *) pIn3->u.p);
 	assert(space != NULL);
-	struct index *index = space_index(space, SQLITE_PAGENO_TO_INDEXID(p2));
+	struct index *index = space_index(space, p2);
 	assert(index != NULL);
 	/*
 	 * Since Tarantool iterator provides the full tuple,
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 85143ed..6c07fec 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -4759,10 +4759,16 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 			assert(iIndexCur >= 0);
 			if (op) {
 				if (pIx != NULL) {
+					uint32_t space_id =
+						SQLITE_PAGENO_TO_SPACEID(pIx->
+									 tnum);
 					struct space *space =
-						space_by_id(SQLITE_PAGENO_TO_SPACEID(pIx->tnum));
+						space_by_id(space_id);
+					uint32_t idx_id =
+						SQLITE_PAGENO_TO_INDEXID(pIx->
+									 tnum);
 					vdbe_emit_open_cursor(pParse, iIndexCur,
-							      pIx->tnum, space);
+							      idx_id, space);
 				} else {
 					vdbe_emit_open_cursor(pParse, iIndexCur,
 							      idx_def->iid,
-- 
2.7.4

[tarantool-patches] [PATCH v2 4/4] sql: remove OP_LoadPtr

[Kirill Shcherbatov]

From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

This opcode was used when Vdbe had to store key_def in P4 for
OP_OpenRead/Write, so P4 was occupied and OP_LoadPtr was used to
store space pointer in another opcode. But now P4 is free for
OpenRead/Write and space pointer can be stored here.

Alongside, some useless key_def duplications are removed.
---
 src/box/sql/analyze.c   |  8 ++----
 src/box/sql/build.c     | 20 +++++----------
 src/box/sql/delete.c    | 15 ++---------
 src/box/sql/insert.c    | 18 +++----------
 src/box/sql/pragma.c    | 39 ++++++++++++++--------------
 src/box/sql/sqliteInt.h |  5 ----
 src/box/sql/vdbe.c      | 68 +++++--------------------------------------------
 7 files changed, 40 insertions(+), 133 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 067a86e..699a4e8 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -794,7 +794,6 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
 	int iTabCur;		/* Table cursor */
 	Vdbe *v;		/* The virtual machine being built up */
 	int i;			/* Loop counter */
-	int space_ptr_reg = iMem++;
 	int regStat4 = iMem++;	/* Register to hold Stat4Accum object */
 	int regChng = iMem++;	/* Index of changed index field */
 	int regKey = iMem++;	/* Key argument passed to stat_push() */
@@ -894,11 +893,8 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
 			space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
 		int idx_id = SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
 		assert(space != NULL);
-		sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
-				  (void*)space, P4_SPACEPTR);
-		sqlite3VdbeAddOp3(v, OP_OpenRead, iIdxCur, idx_id,
-				  space_ptr_reg);
-		sql_vdbe_set_p4_key_def(pParse, pIdx);
+		sqlite3VdbeAddOp4(v, OP_OpenRead, iIdxCur, idx_id, 0,
+				  (void *) space, P4_SPACEPTR);
 		VdbeComment((v, "%s", pIdx->zName));
 
 		/* Invoke the stat_init() function. The arguments are:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5f7a35a..26af0fe 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1108,12 +1108,8 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
 		      struct space *space)
 {
 	assert(space != NULL);
-	struct Vdbe *vdbe = parse_context->pVdbe;
-	int space_ptr_reg = ++parse_context->nMem;
-	sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
-			  P4_SPACEPTR);
-	return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, index_id,
-				 space_ptr_reg);
+	return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_OpenWrite, cursor,
+				 index_id, 0, (void *) space, P4_SPACEPTR);
 }
 /*
  * Generate code that will increment the schema cookie.
@@ -2961,7 +2957,6 @@ sql_create_index(struct Parse *parse, struct Token *token,
 		Vdbe *v;
 		char *zStmt;
 		int iCursor = parse->nTab++;
-		int index_space_ptr_reg = parse->nTab++;
 		int iSpaceId, iIndexId, iFirstSchemaCol;
 
 		v = sqlite3GetVdbe(parse);
@@ -2969,12 +2964,9 @@ sql_create_index(struct Parse *parse, struct Token *token,
 			goto exit_create_index;
 
 		sql_set_multi_write(parse, true);
-
-
-		sqlite3VdbeAddOp2(v, OP_SIDtoPtr, BOX_INDEX_ID,
-				  index_space_ptr_reg);
-		sqlite3VdbeAddOp4Int(v, OP_OpenWrite, iCursor, 0,
-				     index_space_ptr_reg, 6);
+		sqlite3VdbeAddOp4(v, OP_OpenWrite, iCursor, 0, 0,
+				  (void *)space_by_id(BOX_INDEX_ID),
+				  P4_SPACEPTR);
 		sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ);
 
 		/*
@@ -3943,7 +3935,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
 	int cursor = parser->nTab++;
 	vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
 
-	int name_reg = parser->nMem++;
+	int name_reg = ++parser->nMem;
 	int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
 				      P4_DYNAMIC);
 	sqlite3VdbeAddOp4Int(v, cond_opcode, cursor, label + 3, name_reg, 1);
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index d1c5935..ca1e77d 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -335,19 +335,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 				iAddrOnce = sqlite3VdbeAddOp0(v, OP_Once);
 				VdbeCoverage(v);
 			}
-
-			int space_ptr_reg = ++parse->nMem;
-			sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
-					  (void *)space, P4_SPACEPTR);
-
-			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor, 0,
-					  space_ptr_reg);
-			struct key_def *def = key_def_dup(pk_def);
-			if (def == NULL) {
-				sqlite3OomFault(parse->db);
-				goto delete_from_cleanup;
-			}
-			sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
+			sqlite3VdbeAddOp4(v, OP_OpenWrite, tab_cursor, 0, 0,
+					  (void *) space, P4_SPACEPTR);
 			VdbeComment((v, "%s", space->index[0]->def->name));
 
 			if (one_pass == ONEPASS_MULTI)
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 62b85d5..6aede16 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -204,13 +204,8 @@ vdbe_has_table_read(struct Parse *parser, const struct Table *table)
 		 * and Write cursors.
 		 */
 		if (op->opcode == OP_OpenRead || op->opcode == OP_OpenWrite) {
-			assert(i > 1);
-			struct VdbeOp *space_var_op =
-				sqlite3VdbeGetOp(v, i - 1);
-			assert(space_var_op != NULL);
-			assert(space_var_op->opcode == OP_LoadPtr);
-			struct space *space = space_var_op->p4.space;
-
+			assert(op->p4type == P4_SPACEPTR);
+			struct space *space = op->p4.space;
 			if (space->def->id == table->def->id)
 				return true;
 
@@ -1581,10 +1576,6 @@ sqlite3OpenTableAndIndices(Parse * pParse,	/* Parsing context */
 		*piIdxCur = iBase;
 	struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pTab->tnum));
 	assert(space != NULL);
-	int space_ptr_reg = ++pParse->nMem;
-	sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
-			  P4_SPACEPTR);
-
 	/* One iteration of this cycle adds OpenRead/OpenWrite which
 	 * opens cursor for current index.
 	 *
@@ -1633,9 +1624,8 @@ sqlite3OpenTableAndIndices(Parse * pParse,	/* Parsing context */
 			if (aToOpen == 0 || aToOpen[i + 1]) {
 				int idx_id =
 					SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
-				sqlite3VdbeAddOp3(v, op, iIdxCur, idx_id,
-						  space_ptr_reg);
-				sql_vdbe_set_p4_key_def(pParse, pIdx);
+				sqlite3VdbeAddOp4(v, op, iIdxCur, idx_id, 0,
+						  (void *) space, P4_SPACEPTR);
 				sqlite3VdbeChangeP5(v, p5);
 				VdbeComment((v, "%s", pIdx->zName));
 			}
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index ca119e0..8ff5bbc 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -678,29 +678,28 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 					x = sqlite3FkLocateIndex(pParse,
 								 pParent, pFK,
 								 &pIdx, 0);
-					if (x == 0) {
-						if (pIdx == 0) {
-							sqlite3OpenTable(pParse,
-									 i,
-									 pParent,
-									 OP_OpenRead);
-						} else {
-							int idx_id =
-								SQLITE_PAGENO_TO_INDEXID(
-									pIdx->
-									tnum);
-							sqlite3VdbeAddOp3(v,
-									  OP_OpenRead,
-									  i,
-									  idx_id,
-									  0);
-							sql_vdbe_set_p4_key_def(pParse,
-										pIdx);
-						}
-					} else {
+					if (x != 0) {
 						k = 0;
 						break;
 					}
+					if (pIdx == NULL) {
+						sqlite3OpenTable(pParse, i,
+								 pParent,
+								 OP_OpenRead);
+						continue;
+					}
+					struct space *space =
+						space_cache_find(pIdx->pTable->
+								 def->id);
+					int idx_id =
+						SQLITE_PAGENO_TO_INDEXID(pIdx->
+									 tnum);
+					assert(space != NULL);
+					sqlite3VdbeAddOp4(v, OP_OpenRead, i,
+							  idx_id, 0,
+							  (void *) space,
+							  P4_SPACEPTR);
+
 				}
 				assert(pParse->nErr > 0 || pFK == 0);
 				if (pFK)
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 4a1eef0..18bf949 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3008,11 +3008,6 @@ struct Parse {
 #define OPFLAG_NOOP_IF_NULL  0x02	/* OP_FCopy: if source register is NULL
 					 * then do nothing
 					 */
-#define OPFLAG_FRESH_PTR     0x20	/* OP_Open**: set if space pointer
-					 * comes from OP_SIDtoPtr, i.e. it
-					 * is fresh, even in case schema
-					 * changes previously.
-					 */
 
 /*
  * Each trigger present in the database schema is stored as an
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a0eb41b..49a17b9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1077,19 +1077,6 @@ case OP_Int64: {           /* out2 */
 	break;
 }
 
-/* Opcode: LoadPtr * P2 * P4 *
- * Synopsis: r[P2] = P4
- *
- * P4 is a generic or space pointer. Copy it into register P2.
- */
-case OP_LoadPtr: {
-	pOut = out2Prerelease(p, pOp);
-	assert(pOp->p4type == P4_PTR || pOp->p4type == P4_SPACEPTR );
-	pOut->u.p = pOp->p4.space;
-	pOut->flags = MEM_Ptr;
-	break;
-}
-
 #ifndef SQLITE_OMIT_FLOATING_POINT
 /* Opcode: Real * P2 * P4 *
  * Synopsis: r[P2]=P4
@@ -3137,26 +3124,15 @@ case OP_SetCookie: {
 }
 
 /* Opcode: OpenRead P1 P2 P3 P4 P5
- * Synopsis: index id = P2, space ptr = P3
+ * Synopsis: index id = P2, space ptr = P4
  *
- * Open a cursor for a space specified by pointer in P3 and index
+ * Open a cursor for a space specified by pointer in P4 and index
  * id in P2. Give the new cursor an identifier of P1. The P1
  * values need not be contiguous but all P1 values should be
  * small integers. It is an error for P1 to be negative.
- *
- * The P4 value may be a pointer to a key_def structure.
- * If it is a pointer to a key_def structure, then said structure
- * defines the content and collatining sequence of the index
- * being opened. Otherwise, P4 is NULL.
- *
- * If schema has changed since compile time, VDBE ends execution
- * with appropriate error message. The only exception is
- * when P5 is set to OPFLAG_FRESH_PTR, which means that
- * space pointer has been fetched in runtime right before
- * this opcode.
  */
 /* Opcode: ReopenIdx P1 P2 P3 P4 P5
- * Synopsis: index id = P2, space ptr = P3
+ * Synopsis: index id = P2, space ptr = P4
  *
  * The ReopenIdx opcode works exactly like OpenRead except that
  * it first checks to see if the cursor on P1 is already open
@@ -3166,7 +3142,7 @@ case OP_SetCookie: {
  * The ReopenIdx opcode may only be used with P5 == 0.
  */
 /* Opcode: OpenWrite P1 P2 P3 P4 P5
- * Synopsis: index id = P2, space ptr = P3
+ * Synopsis: index id = P2, space ptr = P4
  *
  * For now, OpenWrite is an alias for OpenRead.
  * It exists just due legacy reasons and should be removed:
@@ -3182,9 +3158,7 @@ case OP_ReopenIdx: {
 	assert(pOp->p5==0 || pOp->p5==OPFLAG_SEEKEQ);
 	pCur = p->apCsr[pOp->p1];
 	p2 = pOp->p2;
-	pIn3 = &aMem[pOp->p3];
-	assert(pIn3->flags & MEM_Ptr);
-	if (pCur && pCur->uc.pCursor->space == (struct space *) pIn3->u.p &&
+	if (pCur && pCur->uc.pCursor->space == pOp->p4.space &&
 	    pCur->uc.pCursor->index->def->iid == (uint32_t)p2)
 		goto open_cursor_set_hints;
 	/* If the cursor is not currently open or is open on a different
@@ -3194,13 +3168,7 @@ case OP_OpenRead:
 case OP_OpenWrite:
 
 	assert(pOp->opcode==OP_OpenWrite || pOp->p5==0 || pOp->p5==OPFLAG_SEEKEQ);
-	/*
-	 * Even if schema has changed, pointer can come from
-	 * OP_SIDtoPtr opcode, which converts space id to pointer
-	 * during runtime.
-	 */
-	if (box_schema_version() != p->schema_ver &&
-	    (pOp->p5 & OPFLAG_FRESH_PTR) == 0) {
+	if (box_schema_version() != p->schema_ver) {
 		p->expired = 1;
 		rc = SQLITE_ERROR;
 		sqlite3VdbeError(p, "schema version has changed: " \
@@ -3208,9 +3176,7 @@ case OP_OpenWrite:
 		goto abort_due_to_error;
 	}
 	p2 = pOp->p2;
-	pIn3 = &aMem[pOp->p3];
-	assert(pIn3->flags & MEM_Ptr);
-	struct space *space = ((struct space *) pIn3->u.p);
+	struct space *space = pOp->p4.space;
 	assert(space != NULL);
 	struct index *index = space_index(space, p2);
 	assert(index != NULL);
@@ -4461,26 +4427,6 @@ case OP_SDelete: {
 	break;
 }
 
-/* Opcode: SIDtoPtr P1 P2 * * *
- * Synopsis: space id = P1, space[out] = r[P2]
- *
- * This opcode makes look up by space id and save found space
- * into register, specified by the content of register P2.
- * Such trick is needed during DLL routine, since schema may
- * change and pointers become expired.
- */
-case OP_SIDtoPtr: {
-	assert(pOp->p1 > 0);
-	assert(pOp->p2 >= 0);
-
-	pIn2 = out2Prerelease(p, pOp);
-	struct space *space = space_by_id(pOp->p1);
-	assert(space != NULL);
-	pIn2->u.p = (void *) space;
-	pIn2->flags = MEM_Ptr;
-	break;
-}
-
 /* Opcode: IdxDelete P1 P2 P3 * *
  * Synopsis: key=r[P2@P3]
  *
-- 
2.7.4

[tarantool-patches] Re: [PATCH v2 1/4] sql: get rid off sqlite3NestedParse in clean stats

[n.pettik]

> On 10 Jul 2018, at 20:08, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> Now we manually generate AST structures to drop outdated
> stats from _sql_stat1 and _sql_stat4 spaces instead of
> starting sqlite3NestedParse. This function become totally

Typo: becomes.

> 
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -116,71 +116,53 @@
> +vdbe_emit_stat_space_open(struct Parse *parse, int stat_cursor,
> +			  const char *index_name, const char *table_name)
> {
> -	const char *aTable[] = {
> -		"_sql_stat1",
> -		"_sql_stat4",
> -		NULL};
> -	int i;
> -	sqlite3 *db = pParse->db;
> -	Vdbe *v = sqlite3GetVdbe(pParse);
> -	int aRoot[ArraySize(aTable)];
> -	u8 aCreateTbl[ArraySize(aTable)];
> +	const char *space_names[] = {"_sql_stat1", "_sql_stat4"};

You already have argument ’table_name’, so it looks confusing.
Lets name it like ‘stat_names’. The same for space_ids.

> +	const uint32_t space_ids[] = {BOX_SQL_STAT1_ID, BOX_SQL_STAT4_ID};
> +	struct Vdbe *v = sqlite3GetVdbe(parse);
> +	assert(v != NULL);
> +	assert(sqlite3VdbeDb(v) == parse->db);
> 
> -	if (v == 0)
> -		return;
> -	assert(sqlite3VdbeDb(v) == db);
> -
> -	/* Create new statistic tables if they do not exist, or clear them
> -	 * if they do already exist.
> +	/*
> +	 * Create new statistic tables if they do not exist, or

But comment below says that ’tables already exist since they are system’.

> +	 * clear them if they do already exist.
> 	 */
> -	for (i = 0; aTable[i]; i++) {
> -		const char *zTab = aTable[i];
> -		Table *pStat;
> -		/* The table already exists, because it is a system space */
> -		pStat = sqlite3HashFind(&db->pSchema->tblHash, zTab);
> -		assert(pStat != NULL);
> -		aRoot[i] = pStat->tnum;
> -		aCreateTbl[i] = 0;
> -		if (zWhere) {
> -			sqlite3NestedParse(pParse,
> -					   "DELETE FROM \"%s\" WHERE \"%s\"=%Q",
> -					   zTab, zWhereType, zWhere);
> +	for (uint i = 0; i < lengthof(space_names); ++i) {
> +		const char *space_name = space_names[i];
> +		/*
> +		 * The table already exists, because it is a
> +		 * system space.
> +		 */
> +		assert(sqlite3HashFind(&parse->db->pSchema->tblHash,
> +				       space_name) != NULL);
> +		if (table_name != NULL || index_name != NULL) {

I don’t understand situation when index_name != NULL but table_name == NULL.
In our SQL index name is local to table, (i.e. indexes with the same name could
exist within different tables). I see that vdbe_emit_stat_space_open() is called twice
in analyzeTable() and analyzeTable() in turn always called with  pOnlyIdx == NULL.
Hence, in vdbe_emit_stat_space_open() index_name is always == NULL.
Lets remove this dead code and excess arg.

> +			vdbe_emit_stat_space_clear(parse, space_name,
> +						   index_name, table_name);
> 		} else {
> -			/*
> -			 * The sql_stat[134] table already exists.
> -			 * Delete all rows.
> -			 */
> -			sqlite3VdbeAddOp2(v, OP_Clear,
> -					  SQLITE_PAGENO_TO_SPACEID(aRoot[i]), 0);
> +			sqlite3VdbeAddOp2(v, OP_Clear, space_ids[i], 0);

OP_Clear uses only first operand, so you can use sqlite3VdbeAddOp1();

[tarantool-patches] Re: [PATCH v2 3/4] sql: refactor vdbe_emit_open_cursor calls

[n.pettik]

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 58e159c..62b85d5 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -179,41 +179,51 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg)
> -static int
> -readsTable(Parse * p, Table * pTab)
> +static bool
> +vdbe_has_table_read(struct Parse *parser, const struct Table *table)
> {
> +		if (op->opcode == OP_OpenRead || op->opcode == OP_OpenWrite) {
> +			assert(i > 1);
> +			struct VdbeOp *space_var_op =
> +				sqlite3VdbeGetOp(v, i - 1);
> +			assert(space_var_op != NULL);
> +			assert(space_var_op->opcode == OP_LoadPtr);
> +			struct space *space = space_var_op->p4.space;
> +
> +			if (space->def->id == table->def->id)
> +				return true;
> +
> +			int idx_id = op->p2;
> +			for (struct Index *pIndex = table->pIndex;
> +				pIndex != NULL; pIndex = pIndex->pNext) {
> +				if (idx_id ==
> +				    SQLITE_PAGENO_TO_INDEXID(pIndex->tnum))
> +					return true;

Why do you need these index iterations? If spaces don’t match, what is the
point to check index ids? Btw, I deleted this code and all tests seem to pass.

The rest is OK in this patch.

[tarantool-patches] Re: [PATCH v2 2/4] sql: remove usless sqlite3NestedParse function

[n.pettik]

> On 10 Jul 2018, at 20:08, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> As sqlite3NestedParse become totaly useless, let’s

Typo: becomes or became.

[tarantool-patches] Re: [PATCH v2 4/4] sql: remove OP_LoadPtr

[n.pettik]

> @@ -2969,12 +2964,9 @@ sql_create_index(struct Parse *parse, struct Token *token,
> 			goto exit_create_index;
> 
> 		sql_set_multi_write(parse, true);
> -
> -
> -		sqlite3VdbeAddOp2(v, OP_SIDtoPtr, BOX_INDEX_ID,
> -				  index_space_ptr_reg);
> -		sqlite3VdbeAddOp4Int(v, OP_OpenWrite, iCursor, 0,
> -				     index_space_ptr_reg, 6);
> +		sqlite3VdbeAddOp4(v, OP_OpenWrite, iCursor, 0, 0,
> +				  (void *)space_by_id(BOX_INDEX_ID),
> +				  P4_SPACEPTR);
> 		sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ);

Wait, AFAIK SIDtoPtr was used deliberately taking into consideration
the fact that DDL may change ptr to space. So conversion id -> space ptr
is delayed until vdbe execution (right before cursor opening). Am I wrong?

[tarantool-patches] Re: [PATCH v2 4/4] sql: remove OP_LoadPtr

[Vladislav Shpilevoy]

On 10/07/2018 21:34, n.pettik wrote:
> 
>> @@ -2969,12 +2964,9 @@ sql_create_index(struct Parse *parse, struct Token *token,
>> 			goto exit_create_index;
>>
>> 		sql_set_multi_write(parse, true);
>> -
>> -
>> -		sqlite3VdbeAddOp2(v, OP_SIDtoPtr, BOX_INDEX_ID,
>> -				  index_space_ptr_reg);
>> -		sqlite3VdbeAddOp4Int(v, OP_OpenWrite, iCursor, 0,
>> -				     index_space_ptr_reg, 6);
>> +		sqlite3VdbeAddOp4(v, OP_OpenWrite, iCursor, 0, 0,
>> +				  (void *)space_by_id(BOX_INDEX_ID),
>> +				  P4_SPACEPTR);
>> 		sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ);
> 
> Wait, AFAIK SIDtoPtr was used deliberately taking into consideration
> the fact that DDL may change ptr to space. So conversion id -> space ptr
> is delayed until vdbe execution (right before cursor opening). Am I wrong?

Yes, you are wrong here, because it was used to restore pointer to
struct space "_index", but "_index" is a system space, and it is never
altered. So pointer to "_index" is always valid, and it makes no sense
to restore it on runtime.

> 

[tarantool-patches] Re: [PATCH v2 4/4] sql: remove OP_LoadPtr

[n.pettik]

> Yes, you are wrong here, because it was used to restore pointer to
> struct space "_index", but "_index" is a system space, and it is never
> altered. So pointer to "_index" is always valid, and it makes no sense
> to restore it on runtime.

Then I am OK with last patch in series.

[tarantool-patches] Re: [PATCH v2 2/4] sql: remove usless sqlite3NestedParse function

[Kirill Shcherbatov]

> Typo: becomes or became.
Ok, tnx.

[tarantool-patches] Re: [PATCH v2 3/4] sql: refactor vdbe_emit_open_cursor calls

[Kirill Shcherbatov]

> Why do you need these index iterations? If spaces don’t match, what is the
> point to check index ids? Btw, I deleted this code and all tests seem to pass.
I've dropped. Tnx.

> The rest is OK in this patch.

[tarantool-patches] Re: [PATCH v2 1/4] sql: get rid off sqlite3NestedParse in clean stats

[Kirill Shcherbatov]

Thank you for review.

> Typo: becomes.
Ok, tnx.

> You already have argument ’table_name’, so it looks confusing.
> Lets name it like ‘stat_names’. The same for space_ids.
-       const char *space_names[] = {"_sql_stat1", "_sql_stat4"};
-       const uint32_t space_ids[] = {BOX_SQL_STAT1_ID, BOX_SQL_STAT4_ID};
+       const char *stat_names[] = {"_sql_stat1", "_sql_stat4"};
+       const uint32_t stat_ids[] = {BOX_SQL_STAT1_ID, BOX_SQL_STAT4_ID};

> But comment below says that ’tables already exist since they are system’.
Hmm, loks like some legacy. 
-
-       /*
-        * Create new statistic tables if they do not exist, or
-        * clear them if they do already exist.
-        */

> 
>> +	 * clear them if they do already exist.
>> 	 */
>> -	for (i = 0; aTable[i]; i++) {
>> -		const char *zTab = aTable[i];
>> -		Table *pStat;
>> -		/* The table already exists, because it is a system space */
>> -		pStat = sqlite3HashFind(&db->pSchema->tblHash, zTab);
>> -		assert(pStat != NULL);
>> -		aRoot[i] = pStat->tnum;
>> -		aCreateTbl[i] = 0;
>> -		if (zWhere) {
>> -			sqlite3NestedParse(pParse,
>> -					   "DELETE FROM \"%s\" WHERE \"%s\"=%Q",
>> -					   zTab, zWhereType, zWhere);
>> +	for (uint i = 0; i < lengthof(space_names); ++i) {
>> +		const char *space_name = space_names[i];
>> +		/*
>> +		 * The table already exists, because it is a
>> +		 * system space.
>> +		 */
>> +		assert(sqlite3HashFind(&parse->db->pSchema->tblHash,
>> +				       space_name) != NULL);
>> +		if (table_name != NULL || index_name != NULL) {
> 
> I don’t understand situation when index_name != NULL but table_name == NULL.
> In our SQL index name is local to table, (i.e. indexes with the same name could
> exist within different tables). I see that vdbe_emit_stat_space_open() is called twice
> in analyzeTable() and analyzeTable() in turn always called with  pOnlyIdx == NULL.
> Hence, in vdbe_emit_stat_space_open() index_name is always == NULL.
> Lets remove this dead code and excess arg.

-               if (table_name != NULL || index_name != NULL) {
+               if (table_name != NULL) {

/**
 * Generate code that will do an analysis of a single table in
 * a database.
 *
 * @param parse Parser context.
 * @param table Target table to analyze.
 */
static void
vdbe_emit_analyze_table(struct Parse *parse, struct Table *table)
{
	assert(table != NULL);
	sql_set_multi_write(parse, false);
	int stat_cursor = parse->nTab;
	parse->nTab += 3;
	vdbe_emit_stat_space_open(parse, stat_cursor, NULL, table->def->name);
	analyzeOneTable(parse, table, NULL, stat_cursor, parse->nMem + 1,
			parse->nTab);
	loadAnalysis(parse);
}


> OP_Clear uses only first operand, so you can use sqlite3VdbeAddOp1();
-                       sqlite3VdbeAddOp2(v, OP_Clear, stat_ids[i], 0);
+                       sqlite3VdbeAddOp1(v, OP_Clear, stat_ids[i]);

[tarantool-patches] Re: [PATCH v2 1/4] sql: get rid off sqlite3NestedParse in clean stats

[n.pettik]

> * Generate code that will do an analysis of a single table in
> * a database.
> *
> * @param parse Parser context.
> * @param table Target table to analyze.
> */
> static void
> vdbe_emit_analyze_table(struct Parse *parse, struct Table *table)
> {
> 	assert(table != NULL);
> 	sql_set_multi_write(parse, false);
> 	int stat_cursor = parse->nTab;
> 	parse->nTab += 3;
> 	vdbe_emit_stat_space_open(parse, stat_cursor, NULL, table->def->name);

Look, vdbe_emit_stat_space_open() still always takes NULL as index_name.
Lets remove it. 

[tarantool-patches] Re: [PATCH v2 1/4] sql: get rid off sqlite3NestedParse in clean stats

[Kirill Shcherbatov]

> Look, vdbe_emit_stat_space_open() still always takes NULL as index_name.
> Lets remove it. 
Done.

[tarantool-patches] Re: [PATCH v2 1/4] sql: get rid off sqlite3NestedParse in clean stats

[n.pettik]

> On 11 Jul 2018, at 15:23, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
>> Look, vdbe_emit_stat_space_open() still always takes NULL as index_name.
>> Lets remove it. 
> Done.

Thx, now whole patch-set LGTM.

[tarantool-patches] Re: [PATCH v2 0/4] sql: get rid off sqlite3NestedParse

[Kirill Yukhin]

Hello,
On 10 июл 20:08, Kirill Shcherbatov wrote:
> As we are going to implement parser as separate library,
> we should get rid off sqlite3NestedParse calls.   
> Last usages in statistics update could be trivially rewrited
> to do not start parsing. We manually build AST structures 
> simmilar to parser did and manually call sql_table_delete_from. 
> Finilly, I've removed all sqlite3NestedParse mentionings.
> Also slightly refactored OP_Open{Write,Read} opcodes and 
> vdbe_emit_open_cursor function to use index_id instead of tnum.
> 
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3496-no-nested-parse
> Issue: https://github.com/tarantool/tarantool/issues/3496
I've pushed the patchset into 2.0 branch.

--
Regards, Kirill Yukhin