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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

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