[tarantool-patches] [PATCH v1 0/2] sql: get rid off sqlite3NestedParse

[tarantool-patches] [PATCH v1 0/2] 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.

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

Kirill Shcherbatov (2):
  sql: get rid off sqlite3NestedParse in clean stats
  sql: remove usless sqlite3NestedParse function

 src/box/sql/analyze.c   |  39 ++++++++--------
 src/box/sql/build.c     | 122 ++++++++++++++++++++++++------------------------
 src/box/sql/delete.c    |   6 +--
 src/box/sql/insert.c    |   6 +--
 src/box/sql/sqliteInt.h |  14 +++++-
 src/box/sql/update.c    |   6 +--
 6 files changed, 97 insertions(+), 96 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v1 1/2] 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   | 39 +++++++++++------------
 src/box/sql/build.c     | 82 ++++++++++++++++++++++++++++++++++++-------------
 src/box/sql/sqliteInt.h | 13 ++++++++
 3 files changed, 94 insertions(+), 40 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 5f73f02..e08c151 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -116,7 +116,7 @@
 #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.
@@ -127,21 +127,24 @@
  * 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_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);
+	struct sqlite3 *db = parse->db;
+	struct Vdbe *v = sqlite3GetVdbe(parse);
+	assert(v != NULL);
 	int aRoot[ArraySize(aTable)];
 	u8 aCreateTbl[ArraySize(aTable)];
 
@@ -160,10 +163,9 @@ openStatTable(Parse * pParse,	/* Parsing context */
 		assert(pStat != NULL);
 		aRoot[i] = pStat->tnum;
 		aCreateTbl[i] = 0;
-		if (zWhere) {
-			sqlite3NestedParse(pParse,
-					   "DELETE FROM \"%s\" WHERE \"%s\"=%Q",
-					   zTab, zWhereType, zWhere);
+		if (table_name != NULL || index_name != NULL) {
+			vdbe_stat_space_clear(parse, zTab, index_name,
+					      table_name);
 		} else {
 			/*
 			 * The sql_stat[134] table already exists.
@@ -178,7 +180,7 @@ openStatTable(Parse * pParse,	/* Parsing context */
 	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);
+		vdbe_emit_open_cursor(parse, stat_cursor + i, aRoot[i], space);
 		sqlite3VdbeChangeP5(v, aCreateTbl[i]);
 		VdbeComment((v, aTable[i]));
 	}
@@ -1117,7 +1119,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_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,11 +1147,10 @@ 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");
-	} else {
-		openStatTable(pParse, iStatCur, pTab->def->name, "tbl");
-	}
+	if (pOnlyIdx != NULL)
+		vdbe_stat_space_open(pParse, iStatCur, pOnlyIdx->zName, NULL);
+	else
+		vdbe_stat_space_open(pParse, iStatCur, NULL, pTab->def->name);
 	analyzeOneTable(pParse, pTab, pOnlyIdx, iStatCur, pParse->nMem + 1,
 			pParse->nTab);
 	loadAnalysis(pParse);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 0072f84..ac53906 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2050,6 +2050,63 @@ sql_store_select(struct Parse *parse_context, struct Select *select)
 }
 
 /**
+ * Create expression record of with struct ID EQ STRING.
+ *
+ * @param parse The parsing context.
+ * @param col_type_name Name of column.
+ * @param col_name 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_type_name,
+		   const char *col_name)
+{
+	struct sqlite3 *db = parse->db;
+
+	struct Expr *col_type_expr =
+		sqlite3Expr(db, TK_ID, col_type_name);
+	struct Expr *col_name_expr =
+		sqlite3Expr(db, TK_STRING, col_name);
+	struct Expr *col_eq_expr =
+		sqlite3PExpr(parse, TK_EQ, col_type_expr, col_name_expr);
+	if (col_type_expr == NULL || col_name_expr == NULL) {
+		sql_expr_delete(db, col_eq_expr, false);
+		col_eq_expr = NULL;
+	}
+	return col_eq_expr;
+}
+
+void
+vdbe_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.
  *
@@ -2062,27 +2119,10 @@ static void
 sql_clear_stat_spaces(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_stat_space_clear(parse, "_sql_stat4", idx_name,
+			      table_name);
+	vdbe_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..a0a874c 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_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] Re: [PATCH v1 1/2] sql: get rid off sqlite3NestedParse in clean stats

[Vladislav Shpilevoy]

Hello. Thanks for the patch!

I have pushed my review fixes on the branch. Please, squash, if
you agree, and debug if the tests fail. Below you may find 3
comments that I fixed. Style violations I did not mentioned here.
Just fixed on the branch.

Also I have found that vdbe_emit_open_cursor() has the second
parameter named 'index_id', but in some places the function
takes real index_id, in other places it takes tnum, and in vdbe
it is interpreted as tnum. Please, fix this mess in a separate
commit. I think, we should always pass index_id.

On 04/07/2018 20:17, Kirill Shcherbatov 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
> useless and could be removed.
> 
> Part of #3496.
> ---
>   src/box/sql/analyze.c   | 39 +++++++++++------------
>   src/box/sql/build.c     | 82 ++++++++++++++++++++++++++++++++++++-------------
>   src/box/sql/sqliteInt.h | 13 ++++++++
>   3 files changed, 94 insertions(+), 40 deletions(-)
> 
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 5f73f02..e08c151 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -116,7 +116,7 @@
>   #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.
> @@ -160,10 +163,9 @@ openStatTable(Parse * pParse,	/* Parsing context */
>   		assert(pStat != NULL);
>   		aRoot[i] = pStat->tnum;
>   		aCreateTbl[i] = 0;

