[tarantool-patches] [PATCH 0/2] sql: SQL bindings refactoring

[tarantool-patches] [PATCH 0/2] sql: SQL bindings refactoring

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/gh-3332-format-leak-fix
Issue: https://github.com/tarantool/tarantool/issues/3332

First patch introduces fix for leaked tuple formats: under certain
circumstances (described in commit message) some tuples from ephemeral
space may remain referenced after space's death and as a result, format
can't be destroyed.

Second patch consists of slight refactoring of cursor closing routine.

Nikita Pettik (2):
  sql: fix tuple format leak
  sql: refactor cursor closing routine

 src/box/sql.c                                   | 60 +++++++++++--------------
 src/box/sql/cursor.c                            | 46 ++++++++++---------
 src/box/sql/cursor.h                            |  6 ++-
 src/box/sql/tarantoolInt.h                      |  8 ++--
 src/box/sql/vdbe.c                              | 35 +++++----------
 src/box/sql/vdbeaux.c                           |  2 +-
 test/sql-tap/gh-3332-tuple-format-leak.test.lua | 31 +++++++++++++
 7 files changed, 101 insertions(+), 87 deletions(-)
 create mode 100755 test/sql-tap/gh-3332-tuple-format-leak.test.lua

-- 
2.15.1

[tarantool-patches] [PATCH 1/2] sql: fix tuple format leak

[Nikita Pettik]

Consider following VDBE program (X stands for cursor number):

OP_OpenTEphemeral X ...
OP_Last X ...
OP_IdxInsert X ...
OP_Last X ...

Before this patch, during execution of OP_IdxInsert key, which is held in
cursor, is nullified. However, during OP_Last key is checked on NULL.
If key is really NULL, new memory is allocated and iterator is
nullified (if key is NULL, then this cursor is accounted for brand
new and should be initialized). In this regard, iterator is lost without
being freed.
As a result, some tuples (alongside with iterator itself) remain
referenced, so format can't be freed. Thus, after certain number of
queries, whcih produce ephemeral spaces, tuple format limit is reached and
execution fails.

This patch simply changes signatures of functions which handle
REPLACE/INSERT, so that cursor is not invlolved in this routine.

Closes #3332
---
 src/box/sql.c                                   | 45 +++++++++++++------------
 src/box/sql/tarantoolInt.h                      |  7 ++--
 src/box/sql/vdbe.c                              | 33 ++++++------------
 test/sql-tap/gh-3332-tuple-format-leak.test.lua | 31 +++++++++++++++++
 4 files changed, 69 insertions(+), 47 deletions(-)
 create mode 100755 test/sql-tap/gh-3332-tuple-format-leak.test.lua

diff --git a/src/box/sql.c b/src/box/sql.c
index a6713f1f0..dd0cfcc1a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -451,17 +451,13 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
  *
  * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
  */
-int tarantoolSqlite3EphemeralInsert(BtCursor *pCur)
+int tarantoolSqlite3EphemeralInsert(struct space *space, char *tuple,
+				    char *tuple_end)
 {
-	assert(pCur);
-	assert(pCur->curFlags & BTCF_TEphemCursor);
-	mp_tuple_assert(pCur->key, pCur->key + pCur->nKey);
-
-	if (space_ephemeral_replace(pCur->space, pCur->key,
-				    pCur->key + pCur->nKey) != 0) {
-		diag_log();
+	assert(space != NULL);
+	mp_tuple_assert(tuple, tuple_end);
+	if (space_ephemeral_replace(space, tuple, tuple_end) != 0)
 		return SQL_TARANTOOL_INSERT_FAIL;
-	}
 	return SQLITE_OK;
 }
 
@@ -475,28 +471,29 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
 }
 
 static inline int
