[tarantool-patches] [PATCH 0/3] Ephemeral tables perf leak & next rowid bug

[tarantool-patches] [PATCH 0/3] Ephemeral tables perf leak & next rowid bug

[AKhatskevich]

Key points:
 - creater issue #3297 (OP_NextIdEphemeral produces wrong rowids)
 - #3297 fixed for the most common case (but there still is code
  in select.c which may fail because of the bug); the fix just
  allocates register and increments it before each insert
 - small mem leak fixed

AKhatskevich (3):
  sql: Generate rowid by counter
  sql: fix memory leak
  sql: report errors in EphemeralGetMaxId

 src/box/sql.c        |  4 +++-
 src/box/sql/insert.c | 10 +++++++++-
 src/box/sql/select.c |  6 +++---
 3 files changed, 15 insertions(+), 5 deletions(-)

-- 
2.14.1

[tarantool-patches] [PATCH 1/3] sql: Generate rowid by counter

[AKhatskevich]

The function `OP_NextIdEphemeral` do not produce unique ids.

The new way to get rowid is to create sequential a counter.
One of registers initializes with int64_t = 0 and increases after each
insert.

There are similar cases in `select.c` file, however, they are not as
straight-forward and would be fixed in future commits.

Related to #3297
---
 src/box/sql/insert.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 28c01092d..4f3cb3116 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -536,6 +536,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 			int regTempId;	/* Register to hold temp table ID */
 			int regCopy;    /* Register to keep copy of registers from select */
 			int addrL;	/* Label "L" */
+			int64_t initial_pk = 0;
 
 			srcTab = pParse->nTab++;
 			regRec = sqlite3GetTempReg(pParse);
@@ -544,9 +545,16 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 			KeyInfo *pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, 1+nColumn, 0);
 			sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, srcTab, nColumn+1,
 					  0, (char*)pKeyInfo, P4_KEYINFO);
+			/* Create counter for rowid */
+			sqlite3VdbeAddOp4Dup8(v, OP_Int64,
+					      0 /* unused */,
+					      regTempId,
+					      0 /* unused */,
+					      (const unsigned char*) &initial_pk,
+					      P4_INT64);
 			addrL = sqlite3VdbeAddOp1(v, OP_Yield, dest.iSDParm);
 			VdbeCoverage(v);
-			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, srcTab, 2, regTempId);
+			sqlite3VdbeAddOp2(v, OP_AddImm, regTempId, 1);
 			sqlite3VdbeAddOp3(v, OP_Copy, regFromSelect, regCopy, nColumn-1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy,
 					  nColumn + 1, regRec);
-- 
2.14.1

[tarantool-patches] [PATCH 2/3] sql: fix memory leak

[AKhatskevich]

index_def_new makes its own copy of key_def, so it should be deleted.
---
 src/box/sql.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/box/sql.c b/src/box/sql.c
index 224747157..98bcd93ca 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -442,6 +442,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
 		index_def_new(0 /*space id */, 0 /* index id */, "ephemer_idx",
 			      strlen("ephemer_idx"), TREE, &index_opts_default,
 			      ephemer_key_def, NULL /* pk def */);
+	key_def_delete(ephemer_key_def);
 
 	struct rlist key_list;
 	rlist_create(&key_list);
-- 
2.14.1

[tarantool-patches] [PATCH 3/3] sql: report errors in EphemeralGetMaxId

[AKhatskevich]

In case of error in `tarantoolSqlite3EphemeralGetMaxId` it was creating
diag string and silently continue working. It was a huge luck that
non-valid output of the function did not lead to crashes.

After the fix, it was found that some opcodes in `select.c` were using
wrong registers. These were fixed too, but they still have bugs. For
more information see #3297.

Related to #3297
---
 src/box/sql.c        | 3 ++-
 src/box/sql/select.c | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 98bcd93ca..31ceacb53 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1744,7 +1744,8 @@ int tarantoolSqlite3EphemeralGetMaxId(BtCursor *pCur, uint32_t fieldno,
 		*max_id = 0;
 		return SQLITE_OK;
 	}
-	tuple_field_u64(tuple, fieldno, max_id);
+	if (tuple_field_u64(tuple, fieldno, max_id) == -1)
+		return SQL_TARANTOOL_ERROR;
 
 	return SQLITE_OK;
 }
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index a76286f23..c91685f7f 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1025,7 +1025,7 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 				/* Last column is required for ID. */
 				int regCopy = sqlite3GetTempRange(pParse, nResultCol + 1);
 				sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, iParm,
-						  0, regCopy + nResultCol);
+						  nResultCol, regCopy + nResultCol);
 				/* Positioning ID column to be last in inserted tuple.
 				 * NextId -> regCopy + n + 1
 				 * Copy [regResult, regResult + n] -> [regCopy, regCopy + n]
@@ -1504,7 +1504,7 @@ generateSortTail(Parse * pParse,	/* Parsing context */
 	case SRT_Table:
 	case SRT_EphemTab: {
 			int regCopy = sqlite3GetTempRange(pParse,  nColumn);
-			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, iParm, 0, regTupleid);
+			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, iParm, nColumn, regTupleid);
 			sqlite3VdbeAddOp3(v, OP_Copy, regRow, regCopy, nSortData - 1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy, nColumn + 1, regRow);
 			sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, regRow);
@@ -2957,7 +2957,7 @@ generateOutputSubroutine(Parse * pParse,	/* Parsing context */
 			int regRec = sqlite3GetTempReg(pParse);
 			int regCopy = sqlite3GetTempRange(pParse, pIn->nSdst + 1);
 			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, pDest->iSDParm,
-					  0, regCopy + pIn->nSdst);
+					  pIn->nSdst, regCopy + pIn->nSdst);
 			sqlite3VdbeAddOp3(v, OP_Copy, pIn->iSdst, regCopy,
 					  pIn->nSdst - 1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy,
-- 
2.14.1