1. Unused array.

2. You do not need to lookup pStat for tnum in non-debug build since
you know stat tables id from schema_def.h. This HashFind is useful for
debug assertion only.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0072f84..ac53906 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2050,6 +2050,63 @@ sql_store_select(struct Parse *parse_context, struct Select *select)
>   }
>   
>   /**
> + * Create expression record of with struct ID EQ STRING.
> + *
> + * @param parse The parsing context.
> + * @param col_type_name Name of column.
> + * @param col_name 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_type_name,
> +		   const char *col_name)
> +{
> +	struct sqlite3 *db = parse->db;
> +
> +	struct Expr *col_type_expr =
> +		sqlite3Expr(db, TK_ID, col_type_name);
> +	struct Expr *col_name_expr =
> +		sqlite3Expr(db, TK_STRING, col_name);
> +	struct Expr *col_eq_expr =
> +		sqlite3PExpr(parse, TK_EQ, col_type_expr, col_name_expr);
> +	if (col_type_expr == NULL || col_name_expr == NULL) {

3. If col_eq_expr == NULL here, then col_type/name_expr leak.

> +		sql_expr_delete(db, col_eq_expr, false);
> +		col_eq_expr = NULL;
> +	}
> +	return col_eq_expr;
> +}
> +

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

[Kirill Shcherbatov]

> I have pushed my review fixes on the branch. Please, squash, if
> you agree, and debug if the tests fail. Below you may find 3
> comments that I fixed. Style violations I did not mentioned here.
> Just fixed on the branch.
Hello! Thank you for so detailed review fixes.
I've squashed your changes, they are looking good for me and they also pass all
tests.

> Also I have found that vdbe_emit_open_cursor() has the second
> parameter named 'index_id', but in some places the function
> takes real index_id, in other places it takes tnum, and in vdbe
> it is interpreted as tnum. Please, fix this mess in a separate
> commit. I think, we should always pass index_id.
I've tried to do this and I'll append my diff to the end of this message,
but there are some strange things.. 
This should be investigated separately. I don't mind to do this, but I believe 
that this may  delayed to be not a part of "remove sqlite3NestedParse function".

Take a look at the end of this message. 

> 1. Unused array.
> 2. You do not need to lookup pStat for tnum in non-debug build since
> you know stat tables id from schema_def.h. This HashFind is useful for
> debug assertion only.
You've fixed all of this.

>> +	struct Expr *col_eq_expr =
>> +		sqlite3PExpr(parse, TK_EQ, col_type_expr, col_name_expr);
>> +	if (col_type_expr == NULL || col_name_expr == NULL) {
> 
> 3. If col_eq_expr == NULL here, then col_type/name_expr leak.
As you already coincide (according to you changes on branch), there is no 
leak as  sqlite3PExpr manually releases arguments on failure.

----------------------------------

From c5c1d325c32a3d9d9e75fbb8f6a8d58e3bec3a34 Mon Sep 17 00:00:00 2001
Message-Id: <c5c1d325c32a3d9d9e75fbb8f6a8d58e3bec3a34.1530900139.git.kshcherbatov@tarantool.org>
In-Reply-To: <cover.1530900139.git.kshcherbatov@tarantool.org>
References: <cover.1530900139.git.kshcherbatov@tarantool.org>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Fri, 6 Jul 2018 20:18:08 +0300
Subject: [PATCH 3/3] crap

---
 src/box/sql/analyze.c |  3 +--
 src/box/sql/build.c   |  3 ++-
 src/box/sql/expr.c    |  5 ++++-
 src/box/sql/fkey.c    |  3 ++-
 src/box/sql/insert.c  |  8 ++++++--
 src/box/sql/select.c  |  4 +---
 src/box/sql/vdbe.c    |  4 +++-
 src/box/sql/where.c   | 18 ++++++++++++++++--
 8 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 336d146..ff3e735 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]));
 	}
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/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..3beae89 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1934,11 +1934,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/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/vdbe.c b/src/box/sql/vdbe.c
index 7a4d376..00b67dc 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3208,12 +3208,14 @@ case OP_OpenWrite:
 				    "need to re-compile SQL statement");
 		goto abort_due_to_error;
 	}
+	pOp->p2 = SQLITE_PAGENO_TO_INDEXID(p2);
 	p2 = pOp->p2;
+
 	pIn3 = &aMem[pOp->p3];
 	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..f83456a 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -4759,10 +4759,24 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 			assert(iIndexCur >= 0);
 			if (op) {
 				if (pIx != NULL) {
+					/*
+					 * This diff cause really strange
+					 * effects for memtx.
+					 * Failed to allocate * bytes for tuple:
+					 * tuple is too large.
+					 * Check 'memtx_max_tuple_size'
+					 * configuration option.
+					 */
+					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