-insertOrReplace(BtCursor *pCur, enum iproto_type type)
+insertOrReplace(struct space *space, char *tuple, char *tuple_end,
+		enum iproto_type type)
 {
-	assert(pCur->curFlags & BTCF_TaCursor);
+	assert(space != NULL);
 	struct request request;
 	memset(&request, 0, sizeof(request));
-	request.tuple = pCur->key;
-	request.tuple_end = pCur->key + pCur->nKey;
-	request.space_id = pCur->space->def->id;
+	request.tuple = tuple;
+	request.tuple_end = tuple_end;
+	request.space_id = space->def->id;
 	request.type = type;
 	mp_tuple_assert(request.tuple, request.tuple_end);
-	int rc = box_process_rw(&request, pCur->space, NULL);
+	int rc = box_process_rw(&request, space, NULL);
 	return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;
 }
 
-int tarantoolSqlite3Insert(BtCursor *pCur)
+int tarantoolSqlite3Insert(struct space *space, char *tuple, char *tuple_end)
 {
-	return insertOrReplace(pCur, IPROTO_INSERT);
+	return insertOrReplace(space, tuple, tuple_end, IPROTO_INSERT);
 }
 
-int tarantoolSqlite3Replace(BtCursor *pCur)
+int tarantoolSqlite3Replace(struct space *space, char *tuple, char *tuple_end)
 {
-	return insertOrReplace(pCur, IPROTO_REPLACE);
+	return insertOrReplace(space, tuple, tuple_end, IPROTO_REPLACE);
 }
 
 /*
@@ -1092,8 +1089,14 @@ key_alloc(BtCursor *cur, size_t key_size)
 			diag_set(OutOfMemory, key_size, "malloc", "cur->key");
 			return -1;
 		}
-		cur->iter = NULL;
-		cur->last_tuple = NULL;
+		/*
+		 * Key can be NULL, only if it is a brand new
+		 * cursor. In this case, iterator and tuple must
+		 * also be NULLs, since memory for cursor is
+		 * filled with 0.
+		 */
+		assert(cur->iter == NULL);
+		assert(cur->last_tuple == NULL);
 	} else {
 		char *new_key = realloc(cur->key, key_size);
 		if (new_key == NULL) {
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 65acf1198..19c6fdca9 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -74,8 +74,8 @@ int tarantoolSqlite3Previous(BtCursor * pCur, int *pRes);
 int tarantoolSqlite3MovetoUnpacked(BtCursor * pCur, UnpackedRecord * pIdxKey,
 				   int *pRes);
 int tarantoolSqlite3Count(BtCursor * pCur, i64 * pnEntry);
-int tarantoolSqlite3Insert(BtCursor * pCur);
-int tarantoolSqlite3Replace(BtCursor * pCur);
+int tarantoolSqlite3Insert(struct space *space, char *tuple, char *tuple_end);
+int tarantoolSqlite3Replace(struct space *space, char *tuple, char *tuple_end);
 int tarantoolSqlite3Delete(BtCursor * pCur, u8 flags);
 int
 sql_delete_by_key(struct space *space, char *key, uint32_t key_size);
@@ -98,7 +98,8 @@ int tarantoolSqlite3RenameParentTable(int iTab, const char *zOldParentName,
 /* Interface for ephemeral tables. */
 int tarantoolSqlite3EphemeralCreate(BtCursor * pCur, uint32_t filed_count,
 				    struct coll *aColl);
-int tarantoolSqlite3EphemeralInsert(BtCursor * pCur);
+int tarantoolSqlite3EphemeralInsert(struct space *space, char *tuple,
+				    char *tuple_end);
 int tarantoolSqlite3EphemeralDelete(BtCursor * pCur);
 int tarantoolSqlite3EphemeralCount(BtCursor * pCur, i64 * pnEntry);
 int tarantoolSqlite3EphemeralDrop(BtCursor * pCur);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 823718398..8151e1557 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4358,33 +4358,26 @@ case OP_IdxInsert: {        /* in2 */
 		rc = sqlite3VdbeSorterWrite(pC, pIn2);
 	} else {
 		BtCursor *pBtCur = pC->uc.pCursor;
-		pBtCur->nKey = pIn2->n;
-		pBtCur->key = pIn2->z;
 		if (pBtCur->curFlags & BTCF_TaCursor) {
 			/* Make sure that memory has been allocated on region. */
 			assert(aMem[pOp->p2].flags & MEM_Ephem);
 			if (pOp->opcode == OP_IdxInsert)
-				rc = tarantoolSqlite3Insert(pBtCur);
+				rc = tarantoolSqlite3Insert(pBtCur->space,
+							    pIn2->z,
+							    pIn2->z + pIn2->n);
 			else
-				rc = tarantoolSqlite3Replace(pBtCur);
+				rc = tarantoolSqlite3Replace(pBtCur->space,
+							     pIn2->z,
+							     pIn2->z + pIn2->n);
 		} else if (pBtCur->curFlags & BTCF_TEphemCursor) {
-			rc = tarantoolSqlite3EphemeralInsert(pBtCur);
+			rc = tarantoolSqlite3EphemeralInsert(pBtCur->space,
+							     pIn2->z,
+							     pIn2->z + pIn2->n);
 		} else {
 			unreachable();
 		}
 		assert(pC->deferredMoveto==0);
 		pC->cacheStatus = CACHE_STALE;
-
-		/*
-		 * Memory for tuple passed to Tarantool is
-		 * allocated in region. This memory will be
-		 * automatically released by Tarantool.
-		 * However, VDBE in the end will also try to
-		 * release it, so pointers should be explicitly
-		 * nullified.
-		 */
-		pBtCur->nKey = 0;
-		pBtCur->key = NULL;
 	}
 
 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
@@ -4424,13 +4417,7 @@ case OP_SInsert: {
 	struct space *space = space_by_id(pOp->p1);
 	assert(space != NULL);
 	assert(space_is_system(space));
-	/* Create surrogate cursor to pass to SQL bindings. */
-	BtCursor surrogate_cur;
-	surrogate_cur.space = space;
-	surrogate_cur.key = pIn2->z;
-	surrogate_cur.nKey = pIn2->n;
-	surrogate_cur.curFlags = BTCF_TaCursor;
-	rc = tarantoolSqlite3Insert(&surrogate_cur);
+	rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
 	if (rc)
 		goto abort_due_to_error;
 	if (pOp->p5 & OPFLAG_NCHANGE)
diff --git a/test/sql-tap/gh-3332-tuple-format-leak.test.lua b/test/sql-tap/gh-3332-tuple-format-leak.test.lua
new file mode 100755
index 000000000..05b30aa31
--- /dev/null
+++ b/test/sql-tap/gh-3332-tuple-format-leak.test.lua
@@ -0,0 +1,31 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(2)
+
+test:do_test(
+    "format-leak-prep",
+    function()
+        box.sql.execute("CREATE TABLE t1(id UNSIGNED BIG INT PRIMARY KEY,\
+                         max_players INTEGER, n_players INTEGER, flags INTEGER);");
+        box.sql.execute("CREATE INDEX IDX_MAX_PLAYERS ON t1(max_players);");
+        box.sql.execute("CREATE INDEX IDX_N_PLAYERS ON t1(n_players);");
+        box.sql.execute("CREATE INDEX IDX_FLAGS ON t1(flags);");
+        for i = 1, 10 do
+            box.sql.execute(string.format("INSERT INTO t1 VALUES (%s, %s, %s, %s);",
+                                          i, 15, 6, 3));
+        end
+    end, {
+
+    })
+
+test:do_test(
+    "format-leak",
+    function()
+        for i = 1, 100000 do
+            box.sql.execute("SELECT id FROM t1 WHERE flags=3 ORDER BY id LIMIT 2");
+        end
+    end, {
+
+    })
+
+test:finish_test()
-- 
2.15.1

[tarantool-patches] [PATCH 2/2] sql: refactor cursor closing routine

[Nikita Pettik]

Unite 'close' and 'clear' functions into one. Fix naming and codestyle.
---
 src/box/sql.c              | 15 +--------------
 src/box/sql/cursor.c       | 46 +++++++++++++++++++++++++---------------------
 src/box/sql/cursor.h       |  6 ++++--
 src/box/sql/tarantoolInt.h |  1 -
 src/box/sql/vdbe.c         |  2 +-
 src/box/sql/vdbeaux.c      |  2 +-
 6 files changed, 32 insertions(+), 40 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index dd0cfcc1a..3138d9991 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -114,8 +114,6 @@ sql_get()
 
 /*********************************************************************
  * SQLite cursor implementation on top of Tarantool storage API-s.
- * See the corresponding SQLite function in btree.c for documentation.
- * Ex: sqlite3BtreeCloseCursor -> tarantoolSqlite3CloseCursor
  *
  * NB: SQLite btree cursor emulation is less than perfect. The problem
  * is that btree cursors are more low-level compared to Tarantool
@@ -184,18 +182,6 @@ is_tarantool_error(int rc)
 		rc == SQL_TARANTOOL_INSERT_FAIL);
 }
 
-int tarantoolSqlite3CloseCursor(BtCursor *pCur)
-{
-	assert(pCur->curFlags & BTCF_TaCursor ||
-	       pCur->curFlags & BTCF_TEphemCursor);
-
-	if (pCur->iter)
-		box_iterator_free(pCur->iter);
-	if (pCur->last_tuple)
-		box_tuple_unref(pCur->last_tuple);
-	return SQLITE_OK;
-}
-
 const void *tarantoolSqlite3PayloadFetch(BtCursor *pCur, u32 *pAmt)
 {
 	assert(pCur->curFlags & BTCF_TaCursor ||
@@ -467,6 +453,7 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
 	assert(pCur);
 	assert(pCur->curFlags & BTCF_TEphemCursor);
 	space_delete_ephemeral(pCur->space);
+	pCur->space = NULL;
 	return SQLITE_OK;
 }
 
diff --git a/src/box/sql/cursor.c b/src/box/sql/cursor.c
index 533bfb587..c38629f3b 100644
--- a/src/box/sql/cursor.c
+++ b/src/box/sql/cursor.c
@@ -31,18 +31,26 @@
 
 #include "sqliteInt.h"
 #include "tarantoolInt.h"
+#include "box/tuple.h"
 
-/*
- * Clear the current cursor position.
+/**
+ * Release tuple, free iterator, invalidate cursor's state.
+ * Note that this routine doesn't nullify space and index:
+ * it is also used during OP_NullRow opcode to refresh given
+ * cursor.
  */
 void
-sqlite3ClearCursor(BtCursor * pCur)
+sql_cursor_cleanup(struct BtCursor *cursor)
 {
-	free(pCur->key);
-	pCur->key = 0;
-	pCur->iter = NULL;
-	pCur->last_tuple = NULL;
-	pCur->eState = CURSOR_INVALID;
+	if (cursor->iter)
+		box_iterator_free(cursor->iter);
+	if (cursor->last_tuple)
+		tuple_unref(cursor->last_tuple);
+	free(cursor->key);
+	cursor->key = NULL;
+	cursor->iter = NULL;
+	cursor->last_tuple = NULL;
+	cursor->eState = CURSOR_INVALID;
 }
 
 /*
@@ -64,23 +72,19 @@ sqlite3CursorZero(BtCursor * p)
 	memset(p, 0, sizeof(*p));
 }
 
-/*
+/**
  * Close a cursor and invalidate its state. In case of
  * ephemeral cursor, corresponding space should be dropped.
  */
-int
-sqlite3CloseCursor(BtCursor * pCur)
+void
+sql_cursor_close(struct BtCursor *cursor)
 {
-	assert((pCur->curFlags & BTCF_TaCursor) ||
-	       (pCur->curFlags & BTCF_TEphemCursor));
-
-	if (pCur->curFlags & BTCF_TEphemCursor) {
-		tarantoolSqlite3EphemeralDrop(pCur);
-	}
-	tarantoolSqlite3CloseCursor(pCur);
-	sqlite3ClearCursor(pCur);
-
-	return SQLITE_OK;
+	assert(cursor->space != NULL);
+	assert((cursor->curFlags & BTCF_TaCursor) ||
+	       (cursor->curFlags & BTCF_TEphemCursor));
+	if (cursor->curFlags & BTCF_TEphemCursor)
+		tarantoolSqlite3EphemeralDrop(cursor);
+	sql_cursor_cleanup(cursor);
 }
 
 #ifndef NDEBUG			/* The next routine used only within assert() statements */
diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
index 5b8e7c29d..e5d2aae3a 100644
--- a/src/box/sql/cursor.h
+++ b/src/box/sql/cursor.h
@@ -72,14 +72,16 @@ struct BtCursor {
 void sqlite3CursorZero(BtCursor *);
 void sqlite3CursorHintFlags(BtCursor *, unsigned);
 
-int sqlite3CloseCursor(BtCursor *);
+void
+sql_cursor_close(struct BtCursor *cursor);
 int sqlite3CursorMovetoUnpacked(BtCursor *, UnpackedRecord * pUnKey, int *pRes);
 
 int sqlite3CursorNext(BtCursor *, int *pRes);
 int sqlite3CursorPrevious(BtCursor *, int *pRes);
 int sqlite3CursorPayload(BtCursor *, u32 offset, u32 amt, void *);
 
-void sqlite3ClearCursor(BtCursor *);
+void
+sql_cursor_cleanup(struct BtCursor *cursor);
 int sqlite3CursorHasHint(BtCursor *, unsigned int mask);
 
 #ifndef NDEBUG
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 19c6fdca9..ca31013ef 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -52,7 +52,6 @@ const char *tarantoolErrorMessage();
 int is_tarantool_error(int rc);
 
 /* Storage interface. */
-int tarantoolSqlite3CloseCursor(BtCursor * pCur);
 const void *tarantoolSqlite3PayloadFetch(BtCursor * pCur, u32 * pAmt);
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 8151e1557..0fddbcb66 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4061,7 +4061,7 @@ case OP_NullRow: {
 	pC->cacheStatus = CACHE_STALE;
 	if (pC->eCurType==CURTYPE_TARANTOOL) {
 		assert(pC->uc.pCursor!=0);
-		sqlite3ClearCursor(pC->uc.pCursor);
+		sql_cursor_cleanup(pC->uc.pCursor);
 	}
 	break;
 }
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index bb121a318..40572fc97 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2217,7 +2217,7 @@ sqlite3VdbeFreeCursor(Vdbe * p, VdbeCursor * pCx)
 		}
 	case CURTYPE_TARANTOOL:{
 		assert(pCx->uc.pCursor != 0);
-		sqlite3CloseCursor(pCx->uc.pCursor);
+		sql_cursor_close(pCx->uc.pCursor);
 			break;
 		}
 	}
-- 
2.15.1

[tarantool-patches] Re: [PATCH 1/2] sql: fix tuple format leak

[Vladislav Shpilevoy]

Hello. Thank you for contributing! See below 3 comments.

> diff --git a/src/box/sql.c b/src/box/sql.c
> index a6713f1f0..dd0cfcc1a 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -451,17 +451,13 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
>    *
>    * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
>    */
> -int tarantoolSqlite3EphemeralInsert(BtCursor *pCur)
> +int tarantoolSqlite3EphemeralInsert(struct space *space, char *tuple,
> +				    char *tuple_end)

1. Please, update the comment as well. And lets move it to a header, as 
it is done in tarantool core.

>   {
> -	assert(pCur);
> -	assert(pCur->curFlags & BTCF_TEphemCursor);
> -	mp_tuple_assert(pCur->key, pCur->key + pCur->nKey);
> -
> -	if (space_ephemeral_replace(pCur->space, pCur->key,
> -				    pCur->key + pCur->nKey) != 0) {
> -		diag_log();
> +	assert(space != NULL);
> +	mp_tuple_assert(tuple, tuple_end);
> +	if (space_ephemeral_replace(space, tuple, tuple_end) != 0)
>   		return SQL_TARANTOOL_INSERT_FAIL;
> -	}
>   	return SQLITE_OK;
>   }
>   
> @@ -475,28 +471,29 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
>   }
>   
>   static inline int
> -insertOrReplace(BtCursor *pCur, enum iproto_type type)
> +insertOrReplace(struct space *space, char *tuple, char *tuple_end,
> +		enum iproto_type type)

2. Please, make a pointer be const, if it is not changed. Here it is 
const. Const specifier helps compiler to do more accurate optimization.

> -int tarantoolSqlite3Insert(BtCursor *pCur)
> +int tarantoolSqlite3Insert(struct space *space, char *tuple, char *tuple_end)
>   {
> -	return insertOrReplace(pCur, IPROTO_INSERT);
> +	return insertOrReplace(space, tuple, tuple_end, IPROTO_INSERT);
>   }
>   
> -int tarantoolSqlite3Replace(BtCursor *pCur)
> +int tarantoolSqlite3Replace(struct space *space, char *tuple, char *tuple_end)

3. Same.

[tarantool-patches] Re: [PATCH 2/2] sql: refactor cursor closing routine

[Vladislav Shpilevoy]

See below a pair of comments.

> diff --git a/src/box/sql/cursor.c b/src/box/sql/cursor.c
> index 533bfb587..c38629f3b 100644
> --- a/src/box/sql/cursor.c
> +++ b/src/box/sql/cursor.c
> @@ -31,18 +31,26 @@
>   
>   #include "sqliteInt.h"
>   #include "tarantoolInt.h"
> +#include "box/tuple.h"
>   
> -/*
> - * Clear the current cursor position.
> +/**
> + * Release tuple, free iterator, invalidate cursor's state.
> + * Note that this routine doesn't nullify space and index:
> + * it is also used during OP_NullRow opcode to refresh given
> + * cursor.
>    */
>   void
> -sqlite3ClearCursor(BtCursor * pCur)
> +sql_cursor_cleanup(struct BtCursor *cursor)
>   {
> -	free(pCur->key);
> -	pCur->key = 0;
> -	pCur->iter = NULL;
> -	pCur->last_tuple = NULL;
> -	pCur->eState = CURSOR_INVALID;
> +	if (cursor->iter)
> +		box_iterator_free(cursor->iter);

1. If you use internal structures like struct iterator, then lets
manage them using internal API - iterator_delete(). I know, that
box_iterator_t is the same as struct iterator, but lets be
consistent.
> diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
> index 5b8e7c29d..e5d2aae3a 100644
> --- a/src/box/sql/cursor.h
> +++ b/src/box/sql/cursor.h
> @@ -72,14 +72,16 @@ struct BtCursor {
>   void sqlite3CursorZero(BtCursor *);
>   void sqlite3CursorHintFlags(BtCursor *, unsigned);
>   
> -int sqlite3CloseCursor(BtCursor *);
> +void
> +sql_cursor_close(struct BtCursor *cursor);

2. Lets store comments near to function declarations rather
than implementations.

[tarantool-patches] Re: [PATCH 1/2] sql: fix tuple format leak

[n.pettik]

> On 12 Apr 2018, at 14:58, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hello. Thank you for contributing! See below 3 comments.
> 
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index a6713f1f0..dd0cfcc1a 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -451,17 +451,13 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
>>   *
>>   * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
>>   */
>> -int tarantoolSqlite3EphemeralInsert(BtCursor *pCur)
>> +int tarantoolSqlite3EphemeralInsert(struct space *space, char *tuple,
>> +				    char *tuple_end)
> 
> 1. Please, update the comment as well. And lets move it to a header, as it is done in tarantool core.

Fixed:

@@ -441,18 +441,8 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
        return tarantoolSqlite3First(pCur, &unused);
 }
 
-/*
- * Insert tuple which is contained in pX into ephemeral space. In contrast to
- * ordinary spaces, there is no need to create and fill request or handle
- * transaction routine.
- *
- * @param pCur Cursor pointing to ephemeral space.
- * @param pX Payload containing tuple to insert.
- *
- * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
- */
+int tarantoolSqlite3EphemeralInsert(struct space *space, const char *tuple,
+                                   const char *tuple_end)

@@ -98,8 +100,19 @@ int tarantoolSqlite3RenameParentTable(int iTab, const char *zOldParentName,
 /* Interface for ephemeral tables. */
 int tarantoolSqlite3EphemeralCreate(BtCursor * pCur, uint32_t filed_count,
                                    struct coll *aColl);
-int tarantoolSqlite3EphemeralInsert(struct space *space, char *tuple,
-                                   char *tuple_end);
+/**
+ * Insert tuple into ephemeral space.
+ * In contrast to ordinary spaces, there is no need to create and
+ * fill request or handle transaction routine.
+ *
+ * @param space Ephemeral space.
+ * @param tuple Tuple to be inserted.
+ * @param tuple_end End of tuple to be inserted.
+ *
+ * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
+ */
+int tarantoolSqlite3EphemeralInsert(struct space *space, const char *tuple,
+                                   const char *tuple_end);

> 
>>  {
>> -	assert(pCur);
>> -	assert(pCur->curFlags & BTCF_TEphemCursor);
>> -	mp_tuple_assert(pCur->key, pCur->key + pCur->nKey);
>> -
>> -	if (space_ephemeral_replace(pCur->space, pCur->key,
>> -				    pCur->key + pCur->nKey) != 0) {
>> -		diag_log();
>> +	assert(space != NULL);
>> +	mp_tuple_assert(tuple, tuple_end);
>> +	if (space_ephemeral_replace(space, tuple, tuple_end) != 0)
>>  		return SQL_TARANTOOL_INSERT_FAIL;
>> -	}
>>  	return SQLITE_OK;
>>  }
>>  @@ -475,28 +471,29 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
>>  }
>>    static inline int
>> -insertOrReplace(BtCursor *pCur, enum iproto_type type)
>> +insertOrReplace(struct space *space, char *tuple, char *tuple_end,
>> +		enum iproto_type type)
> 
> 2. Please, make a pointer be const, if it is not changed. Here it is const. Const specifier helps compiler to do more accurate optimization.

Fixed:

@@ -471,7 +461,7 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
 }
 
 static inline int
-insertOrReplace(struct space *space, char *tuple, char *tuple_end,
+insertOrReplace(struct space *space, const char *tuple, const char *tuple_end,

> 
>> -int tarantoolSqlite3Insert(BtCursor *pCur)
>> +int tarantoolSqlite3Insert(struct space *space, char *tuple, char *tuple_end)
>>  {
>> -	return insertOrReplace(pCur, IPROTO_INSERT);
>> +	return insertOrReplace(space, tuple, tuple_end, IPROTO_INSERT);
>>  }
>>  -int tarantoolSqlite3Replace(BtCursor *pCur)
>> +int tarantoolSqlite3Replace(struct space *space, char *tuple, char *tuple_end)
> 
> 3. Same.

-int tarantoolSqlite3Replace(struct space *space, char *tuple, char *tuple_end)
+int tarantoolSqlite3Replace(struct space *space, const char *tuple,
+                           const char *tuple_end)

-int tarantoolSqlite3Insert(struct space *space, char *tuple, char *tuple_end)
+int tarantoolSqlite3Insert(struct space *space, const char *tuple,
+                          const char *tuple_end)

src/box/sql/tarantoolInt.h:

-int tarantoolSqlite3Insert(struct space *space, char *tuple, char *tuple_end);
-int tarantoolSqlite3Replace((struct space *space, char *tuple, char *tuple_end);
+int tarantoolSqlite3Insert(struct space *space, const char *tuple,
+			   const char *tuple_end);
+int tarantoolSqlite3Replace(struct space *space, const char *tuple,
+			    const char *tuple_end);