----------------------------------

[001] sql-tap/misc3.test.lua                          vinyl           [ pass ]
[002] sql-tap/misc3.test.lua                          memtx           
[002] not ok 2 - misc3-1.2: Execution failed: Failed to allocate 6553623 bytes for tuple: tuple is too large. Check 'memtx_max_tuple_size' configuration option. #  
[002] Traceback:
[002] [Lua ] function 'do_test' at <./sqltester.lua:134>
[002] [main] at </home/kir/tarantool/test/sql-tap/misc3.test.lua:0>
[002] 
[002] not ok 25 - misc3-4.1: Execution failed: Failed to allocate 12131 bytes in slab allocator for memtx_tuple #  
[002] Traceback:
[002] [Lua ] function 'do_execsql_test' at <./sqltester.lua:134>
[002] [main] at </home/kir/tarantool/test/sql-tap/misc3.test.lua:0>
[002] 
[002] not ok 26 - misc3-4.2 #  
[002] Traceback:
[002] [Lua ] function 'do_execsql_test' at <./sqltester.lua:134>
[002] [main] at </home/kir/tarantool/test/sql-tap/misc3.test.lua:0>
[002] 
[002] not ok 27 - misc3-4.3 #  
[002] Traceback:
[002] [Lua ] function 'do_execsql_test' at <./sqltester.lua:134>
[002] [main] at </home/kir/tarantool/test/sql-tap/misc3.test.lua:0>
[002] 
[002] Rejected result file: sql-tap/misc3.reject
[002] [ fail ]
[Main process] Got failed test; gently terminate all workers...
[002] Worker "002_sql-tap" got failed test; stopping the server...

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

[Vladislav Shpilevoy]

Hello. Thanks for the fixes! I have force pushed my two-line minor
fix of a comment on the branch.

>> Also I have found that vdbe_emit_open_cursor() has the second
>> parameter named 'index_id', but in some places the function
>> takes real index_id, in other places it takes tnum, and in vdbe
>> it is interpreted as tnum. Please, fix this mess in a separate
>> commit. I think, we should always pass index_id.
> I've tried to do this and I'll append my diff to the end of this message,
> but there are some strange things..
> This should be investigated separately. I don't mind to do this, but I believe
> that this may  delayed to be not a part of "remove sqlite3NestedParse function".
> 
> Take a look at the end of this message.

Then fix this please on a separate branch. If it is easy, then it can be
done without an issue. If it takes more than 4 hours, then create an issue,
please.

[tarantool-patches] [PATCH v1 2/2] 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     | 40 ----------------------------------------
 src/box/sql/delete.c    |  6 +-----
 src/box/sql/insert.c    |  6 +-----
 src/box/sql/sqliteInt.h |  1 -
 src/box/sql/update.c    |  6 +-----
 5 files changed, 3 insertions(+), 56 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ac53906..573da9a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -116,46 +116,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
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 5a79971..5f78062 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -403,11 +403,7 @@ 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) {
 		sqlite3VdbeAddOp2(v, OP_ResultRow, reg_count, 1);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index c12043b..6ce5be7 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -904,11 +904,7 @@ 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.
-	 */
+	/* Return the number of rows inserted. */
 	if ((user_session->sql_flags & SQLITE_CountRows) && !pParse->nested
 	    && !pParse->pTriggerTab) {
 		sqlite3VdbeAddOp2(v, OP_ResultRow, regRowCount, 1);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index a0a874c..7a3a3d1 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4449,7 +4449,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/update.c b/src/box/sql/update.c
index 212adbc..3a5c6f0 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -625,11 +625,7 @@ 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.
-	 */
+	/* Return the number of rows that were changed. */
 	if ((user_session->sql_flags & SQLITE_CountRows) &&
 	    !pParse->pTriggerTab && !pParse->nested) {
 		sqlite3VdbeAddOp2(v, OP_ResultRow, regRowCount, 1);
-- 
2.7.4

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

[Vladislav Shpilevoy]

Thanks for the patch!

Please, finish it. You did not remove Parse.nested and
PARSE_TAIL. And maybe something else.


On 04/07/2018 20:17, Kirill Shcherbatov wrote:
> As sqlite3NestedParse become totaly useless, let's
> remove unreacheble code and all mentioning.
> 
> Resolves #3496.
> ---
>   src/box/sql/build.c     | 40 ----------------------------------------
>   src/box/sql/delete.c    |  6 +-----
>   src/box/sql/insert.c    |  6 +-----
>   src/box/sql/sqliteInt.h |  1 -
>   src/box/sql/update.c    |  6 +-----
>   5 files changed, 3 insertions(+), 56 deletions(-)
> 

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

[Kirill Shcherbatov]

> Please, finish it. You did not remove Parse.nested and
> PARSE_TAIL. And maybe something else.

Ok, I've removed nested, and some redundant macro definitions.
I've grepp(ed) 'cd src/box/sql; grep -ir nested'
there are still few "nested" words, but they are not about Parser and should be kept.

-----------------------------

From bfb87033cb39d5f5c5c7d36abbbbc80e3b99fd4f Mon Sep 17 00:00:00 2001
Message-Id: <bfb87033cb39d5f5c5c7d36abbbbc80e3b99fd4f.1530900139.git.kshcherbatov@tarantool.org>
In-Reply-To: <cover.1530900139.git.kshcherbatov@tarantool.org>
References: <cover.1530900139.git.kshcherbatov@tarantool.org>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Wed, 4 Jul 2018 20:05:00 +0300
Subject: [PATCH 2/3] sql: remove usless sqlite3NestedParse function

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] Re: [PATCH v1 0/2] sql: get rid off sqlite3NestedParse

[Vladislav Shpilevoy]

LGTM. Nikita, please, make a second review.

On 04/07/2018 20:17, 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.
> 
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3496-no-nested-parse
> Issue: https://github.com/tarantool/tarantool/issues/3496
> 
> Kirill Shcherbatov (2):
>    sql: get rid off sqlite3NestedParse in clean stats
>    sql: remove usless sqlite3NestedParse function
> 
>   src/box/sql/analyze.c   |  39 ++++++++--------
>   src/box/sql/build.c     | 122 ++++++++++++++++++++++++------------------------
>   src/box/sql/delete.c    |   6 +--
>   src/box/sql/insert.c    |   6 +--
>   src/box/sql/sqliteInt.h |  14 +++++-
>   src/box/sql/update.c    |   6 +--
>   6 files changed, 97 insertions(+), 96 deletions(-)
>