Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute
@ 2018-11-17 14:03 imeevma
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 01/10] box: store sql text and length in sql_request imeevma
                   ` (10 more replies)
  0 siblings, 11 replies; 18+ messages in thread
From: imeevma @ 2018-11-17 14:03 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

The goal of this patch-set is to make functions from execute.c
the only way to execute SQL statements. This goal includes
identical output for executed SQL statements no matter how they
were executed: through net.box or through box.

This version of patch-set is not complete. It doesn't have last
part, which is replacing box.sql.execute by box.execute, because
it will lead to massive test editing.

The main goal of this version of patch-set is to get comments
about design of vstream and general comments about this patch-set.

A bit about patches in this patch-set:

Patch 1 allows to use SQL query as plain text in sql_request.

Patch 2 makes execute.c a bit more universal by removing
IPROTO functions from there.

Patch 3 allows us to design vstream by wrapping mpstream
functions.

Patch 4 creates interface vstream and its mpstream implementation.

Patch 5 fixes bug with EXPLAIN being executed through net.box
throws SEGMENTATION FAULT.

Patch 6 fixes bug with SELECT from system spaces returns some
columns as unpacked msgpack.

Patch 7 fixes bug with region being cleared twice during execution
of VDBE.

Patch 8 creates method for port which will allow us to dump port
directly to Lua stack.

Patch 9 creates vstream implementation for Lua and defines new
box.sql.new_execute() function that will become box.execute() in
next vesions.

Patch 10 is created to check that box.sql.new_execute() is able to
pass through test created for box.sql.execute(). Created new
implementation of box.sql.execute() that transforms output of
box.sql.new_execute() to format of old box.sql.execute().

Kirill Shcherbatov (1):
  box: store sql text and length in sql_request

Mergen Imeev (9):
  iproto: remove iproto functions from execute.c
  iproto: replace obuf by mpstream in execute.c
  sql: create interface vstream
  sql: EXPLAIN through net.box leads to SEGFAULT
  sql: SELECT from system spaces returns unpacked msgpack.
  sql: too many autogenerated ids leads to SEGFAULT
  box: add method dump_lua to port
  lua: create vstream implementation for Lua
  sql: check new box.sql.execute()

 src/box/CMakeLists.txt   |   1 +
 src/box/execute.c        | 218 ++++++++++-------------------
 src/box/execute.h        |  43 +++---
 src/box/iproto.cc        |  93 ++++++++++++-
 src/box/lua/call.c       |   1 +
 src/box/lua/net_box.c    |  15 +-
 src/box/lua/schema.lua   |  23 ++++
 src/box/lua/sql.c        | 107 +++------------
 src/box/port.c           |  22 +++
 src/box/port.h           |  12 ++
 src/box/sql/vdbe.c       |   8 +-
 src/box/sql/vdbeaux.c    |   6 -
 src/box/vstream.c        | 348 +++++++++++++++++++++++++++++++++++++++++++++++
 src/box/vstream.h        | 194 ++++++++++++++++++++++++++
 src/box/xrow.c           |  39 ------
 src/box/xrow.h           |  14 --
 test/sql/iproto.result   |  36 +++++
 test/sql/iproto.test.lua |  20 ++-
 18 files changed, 870 insertions(+), 330 deletions(-)
 create mode 100644 src/box/vstream.c
 create mode 100644 src/box/vstream.h

-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 01/10] box: store sql text and length in sql_request
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
@ 2018-11-17 14:03 ` imeevma
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 02/10] iproto: remove iproto functions from execute.c imeevma
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: imeevma @ 2018-11-17 14:03 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

Refactored sql_request structure to store pointer to sql string
data and it's length instead of pointer to msgpack
representation.
This is required to use this structure in sql.c where the query
has a different semantics and can be obtained from stack as a C
string.

Needed for #3505.
---
 src/box/execute.c | 6 ++++--
 src/box/execute.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index e450444..ce9dd83 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -285,6 +285,9 @@ error:
 				return -1;
 		} else {
 			request->sql_text = value;
+			request->sql_text =
+				mp_decode_str(&request->sql_text,
+					      &request->sql_text_len);
 		}
 	}
 	if (request->sql_text == NULL) {
@@ -586,8 +589,7 @@ sql_prepare_and_execute(const struct sql_request *request,
 			struct sql_response *response, struct region *region)
 {
 	const char *sql = request->sql_text;
-	uint32_t len;
-	sql = mp_decode_str(&sql, &len);
+	uint32_t len = request->sql_text_len;
 	struct sqlite3_stmt *stmt;
 	sqlite3 *db = sql_get();
 	if (db == NULL) {
diff --git a/src/box/execute.h b/src/box/execute.h
index 77bfd79..79cee69 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -58,6 +58,8 @@ struct sql_request {
 	uint64_t sync;
 	/** SQL statement text. */
 	const char *sql_text;
+	/** Length of the SQL statement text. */
+	uint32_t sql_text_len;
 	/** Array of parameters. */
 	struct sql_bind *bind;
 	/** Length of the @bind. */
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 02/10] iproto: remove iproto functions from execute.c
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 01/10] box: store sql text and length in sql_request imeevma
@ 2018-11-17 14:03 ` imeevma
  2018-11-19 17:58   ` [tarantool-patches] " Vladislav Shpilevoy
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 03/10] iproto: replace obuf by mpstream in execute.c imeevma
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2018-11-17 14:03 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

To make functions in execute.h more universal we should reduce
their dependence on IPROTO. This patch removes IPROTO functions
from execute.c.

Needed for #3505
---
 src/box/execute.c | 116 ++++++++++++++----------------------------------------
 src/box/execute.h |  34 ++++++++--------
 src/box/iproto.cc |  71 ++++++++++++++++++++++++++++++++-
 src/box/xrow.c    |  39 ------------------
 src/box/xrow.h    |  14 -------
 5 files changed, 115 insertions(+), 159 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index ce9dd83..285ae2e 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -194,23 +194,7 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet)
 	return 0;
 }
 
-/**
- * Parse MessagePack array of SQL parameters and store a result
- * into the @request->bind, bind_count.
- * @param request Request to save decoded parameters.
- * @param data MessagePack array of parameters. Each parameter
- *        either must have scalar type, or must be a map with the
- *        following format: {name: value}. Name - string name of
- *        the named parameter, value - scalar value of the
- *        parameter. Named and positioned parameters can be mixed.
- *        For more details
- *        @sa https://www.sqlite.org/lang_expr.html#varparam.
- * @param region Allocator.
- *
- * @retval  0 Success.
- * @retval -1 Client or memory error.
- */
-static inline int
+int
 sql_bind_list_decode(struct sql_request *request, const char *data,
 		     struct region *region)
 {
@@ -246,60 +230,6 @@ sql_bind_list_decode(struct sql_request *request, const char *data,
 	return 0;
 }
 
-int
-xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
-		struct region *region)
-{
-	if (row->bodycnt == 0) {
-		diag_set(ClientError, ER_INVALID_MSGPACK, "missing request body");
-		return 1;
-	}
-	assert(row->bodycnt == 1);
-	const char *data = (const char *) row->body[0].iov_base;
-	const char *end = data + row->body[0].iov_len;
-	assert((end - data) > 0);
-
-	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
-error:
-		diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
-		return -1;
-	}
-
-	uint32_t map_size = mp_decode_map(&data);
-	request->sql_text = NULL;
-	request->bind = NULL;
-	request->bind_count = 0;
-	request->sync = row->sync;
-	for (uint32_t i = 0; i < map_size; ++i) {
-		uint8_t key = *data;
-		if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) {
-			mp_check(&data, end);   /* skip the key */
-			mp_check(&data, end);   /* skip the value */
-			continue;
-		}
-		const char *value = ++data;     /* skip the key */
-		if (mp_check(&data, end) != 0)  /* check the value */
-			goto error;
-		if (key == IPROTO_SQL_BIND) {
-			if (sql_bind_list_decode(request, value, region) != 0)
-				return -1;
-		} else {
-			request->sql_text = value;
-			request->sql_text =
-				mp_decode_str(&request->sql_text,
-					      &request->sql_text_len);
-		}
-	}
-	if (request->sql_text == NULL) {
-		diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
-			 iproto_key_name(IPROTO_SQL_TEXT));
-		return -1;
-	}
-	if (data != end)
-		goto error;
-	return 0;
-}
-
 /**
  * Serialize a single column of a result set row.
  * @param stmt Prepared and started statement. At least one
@@ -528,9 +458,15 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
 		    int column_count)
 {
 	assert(column_count > 0);
-	if (iproto_reply_array_key(out, column_count, IPROTO_METADATA) != 0)
+	char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
+	if (pos == NULL) {
+		diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, "obuf_alloc",
+			 "pos");
 		return -1;
-
+	}
+	pos = mp_store_u8(pos, IPROTO_METADATA);
+	pos = mp_store_u8(pos, 0xdd);
+	pos = mp_store_u32(pos, column_count);
 	for (int i = 0; i < column_count; ++i) {
 		size_t size = mp_sizeof_map(2) +
 			      mp_sizeof_uint(IPROTO_FIELD_NAME) +
@@ -613,27 +549,28 @@ sql_prepare_and_execute(const struct sql_request *request,
 }
 
 int
-sql_response_dump(struct sql_response *response, struct obuf *out)
+sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
 {
-	struct obuf_svp header_svp;
-	/* Prepare memory for the iproto header. */
-	if (iproto_prepare_header(out, &header_svp, IPROTO_SQL_HEADER_LEN) != 0)
-		return -1;
 	sqlite3 *db = sql_get();
 	struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
 	struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
-	int keys, rc = 0, column_count = sqlite3_column_count(stmt);
+	int rc = 0, column_count = sqlite3_column_count(stmt);
 	if (column_count > 0) {
 		if (sql_get_description(stmt, out, column_count) != 0) {
 err:
-			obuf_rollback_to_svp(out, &header_svp);
 			rc = -1;
 			goto finish;
 		}
-		keys = 2;
-		if (iproto_reply_array_key(out, port_tuple->size,
-					   IPROTO_DATA) != 0)
+		*keys = 2;
+		char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
+		if (pos == NULL) {
+			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
+				 "obuf_alloc", "pos");
 			goto err;
+		}
+		pos = mp_store_u8(pos, IPROTO_DATA);
+		pos = mp_store_u8(pos, 0xdd);
+		pos = mp_store_u32(pos, port_tuple->size);
 		/*
 		 * Just like SELECT, SQL uses output format compatible
 		 * with Tarantool 1.6
@@ -643,13 +580,20 @@ err:
 			goto err;
 		}
 	} else {
-		keys = 1;
+		*keys = 1;
 		assert(port_tuple->size == 0);
 		struct stailq *autoinc_id_list =
 			vdbe_autoinc_id_list((struct Vdbe *)stmt);
 		uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
-		if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
+		char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
+		if (pos == NULL) {
+			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
+				 "obuf_alloc", "pos");
 			goto err;
+		}
+		pos = mp_store_u8(pos, IPROTO_SQL_INFO);
+		pos = mp_store_u8(pos, 0xdf);
+		pos = mp_store_u32(pos, map_size);
 		uint64_t id_count = 0;
 		int changes = db->nChange;
 		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
@@ -683,8 +627,6 @@ err:
 			}
 		}
 	}
-	iproto_reply_sql(out, &header_svp, response->sync, schema_version,
-			 keys);
 finish:
 	port_destroy(&response->port);
 	sqlite3_finalize(stmt);
diff --git a/src/box/execute.h b/src/box/execute.h
index 79cee69..2a242e8 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -104,26 +104,34 @@ struct sql_response {
  * | }                                            |
  * +----------------------------------------------+
  * @param response EXECUTE response.
+ * @param[out] keys number of keys in dumped map.
  * @param out Output buffer.
  *
  * @retval  0 Success.
  * @retval -1 Memory error.
  */
 int
-sql_response_dump(struct sql_response *response, struct obuf *out);
+sql_response_dump(struct sql_response *response, int *keys, struct obuf *out);
 
 /**
- * Parse the EXECUTE request.
- * @param row Encoded data.
- * @param[out] request Request to decode to.
+ * Parse MessagePack array of SQL parameters and store a result
+ * into the @request->bind, bind_count.
+ * @param request Request to save decoded parameters.
+ * @param data MessagePack array of parameters. Each parameter
+ *        either must have scalar type, or must be a map with the
+ *        following format: {name: value}. Name - string name of
+ *        the named parameter, value - scalar value of the
+ *        parameter. Named and positioned parameters can be mixed.
+ *        For more details
+ *        @sa https://www.sqlite.org/lang_expr.html#varparam.
  * @param region Allocator.
  *
- * @retval  0 Sucess.
- * @retval -1 Format or memory error.
+ * @retval  0 Success.
+ * @retval -1 Client or memory error.
  */
 int
-xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
-		struct region *region);
+sql_bind_list_decode(struct sql_request *request, const char *data,
+		     struct region *region);
 
 /**
  * Prepare and execute an SQL statement.
@@ -141,16 +149,6 @@ sql_prepare_and_execute(const struct sql_request *request,
 
 #if defined(__cplusplus)
 } /* extern "C" { */
-#include "diag.h"
-
-/** @copydoc sql_request_decode. Throws on error. */
-static inline void
-xrow_decode_sql_xc(const struct xrow_header *row, struct sql_request *request,
-		   struct region *region)
-{
-	if (xrow_decode_sql(row, request, region) != 0)
-		diag_raise();
-}
 #endif
 
 #endif /* TARANTOOL_SQL_EXECUTE_H_INCLUDED */
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index cd61393..5fb2aff 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1118,6 +1118,67 @@ static const struct cmsg_hop error_route[] = {
 	{ net_send_error, NULL },
 };
 
+/**
+ * Parse the EXECUTE request.
+ * @param row Encoded data.
+ * @param[out] request Request to decode to.
+ * @param region Allocator.
+ *
+ * @retval  0 Sucess.
+ * @retval -1 Format or memory error.
+ */
+static inline int
+xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
+		struct region *region)
+{
+	if (row->bodycnt == 0) {
+		diag_set(ClientError, ER_INVALID_MSGPACK, "missing request body");
+		return 1;
+	}
+	assert(row->bodycnt == 1);
+	const char *data = (const char *) row->body[0].iov_base;
+	const char *end = data + row->body[0].iov_len;
+	assert((end - data) > 0);
+
+	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
+error:
+		diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
+		return -1;
+	}
+
+	uint32_t map_size = mp_decode_map(&data);
+	request->sql_text = NULL;
+	request->bind = NULL;
+	request->bind_count = 0;
+	request->sync = row->sync;
+	for (uint32_t i = 0; i < map_size; ++i) {
+		uint8_t key = *data;
+		if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) {
+			mp_check(&data, end);   /* skip the key */
+			mp_check(&data, end);   /* skip the value */
+			continue;
+		}
+		const char *value = ++data;     /* skip the key */
+		if (mp_check(&data, end) != 0)  /* check the value */
+			goto error;
+		if (key == IPROTO_SQL_BIND) {
+			if (sql_bind_list_decode(request, value, region) != 0)
+				return -1;
+		} else {
+			request->sql_text =
+				mp_decode_str(&value, &request->sql_text_len);
+		}
+	}
+	if (request->sql_text == NULL) {
+		diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
+			 iproto_key_name(IPROTO_SQL_TEXT));
+		return -1;
+	}
+	if (data != end)
+		goto error;
+	return 0;
+}
+
 static void
 iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend,
 		  bool *stop_input)
@@ -1593,8 +1654,16 @@ tx_process_sql(struct cmsg *m)
 	 * become out of date during yield.
 	 */
 	out = msg->connection->tx.p_obuf;
-	if (sql_response_dump(&response, out) != 0)
+	int keys;
+	struct obuf_svp header_svp;
+	/* Prepare memory for the iproto header. */
+	if (iproto_prepare_header(out, &header_svp, IPROTO_SQL_HEADER_LEN) != 0)
+		goto error;
+	if (sql_response_dump(&response, &keys, out) != 0) {
+		obuf_rollback_to_svp(out, &header_svp);
 		goto error;
+	}
+	iproto_reply_sql(out, &header_svp, response.sync, schema_version, keys);
 	iproto_wpos_create(&msg->wpos, out);
 	return;
 error:
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 567b7ed..8a589cb 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -451,45 +451,6 @@ struct PACKED iproto_key_bin {
 	uint32_t mp_len;
 };
 
-/**
- * Write the key to the buffer by the specified savepoint.
- * @param buf Output buffer.
- * @param type Value type (MP_ARRAY32=0xdd, MP_MAP32=0xdf, ...).
- * @param size Value size (array or map length).
- * @param key Key value (IPROTO_DATA/DESCRIPTION/SQL_INFO).
- *
- * @retval  0 Success.
- * @retval -1 Memory error.
- */
-static inline int
-iproto_reply_key(struct obuf *buf, uint8_t type, uint32_t size, uint8_t key)
-{
-	char *pos = (char *) obuf_alloc(buf, IPROTO_KEY_HEADER_LEN);
-	if (pos == NULL) {
-		diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, "obuf_alloc",
-			 "pos");
-		return -1;
-	}
-	struct iproto_key_bin bin;
-	bin.key = key;
-	bin.mp_type = type;
-	bin.mp_len = mp_bswap_u32(size);
-	memcpy(pos, &bin, sizeof(bin));
-	return 0;
-}
-
-int
-iproto_reply_array_key(struct obuf *buf, uint32_t size, uint8_t key)
-{
-	return iproto_reply_key(buf, 0xdd, size, key);
-}
-
-int
-iproto_reply_map_key(struct obuf *buf, uint32_t size, uint8_t key)
-{
-	return iproto_reply_key(buf, 0xdf, size, key);
-}
-
 void
 iproto_reply_select(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
 		    uint32_t schema_version, uint32_t count)
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 881d860..13071ff 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -423,20 +423,6 @@ iproto_reply_select(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
 		    uint32_t schema_version, uint32_t count);
 
 /**
- * Write header of the key to a preallocated buffer by svp.
- * @param buf Buffer to write to.
- * @param size Size of the key (length of the array or of the
- *        string).
- * @param key Body key.
- */
-int
-iproto_reply_array_key(struct obuf *buf, uint32_t size, uint8_t key);
-
-/** @copydoc iproto_reply_array_key. */
-int
-iproto_reply_map_key(struct obuf *buf, uint32_t size, uint8_t key);
-
-/**
  * Encode iproto header with IPROTO_OK response code.
  * @param out Encode to.
  * @param sync Request sync.
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 03/10] iproto: replace obuf by mpstream in execute.c
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 01/10] box: store sql text and length in sql_request imeevma
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 02/10] iproto: remove iproto functions from execute.c imeevma
@ 2018-11-17 14:03 ` imeevma
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 04/10] sql: create interface vstream imeevma
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: imeevma @ 2018-11-17 14:03 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

It is useful to replace obuf by mpstream. It allows us to design
vstream. Interface vstream will be used as universal interface to
dump result of SQL queries.

Needed for #3505
---
 src/box/execute.c | 92 +++++++++++++++++++++++--------------------------------
 src/box/execute.h |  6 ++--
 src/box/iproto.cc | 16 +++++++++-
 3 files changed, 58 insertions(+), 56 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 285ae2e..5c2ec19 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -35,7 +35,6 @@
 #include "sql/sqliteLimit.h"
 #include "errcode.h"
 #include "small/region.h"
-#include "small/obuf.h"
 #include "diag.h"
 #include "sql.h"
 #include "xrow.h"
@@ -454,23 +453,21 @@ sql_bind(const struct sql_request *request, struct sqlite3_stmt *stmt)
  * @retval -1 Client or memory error.
  */
 static inline int
-sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
+sql_get_description(struct sqlite3_stmt *stmt, struct mpstream *stream,
 		    int column_count)
 {
 	assert(column_count > 0);
-	char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
+	char *pos = mpstream_reserve(stream, IPROTO_KEY_HEADER_LEN);
 	if (pos == NULL) {
-		diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, "obuf_alloc",
+		diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, "mpstream_reserve",
 			 "pos");
 		return -1;
 	}
 	pos = mp_store_u8(pos, IPROTO_METADATA);
 	pos = mp_store_u8(pos, 0xdd);
 	pos = mp_store_u32(pos, column_count);
+	mpstream_advance(stream, IPROTO_KEY_HEADER_LEN);
 	for (int i = 0; i < column_count; ++i) {
-		size_t size = mp_sizeof_map(2) +
-			      mp_sizeof_uint(IPROTO_FIELD_NAME) +
-			      mp_sizeof_uint(IPROTO_FIELD_TYPE);
 		const char *name = sqlite3_column_name(stmt, i);
 		const char *type = sqlite3_column_datatype(stmt, i);
 		/*
@@ -479,18 +476,11 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
 		 * column_name simply returns them.
 		 */
 		assert(name != NULL);
-		size += mp_sizeof_str(strlen(name));
-		size += mp_sizeof_str(strlen(type));
-		char *pos = (char *) obuf_alloc(out, size);
-		if (pos == NULL) {
-			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
-			return -1;
-		}
-		pos = mp_encode_map(pos, 2);
-		pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
-		pos = mp_encode_str(pos, name, strlen(name));
-		pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
-		pos = mp_encode_str(pos, type, strlen(type));
+		mpstream_encode_map(stream, 2);
+		mpstream_encode_uint(stream, IPROTO_FIELD_NAME);
+		mpstream_encode_str(stream, name);
+		mpstream_encode_uint(stream, IPROTO_FIELD_TYPE);
+		mpstream_encode_str(stream, type);
 	}
 	return 0;
 }
@@ -549,81 +539,77 @@ sql_prepare_and_execute(const struct sql_request *request,
 }
 
 int
-sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
+sql_response_dump(struct sql_response *response, int *keys,
+		  struct mpstream *stream)
 {
 	sqlite3 *db = sql_get();
 	struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
 	struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
 	int rc = 0, column_count = sqlite3_column_count(stmt);
 	if (column_count > 0) {
-		if (sql_get_description(stmt, out, column_count) != 0) {
+		if (sql_get_description(stmt, stream, column_count) != 0) {
 err:
 			rc = -1;
 			goto finish;
 		}
 		*keys = 2;
-		char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
+		char *pos = mpstream_reserve(stream, IPROTO_KEY_HEADER_LEN);
 		if (pos == NULL) {
 			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
-				 "obuf_alloc", "pos");
+				 "mpstream_reserve", "pos");
 			goto err;
 		}
 		pos = mp_store_u8(pos, IPROTO_DATA);
 		pos = mp_store_u8(pos, 0xdd);
 		pos = mp_store_u32(pos, port_tuple->size);
+		mpstream_advance(stream, IPROTO_KEY_HEADER_LEN);
+
+		mpstream_flush(stream);
 		/*
 		 * Just like SELECT, SQL uses output format compatible
 		 * with Tarantool 1.6
 		 */
-		if (port_dump_msgpack_16(&response->port, out) < 0) {
+		if (port_dump_msgpack_16(&response->port, stream->ctx) < 0) {
 			/* Failed port dump destroyes the port. */
 			goto err;
 		}
+		mpstream_reset(stream);
 	} else {
 		*keys = 1;
 		assert(port_tuple->size == 0);
 		struct stailq *autoinc_id_list =
 			vdbe_autoinc_id_list((struct Vdbe *)stmt);
 		uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
-		char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
-		if (pos == NULL) {
-			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
-				 "obuf_alloc", "pos");
-			goto err;
-		}
-		pos = mp_store_u8(pos, IPROTO_SQL_INFO);
-		pos = mp_store_u8(pos, 0xdf);
-		pos = mp_store_u32(pos, map_size);
 		uint64_t id_count = 0;
-		int changes = db->nChange;
-		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
-			   mp_sizeof_uint(changes);
 		if (!stailq_empty(autoinc_id_list)) {
 			struct autoinc_id_entry *id_entry;
-			stailq_foreach_entry(id_entry, autoinc_id_list, link) {
-				size += id_entry->id >= 0 ?
-					mp_sizeof_uint(id_entry->id) :
-					mp_sizeof_int(id_entry->id);
+			stailq_foreach_entry(id_entry, autoinc_id_list, link)
 				id_count++;
-			}
-			size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) +
-				mp_sizeof_array(id_count);
 		}
-		char *buf = obuf_alloc(out, size);
-		if (buf == NULL) {
-			diag_set(OutOfMemory, size, "obuf_alloc", "buf");
+		char *pos = mpstream_reserve(stream, IPROTO_KEY_HEADER_LEN);
+		if (pos == NULL) {
+			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
+				 "mpstream_reserve", "pos");
 			goto err;
 		}
-		buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
-		buf = mp_encode_uint(buf, changes);
+		pos = mp_store_u8(pos, IPROTO_SQL_INFO);
+		pos = mp_store_u8(pos, 0xdf);
+		pos = mp_store_u32(pos, map_size);
+		mpstream_advance(stream, IPROTO_KEY_HEADER_LEN);
+
+		mpstream_encode_uint(stream, SQL_INFO_ROW_COUNT);
+		mpstream_encode_uint(stream, db->nChange);
 		if (!stailq_empty(autoinc_id_list)) {
-			buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS);
-			buf = mp_encode_array(buf, id_count);
+			mpstream_encode_uint(stream,
+					     SQL_INFO_AUTOINCREMENT_IDS);
+			mpstream_encode_array(stream, id_count);
 			struct autoinc_id_entry *id_entry;
 			stailq_foreach_entry(id_entry, autoinc_id_list, link) {
-				buf = id_entry->id >= 0 ?
-				      mp_encode_uint(buf, id_entry->id) :
-				      mp_encode_int(buf, id_entry->id);
+				int64_t value = id_entry->id;
+				if (id_entry->id >= 0)
+					mpstream_encode_uint(stream, value);
+				else
+					mpstream_encode_int(stream, value);
 			}
 		}
 	}
diff --git a/src/box/execute.h b/src/box/execute.h
index 2a242e8..7eb2121 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -34,6 +34,7 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include "port.h"
+#include "mpstream.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -105,13 +106,14 @@ struct sql_response {
  * +----------------------------------------------+
  * @param response EXECUTE response.
  * @param[out] keys number of keys in dumped map.
- * @param out Output buffer.
+ * @param stream stream to where result is written.
  *
  * @retval  0 Success.
  * @retval -1 Memory error.
  */
 int
-sql_response_dump(struct sql_response *response, int *keys, struct obuf *out);
+sql_response_dump(struct sql_response *response, int *keys,
+		  struct mpstream *stream);
 
 /**
  * Parse MessagePack array of SQL parameters and store a result
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 5fb2aff..83b268b 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1634,12 +1634,20 @@ error:
 	tx_reply_error(msg);
 }
 
+/** Callback to forward and error from mpstream methods. */
+static void
+set_encode_error(void *error_ctx)
+{
+	*(bool *)error_ctx = true;
+}
+
 static void
 tx_process_sql(struct cmsg *m)
 {
 	struct iproto_msg *msg = tx_accept_msg(m);
 	struct obuf *out;
 	struct sql_response response;
+	bool is_error = false;
 
 	tx_fiber_init(msg->connection->session, msg->header.sync);
 
@@ -1659,10 +1667,16 @@ tx_process_sql(struct cmsg *m)
 	/* Prepare memory for the iproto header. */
 	if (iproto_prepare_header(out, &header_svp, IPROTO_SQL_HEADER_LEN) != 0)
 		goto error;
-	if (sql_response_dump(&response, &keys, out) != 0) {
+	struct mpstream stream;
+	mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
+		      set_encode_error, &is_error);
+	if (is_error)
+		goto error;
+	if (sql_response_dump(&response, &keys, &stream) != 0) {
 		obuf_rollback_to_svp(out, &header_svp);
 		goto error;
 	}
+	mpstream_flush(&stream);
 	iproto_reply_sql(out, &header_svp, response.sync, schema_version, keys);
 	iproto_wpos_create(&msg->wpos, out);
 	return;
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 04/10] sql: create interface vstream
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
                   ` (2 preceding siblings ...)
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 03/10] iproto: replace obuf by mpstream in execute.c imeevma
@ 2018-11-17 14:03 ` imeevma
  2018-11-19 17:58   ` [tarantool-patches] " Vladislav Shpilevoy
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 05/10] sql: EXPLAIN through net.box leads to SEGFAULT imeevma
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2018-11-17 14:03 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

If we want to use functions from execute.h not only in IPROTO we
should create special interface. This interface will allow us to
create different implementations for mpstream and lua_State and
use functions from execute.c without changing them. This patch
creates such interface and its implementation for mpstream and
replaces mpstream functions in execute.c by methods of this
interface.

Needed for #3505
---
 src/box/CMakeLists.txt |   1 +
 src/box/execute.c      |  92 +++++++++--------------
 src/box/execute.h      |   4 +-
 src/box/iproto.cc      |  13 +++-
 src/box/vstream.c      | 200 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/box/vstream.h      | 191 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 439 insertions(+), 62 deletions(-)
 create mode 100644 src/box/vstream.c
 create mode 100644 src/box/vstream.h

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index d127647..09cb245 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -56,6 +56,7 @@ add_library(xlog STATIC xlog.c)
 target_link_libraries(xlog core box_error crc32 ${ZSTD_LIBRARIES})
 
 add_library(box STATIC
+    vstream.c
     iproto.cc
     error.cc
     xrow_io.cc
diff --git a/src/box/execute.c b/src/box/execute.c
index 5c2ec19..80ad655 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -37,11 +37,11 @@
 #include "small/region.h"
 #include "diag.h"
 #include "sql.h"
-#include "xrow.h"
 #include "schema.h"
 #include "port.h"
 #include "tuple.h"
 #include "sql/vdbe.h"
+#include "vstream.h"
 
 const char *sql_type_strs[] = {
 	NULL,
@@ -453,20 +453,12 @@ sql_bind(const struct sql_request *request, struct sqlite3_stmt *stmt)
  * @retval -1 Client or memory error.
  */
 static inline int
-sql_get_description(struct sqlite3_stmt *stmt, struct mpstream *stream,
+sql_get_description(struct sqlite3_stmt *stmt, struct vstream *stream,
 		    int column_count)
 {
 	assert(column_count > 0);
-	char *pos = mpstream_reserve(stream, IPROTO_KEY_HEADER_LEN);
-	if (pos == NULL) {
-		diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, "mpstream_reserve",
-			 "pos");
-		return -1;
-	}
-	pos = mp_store_u8(pos, IPROTO_METADATA);
-	pos = mp_store_u8(pos, 0xdd);
-	pos = mp_store_u32(pos, column_count);
-	mpstream_advance(stream, IPROTO_KEY_HEADER_LEN);
+	vstream_encode_reply_array(stream, column_count, IPROTO_METADATA,
+				   "metadata");
 	for (int i = 0; i < column_count; ++i) {
 		const char *name = sqlite3_column_name(stmt, i);
 		const char *type = sqlite3_column_datatype(stmt, i);
@@ -476,12 +468,19 @@ sql_get_description(struct sqlite3_stmt *stmt, struct mpstream *stream,
 		 * column_name simply returns them.
 		 */
 		assert(name != NULL);
-		mpstream_encode_map(stream, 2);
-		mpstream_encode_uint(stream, IPROTO_FIELD_NAME);
-		mpstream_encode_str(stream, name);
-		mpstream_encode_uint(stream, IPROTO_FIELD_TYPE);
-		mpstream_encode_str(stream, type);
+		vstream_encode_map(stream, 2);
+
+		vstream_encode_enum(stream, IPROTO_FIELD_NAME, "name");
+		vstream_encode_str(stream, name);
+		vstream_encode_map_commit(stream);
+
+		vstream_encode_enum(stream, IPROTO_FIELD_TYPE, "type");
+		vstream_encode_str(stream, type);
+		vstream_encode_map_commit(stream);
+
+		vstream_encode_array_commit(stream, i);
 	}
+	vstream_encode_reply_commit(stream);
 	return 0;
 }
 
@@ -540,7 +539,7 @@ sql_prepare_and_execute(const struct sql_request *request,
 
 int
 sql_response_dump(struct sql_response *response, int *keys,
-		  struct mpstream *stream)
+		  struct vstream *stream)
 {
 	sqlite3 *db = sql_get();
 	struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
@@ -553,27 +552,11 @@ err:
 			goto finish;
 		}
 		*keys = 2;
-		char *pos = mpstream_reserve(stream, IPROTO_KEY_HEADER_LEN);
-		if (pos == NULL) {
-			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
-				 "mpstream_reserve", "pos");
+		vstream_encode_reply_array(stream, port_tuple->size,
+					   IPROTO_DATA, "rows");
+		if (vstream_encode_port(stream, &response->port) < 0)
 			goto err;
-		}
-		pos = mp_store_u8(pos, IPROTO_DATA);
-		pos = mp_store_u8(pos, 0xdd);
-		pos = mp_store_u32(pos, port_tuple->size);
-		mpstream_advance(stream, IPROTO_KEY_HEADER_LEN);
-
-		mpstream_flush(stream);
-		/*
-		 * Just like SELECT, SQL uses output format compatible
-		 * with Tarantool 1.6
-		 */
-		if (port_dump_msgpack_16(&response->port, stream->ctx) < 0) {
-			/* Failed port dump destroyes the port. */
-			goto err;
-		}
-		mpstream_reset(stream);
+		vstream_encode_reply_commit(stream);
 	} else {
 		*keys = 1;
 		assert(port_tuple->size == 0);
@@ -586,32 +569,29 @@ err:
 			stailq_foreach_entry(id_entry, autoinc_id_list, link)
 				id_count++;
 		}
-		char *pos = mpstream_reserve(stream, IPROTO_KEY_HEADER_LEN);
-		if (pos == NULL) {
-			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
-				 "mpstream_reserve", "pos");
-			goto err;
-		}
-		pos = mp_store_u8(pos, IPROTO_SQL_INFO);
-		pos = mp_store_u8(pos, 0xdf);
-		pos = mp_store_u32(pos, map_size);
-		mpstream_advance(stream, IPROTO_KEY_HEADER_LEN);
-
-		mpstream_encode_uint(stream, SQL_INFO_ROW_COUNT);
-		mpstream_encode_uint(stream, db->nChange);
+		stream->is_flatten = true;
+		vstream_encode_reply_map(stream, map_size, IPROTO_SQL_INFO,
+					 "info");
+		vstream_encode_enum(stream, SQL_INFO_ROW_COUNT, "rowcount");
+		vstream_encode_uint(stream, db->nChange);
+		vstream_encode_map_commit(stream);
 		if (!stailq_empty(autoinc_id_list)) {
-			mpstream_encode_uint(stream,
-					     SQL_INFO_AUTOINCREMENT_IDS);
-			mpstream_encode_array(stream, id_count);
+			vstream_encode_enum(stream, SQL_INFO_AUTOINCREMENT_IDS,
+					    "autoincrement_ids");
+			vstream_encode_array(stream, id_count);
 			struct autoinc_id_entry *id_entry;
+			int i = 0;
 			stailq_foreach_entry(id_entry, autoinc_id_list, link) {
 				int64_t value = id_entry->id;
 				if (id_entry->id >= 0)
-					mpstream_encode_uint(stream, value);
+					vstream_encode_uint(stream, value);
 				else
-					mpstream_encode_int(stream, value);
+					vstream_encode_int(stream, value);
+				vstream_encode_array_commit(stream, i);
 			}
+			vstream_encode_map_commit(stream);
 		}
+		vstream_encode_reply_commit(stream);
 	}
 finish:
 	port_destroy(&response->port);
diff --git a/src/box/execute.h b/src/box/execute.h
index 7eb2121..882334f 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -34,7 +34,6 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include "port.h"
-#include "mpstream.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -53,6 +52,7 @@ struct obuf;
 struct region;
 struct sql_bind;
 struct xrow_header;
+struct vstream;
 
 /** EXECUTE request. */
 struct sql_request {
@@ -113,7 +113,7 @@ struct sql_response {
  */
 int
 sql_response_dump(struct sql_response *response, int *keys,
-		  struct mpstream *stream);
+		  struct vstream *stream);
 
 /**
  * Parse MessagePack array of SQL parameters and store a result
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 83b268b..f823c2d 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -61,6 +61,8 @@
 #include "rmean.h"
 #include "execute.h"
 #include "errinj.h"
+#include "vstream.h"
+#include "mpstream.h"
 
 enum {
 	IPROTO_SALT_SIZE = 32,
@@ -1667,16 +1669,19 @@ tx_process_sql(struct cmsg *m)
 	/* Prepare memory for the iproto header. */
 	if (iproto_prepare_header(out, &header_svp, IPROTO_SQL_HEADER_LEN) != 0)
 		goto error;
-	struct mpstream stream;
-	mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
+
+	struct mpstream mpstream;
+	mpstream_init(&mpstream, out, obuf_reserve_cb, obuf_alloc_cb,
 		      set_encode_error, &is_error);
 	if (is_error)
 		goto error;
-	if (sql_response_dump(&response, &keys, &stream) != 0) {
+	struct vstream vstream;
+	mp_vstream_init(&vstream, &mpstream);
+	if (sql_response_dump(&response, &keys, &vstream) != 0) {
 		obuf_rollback_to_svp(out, &header_svp);
 		goto error;
 	}
-	mpstream_flush(&stream);
+	mpstream_flush(&mpstream);
 	iproto_reply_sql(out, &header_svp, response.sync, schema_version, keys);
 	iproto_wpos_create(&msg->wpos, out);
 	return;
diff --git a/src/box/vstream.c b/src/box/vstream.c
new file mode 100644
index 0000000..d43c352
--- /dev/null
+++ b/src/box/vstream.c
@@ -0,0 +1,200 @@
+/*
+ * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "vstream.h"
+#include "mpstream.h"
+#include "iproto_constants.h"
+#include "port.h"
+#include "xrow.h"
+
+void
+mp_vstream_encode_array(struct vstream *stream, uint32_t size)
+{
+	mpstream_encode_array(stream->mpstream, size);
+}
+
+void
+mp_vstream_encode_map(struct vstream *stream, uint32_t size)
+{
+	mpstream_encode_map(stream->mpstream, size);
+}
+
+void
+mp_vstream_encode_uint(struct vstream *stream, uint64_t num)
+{
+	mpstream_encode_uint(stream->mpstream, num);
+}
+
+void
+mp_vstream_encode_int(struct vstream *stream, int64_t num)
+{
+	mpstream_encode_int(stream->mpstream, num);
+}
+
+void
+mp_vstream_encode_float(struct vstream *stream, float num)
+{
+	mpstream_encode_float(stream->mpstream, num);
+}
+
+void
+mp_vstream_encode_double(struct vstream *stream, double num)
+{
+	mpstream_encode_double(stream->mpstream, num);
+}
+
+void
+mp_vstream_encode_strn(struct vstream *stream, const char *str, uint32_t len)
+{
+	mpstream_encode_strn(stream->mpstream, str, len);
+}
+
+void
+mp_vstream_encode_nil(struct vstream *stream)
+{
+	mpstream_encode_nil(stream->mpstream);
+}
+
+void
+mp_vstream_encode_bool(struct vstream *stream, bool val)
+{
+	mpstream_encode_bool(stream->mpstream, val);
+}
+
+int
+mp_vstream_encode_port(struct vstream *stream, struct port *port)
+{
+	mpstream_flush(stream->mpstream);
+	/*
+	 * Just like SELECT, SQL uses output format compatible
+	 * with Tarantool 1.6
+	 */
+	if (port_dump_msgpack_16(port, stream->mpstream->ctx) < 0) {
+		/* Failed port dump destroyes the port. */
+		return -1;
+	}
+	mpstream_reset(stream->mpstream);
+	return 0;
+}
+
+int
+mp_vstream_encode_reply_array(struct vstream *stream, uint32_t size,
+			      uint8_t key, const char *str)
+{
+	(void)str;
+	uint8_t type = 0xdd;
+	char *pos = mpstream_reserve(stream->mpstream, IPROTO_KEY_HEADER_LEN);
+	if (pos == NULL) {
+		diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
+			 "mpstream_reserve", "pos");
+		return -1;
+	}
+	pos = mp_store_u8(pos, key);
+	pos = mp_store_u8(pos, type);
+	pos = mp_store_u32(pos, size);
+	mpstream_advance(stream->mpstream, IPROTO_KEY_HEADER_LEN);
+	return 0;
+}
+
+int
+mp_vstream_encode_reply_map(struct vstream *stream, uint32_t size, uint8_t key,
+			    const char *str)
+{
+	(void)str;
+	uint8_t type = 0xdf;
+	char *pos = mpstream_reserve(stream->mpstream, IPROTO_KEY_HEADER_LEN);
+	if (pos == NULL) {
+		diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
+			 "mpstream_reserve", "pos");
+		return -1;
+	}
+	pos = mp_store_u8(pos, key);
+	pos = mp_store_u8(pos, type);
+	pos = mp_store_u32(pos, size);
+	mpstream_advance(stream->mpstream, IPROTO_KEY_HEADER_LEN);
+	return 0;
+}
+
+void
+mp_vstream_encode_enum(struct vstream *stream, int64_t num, const char *str)
+{
+	(void)str;
+	if (num < 0)
+		mpstream_encode_int(stream->mpstream, num);
+	else
+		mpstream_encode_uint(stream->mpstream, num);
+}
+
+void
+mp_vstream_encode_reply_commit(struct vstream *stream)
+{
+	(void)stream;
+}
+
+void
+mp_vstream_encode_map_commit(struct vstream *stream)
+{
+	(void)stream;
+}
+
+void
+mp_vstream_encode_array_commit(struct vstream *stream, uint32_t id)
+{
+	(void)stream;
+	(void)id;
+}
+
+const struct vstream_vtab mp_vstream_vtab = {
+	/** encode_array = */ mp_vstream_encode_array,
+	/** encode_map = */ mp_vstream_encode_map,
+	/** encode_uint = */ mp_vstream_encode_uint,
+	/** encode_int = */ mp_vstream_encode_int,
+	/** encode_float = */ mp_vstream_encode_float,
+	/** encode_double = */ mp_vstream_encode_double,
+	/** encode_strn = */ mp_vstream_encode_strn,
+	/** encode_nil = */ mp_vstream_encode_nil,
+	/** encode_bool = */ mp_vstream_encode_bool,
+	/** encode_enum = */ mp_vstream_encode_enum,
+	/** encode_port = */ mp_vstream_encode_port,
+	/** encode_reply_array = */ mp_vstream_encode_reply_array,
+	/** encode_reply_map = */ mp_vstream_encode_reply_map,
+	/** encode_array_commit = */ mp_vstream_encode_array_commit,
+	/** encode_reply_commit = */ mp_vstream_encode_reply_commit,
+	/** encode_map_commit = */ mp_vstream_encode_map_commit,
+};
+
+void
+mp_vstream_init(struct vstream *vstream, struct mpstream *mpstream)
+{
+	vstream->vtab = &mp_vstream_vtab;
+	vstream->mpstream = mpstream;
+	vstream->is_flatten = false;
+}
diff --git a/src/box/vstream.h b/src/box/vstream.h
new file mode 100644
index 0000000..42f9813
--- /dev/null
+++ b/src/box/vstream.h
@@ -0,0 +1,191 @@
+#ifndef TARANTOOL_VSTREAM_H_INCLUDED
+#define TARANTOOL_VSTREAM_H_INCLUDED
+/*
+ * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "diag.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct vstream;
+struct mpstream;
+struct lua_State;
+struct port;
+
+struct vstream_vtab {
+	void (*encode_array)(struct vstream *stream, uint32_t size);
+	void (*encode_map)(struct vstream *stream, uint32_t size);
+	void (*encode_uint)(struct vstream *stream, uint64_t num);
+	void (*encode_int)(struct vstream *stream, int64_t num);
+	void (*encode_float)(struct vstream *stream, float num);
+	void (*encode_double)(struct vstream *stream, double num);
+	void (*encode_strn)(struct vstream *stream, const char *str,
+			    uint32_t len);
+	void (*encode_nil)(struct vstream *stream);
+	void (*encode_bool)(struct vstream *stream, bool val);
+	void (*encode_enum)(struct vstream *stream, int64_t num,
+			    const char *str);
+	int (*encode_port)(struct vstream *stream, struct port *port);
+	int (*encode_reply_array)(struct vstream *stream, uint32_t size,
+				  uint8_t key, const char *str);
+	int (*encode_reply_map)(struct vstream *stream, uint32_t size,
+				uint8_t key, const char *str);
+	void (*encode_array_commit)(struct vstream *stream, uint32_t id);
+	void (*encode_reply_commit)(struct vstream *stream);
+	void (*encode_map_commit)(struct vstream *stream);
+};
+
+struct vstream {
+	/** Virtual function table. */
+	const struct vstream_vtab *vtab;
+	/** TODO: Write comment. */
+	union {
+		struct mpstream *mpstream;
+		struct lua_State *L;
+	};
+	/** TODO: Write comment. */
+	bool is_flatten;
+};
+
+void
+mp_vstream_init(struct vstream *vstream, struct mpstream *mpstream);
+
+static inline void
+vstream_encode_array(struct vstream *stream, uint32_t size)
+{
+	return stream->vtab->encode_array(stream, size);
+}
+
+static inline void
+vstream_encode_map(struct vstream *stream, uint32_t size)
+{
+	return stream->vtab->encode_map(stream, size);
+}
+
+static inline void
+vstream_encode_uint(struct vstream *stream, uint64_t num)
+{
+	return stream->vtab->encode_uint(stream, num);
+}
+
+static inline void
+vstream_encode_int(struct vstream *stream, int64_t num)
+{
+	return stream->vtab->encode_int(stream, num);
+}
+
+static inline void
+vstream_encode_float(struct vstream *stream, float num)
+{
+	return stream->vtab->encode_float(stream, num);
+}
+
+static inline void
+vstream_encode_double(struct vstream *stream, double num)
+{
+	return stream->vtab->encode_double(stream, num);
+}
+
+static inline void
+vstream_encode_strn(struct vstream *stream, const char *str, uint32_t len)
+{
+	return stream->vtab->encode_strn(stream, str, len);
+}
+
+static inline void
+vstream_encode_str(struct vstream *stream, const char *str)
+{
+	return stream->vtab->encode_strn(stream, str, strlen(str));
+}
+
+static inline void
+vstream_encode_nil(struct vstream *stream)
+{
+	return stream->vtab->encode_nil(stream);
+}
+
+static inline void
+vstream_encode_bool(struct vstream *stream, bool val)
+{
+	return stream->vtab->encode_bool(stream, val);
+}
+
+static inline void
+vstream_encode_enum(struct vstream *stream, int64_t num, const char *str)
+{
+	return stream->vtab->encode_enum(stream, num, str);
+}
+
+static inline int
+vstream_encode_port(struct vstream *stream, struct port *port)
+{
+	return stream->vtab->encode_port(stream, port);
+}
+
+static inline int
+vstream_encode_reply_array(struct vstream *stream, uint32_t size, uint8_t key,
+			   const char *str)
+{
+	return stream->vtab->encode_reply_array(stream, size, key, str);
+}
+
+static inline int
+vstream_encode_reply_map(struct vstream *stream, uint32_t size, uint8_t key,
+			 const char *str)
+{
+	return stream->vtab->encode_reply_map(stream, size, key, str);
+}
+
+static inline void
+vstream_encode_reply_commit(struct vstream *stream)
+{
+	return stream->vtab->encode_reply_commit(stream);
+}
+
+static inline void
+vstream_encode_map_commit(struct vstream *stream)
+{
+	return stream->vtab->encode_map_commit(stream);
+}
+
+static inline void
+vstream_encode_array_commit(struct vstream *stream, uint32_t id)
+{
+	return stream->vtab->encode_array_commit(stream, id);
+}
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_VSTREAM_H_INCLUDED */
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 05/10] sql: EXPLAIN through net.box leads to SEGFAULT
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
                   ` (3 preceding siblings ...)
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 04/10] sql: create interface vstream imeevma
@ 2018-11-17 14:03 ` imeevma
  2018-11-19 13:47   ` [tarantool-patches] " Vladislav Shpilevoy
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 06/10] sql: SELECT from system spaces returns unpacked msgpack imeevma
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2018-11-17 14:03 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

EXPLAIN envokes SEGMENTATION FAULT when being executed through
net.box. It happens due to column type of the result of this
function being NULL.

Needed for #3505
---
 src/box/execute.c        | 11 +++++++----
 src/box/lua/net_box.c    | 15 ++++++++-------
 test/sql/iproto.result   | 11 +++++++++++
 test/sql/iproto.test.lua |  9 ++++++++-
 4 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 80ad655..da2f09c 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -468,15 +468,18 @@ sql_get_description(struct sqlite3_stmt *stmt, struct vstream *stream,
 		 * column_name simply returns them.
 		 */
 		assert(name != NULL);
-		vstream_encode_map(stream, 2);
+		int map_size = (type == NULL) ? 1 : 2;
+		vstream_encode_map(stream, map_size);
 
 		vstream_encode_enum(stream, IPROTO_FIELD_NAME, "name");
 		vstream_encode_str(stream, name);
 		vstream_encode_map_commit(stream);
 
-		vstream_encode_enum(stream, IPROTO_FIELD_TYPE, "type");
-		vstream_encode_str(stream, type);
-		vstream_encode_map_commit(stream);
+		if (map_size == 2) {
+			vstream_encode_enum(stream, IPROTO_FIELD_TYPE, "type");
+			vstream_encode_str(stream, type);
+			vstream_encode_map_commit(stream);
+		}
 
 		vstream_encode_array_commit(stream, i);
 	}
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index c7063d9..342c6a7 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -644,8 +644,7 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
 	lua_createtable(L, count, 0);
 	for (uint32_t i = 0; i < count; ++i) {
 		uint32_t map_size = mp_decode_map(data);
-		assert(map_size == 2);
-		(void) map_size;
+		assert(map_size == 1 || map_size == 2);
 		uint32_t key = mp_decode_uint(data);
 		assert(key == IPROTO_FIELD_NAME);
 		(void) key;
@@ -654,11 +653,13 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
 		const char *str = mp_decode_str(data, &len);
 		lua_pushlstring(L, str, len);
 		lua_setfield(L, -2, "name");
-		key = mp_decode_uint(data);
-		assert(key == IPROTO_FIELD_TYPE);
-		const char *type = mp_decode_str(data, &len);
-		lua_pushlstring(L, type, len);
-		lua_setfield(L, -2, "type");
+		if (map_size == 2) {
+			key = mp_decode_uint(data);
+			assert(key == IPROTO_FIELD_TYPE);
+			const char *type = mp_decode_str(data, &len);
+			lua_pushlstring(L, type, len);
+			lua_setfield(L, -2, "type");
+		}
 		lua_rawseti(L, -2, i + 1);
 	}
 }
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index d077ee8..0571a3b 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -773,6 +773,17 @@ cn:execute('drop table test')
 cn:close()
 ---
 ...
+-- gh-3505: Remove box.sql.execute
+cn = remote.connect(box.cfg.listen)
+---
+...
+-- Segmentation fault when EXPLAIN executed using net.box.
+_ = cn:execute("EXPLAIN SELECT 1;")
+---
+...
+cn:close()
+---
+...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 1470eda..e708bb2 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -247,10 +247,17 @@ cn:execute('drop table test')
 
 cn:close()
 
+-- gh-3505: Remove box.sql.execute
+cn = remote.connect(box.cfg.listen)
+
+-- Segmentation fault when EXPLAIN executed using net.box.
+_ = cn:execute("EXPLAIN SELECT 1;")
+
+cn:close()
+
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 
 -- Cleanup xlog
 box.snapshot()
-
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 06/10] sql: SELECT from system spaces returns unpacked msgpack.
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
                   ` (4 preceding siblings ...)
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 05/10] sql: EXPLAIN through net.box leads to SEGFAULT imeevma
@ 2018-11-17 14:04 ` imeevma
  2018-11-19 13:48   ` [tarantool-patches] " Vladislav Shpilevoy
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 07/10] sql: too many autogenerated ids leads to SEGFAULT imeevma
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2018-11-17 14:04 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

SELECT executed through net.box returns unpacked msgpask. Fixed in
this patch.

Needed for #3505
---
 src/box/execute.c        | 22 +++++++++++++++-------
 test/sql/iproto.result   |  9 +++++++++
 test/sql/iproto.test.lua |  4 ++++
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index da2f09c..a7f8a54 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -283,13 +283,21 @@ sql_column_to_messagepack(struct sqlite3_stmt *stmt, int i,
 	}
 	case SQLITE_BLOB: {
 		uint32_t len = sqlite3_column_bytes(stmt, i);
-		size = mp_sizeof_bin(len);
-		char *pos = (char *) region_alloc(region, size);
-		if (pos == NULL)
-			goto oom;
-		const char *s;
-		s = (const char *)sqlite3_column_blob(stmt, i);
-		mp_encode_bin(pos, s, len);
+		const char *s =
+			(const char *)sqlite3_column_blob(stmt, i);
+		if (sql_column_subtype(stmt, i) == SQL_SUBTYPE_MSGPACK) {
+			size = len;
+			char *pos = (char *)region_alloc(region, size);
+			if (pos == NULL)
+				goto oom;
+			memcpy(pos, s, len);
+		} else {
+			size = mp_sizeof_bin(len);
+			char *pos = (char *)region_alloc(region, size);
+			if (pos == NULL)
+				goto oom;
+			mp_encode_bin(pos, s, len);
+		}
 		break;
 	}
 	case SQLITE_NULL: {
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 0571a3b..d1bd42a 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -781,6 +781,15 @@ cn = remote.connect(box.cfg.listen)
 _ = cn:execute("EXPLAIN SELECT 1;")
 ---
 ...
+-- SELECT from system spaces returns unpacked msgpack.
+res = cn:execute('select "format" from "_space" limit 1;')
+---
+...
+res.rows
+---
+- - [[{'name': 'space_id', 'type': 'unsigned'}, {'name': 'lsn', 'type': 'unsigned'},
+      {'name': 'tuple', 'type': 'array'}]]
+...
 cn:close()
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index e708bb2..54f17bc 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -253,6 +253,10 @@ cn = remote.connect(box.cfg.listen)
 -- Segmentation fault when EXPLAIN executed using net.box.
 _ = cn:execute("EXPLAIN SELECT 1;")
 
+-- SELECT from system spaces returns unpacked msgpack.
+res = cn:execute('select "format" from "_space" limit 1;')
+res.rows
+
 cn:close()
 
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 07/10] sql: too many autogenerated ids leads to SEGFAULT
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
                   ` (5 preceding siblings ...)
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 06/10] sql: SELECT from system spaces returns unpacked msgpack imeevma
@ 2018-11-17 14:04 ` imeevma
  2018-11-19 13:47   ` [tarantool-patches] " Vladislav Shpilevoy
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 08/10] box: add method dump_lua to port imeevma
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2018-11-17 14:04 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

This probleam appeared because region was cleaned twice: once in
sqlite3VdbeHalt() and once in sqlite3VdbeDelete() which was
executed during sqlite3_finalize(). Autogenerated ids that were
saved there, were fetched after sqlite3VdbeHalt() and before
sqlite3_finalize(). In this patch region cleaning in
sqlite3VdbeHalt() were removed.

Needed for #3505
Follow up #2618
Follow up #3199
---
 src/box/sql/vdbe.c       |  8 ++------
 src/box/sql/vdbeaux.c    |  6 ------
 test/sql/iproto.result   | 16 ++++++++++++++++
 test/sql/iproto.test.lua |  7 +++++++
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index b6afe91..cc340e9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2911,12 +2911,8 @@ case OP_MakeRecord: {
 	 * memory shouldn't be reused until it is written into WAL.
 	 *
 	 * However, if memory for ephemeral space is allocated
-	 * on region, it will be freed only in vdbeHalt() routine.
-	 * It is the only way to free this region memory,
-	 * since ephemeral spaces don't have nothing in common
-	 * with txn routine and region memory won't be released
-	 * after txn_commit() or txn_rollback() as it happens
-	 * with ordinary spaces.
+	 * on region, it will be freed only in sqlite3_finalize()
+	 * routine.
 	 */
 	if (bIsEphemeral) {
 		rc = sqlite3VdbeMemClearAndResize(pOut, nByte);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 615a0f0..f2faad8 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2498,12 +2498,6 @@ sqlite3VdbeHalt(Vdbe * p)
 		p->rc = SQLITE_NOMEM_BKPT;
 	}
 
-	/* Release all region memory which was allocated
-	 * to hold tuples to be inserted into ephemeral spaces.
-	 */
-	if (!box_txn())
-		fiber_gc();
-
 	assert(db->nVdbeActive > 0 || box_txn() ||
 	       p->anonymous_savepoint == NULL);
 	return (p->rc == SQLITE_BUSY ? SQLITE_BUSY : SQLITE_OK);
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index d1bd42a..711d0ef 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -790,6 +790,22 @@ res.rows
 - - [[{'name': 'space_id', 'type': 'unsigned'}, {'name': 'lsn', 'type': 'unsigned'},
       {'name': 'tuple', 'type': 'array'}]]
 ...
+-- Too many autogenerated ids leads to SEGFAULT.
+cn = remote.connect(box.cfg.listen)
+---
+...
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT)')
+---
+...
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end
+---
+...
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")
+---
+...
+box.sql.execute('DROP TABLE t1')
+---
+...
 cn:close()
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 54f17bc..67ca5cd 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -257,6 +257,13 @@ _ = cn:execute("EXPLAIN SELECT 1;")
 res = cn:execute('select "format" from "_space" limit 1;')
 res.rows
 
+-- Too many autogenerated ids leads to SEGFAULT.
+cn = remote.connect(box.cfg.listen)
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT)')
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")
+box.sql.execute('DROP TABLE t1')
+
 cn:close()
 
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 08/10] box: add method dump_lua to port
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
                   ` (6 preceding siblings ...)
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 07/10] sql: too many autogenerated ids leads to SEGFAULT imeevma
@ 2018-11-17 14:04 ` imeevma
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 09/10] lua: create vstream implementation for Lua imeevma
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: imeevma @ 2018-11-17 14:04 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

New method dump_lua dumps saved in port tuples to Lua stack. It
will allow us to call this method from vstream without any other
interaction with port.

Needed for #3505
---
 src/box/lua/call.c |  1 +
 src/box/port.c     | 22 ++++++++++++++++++++++
 src/box/port.h     | 12 ++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 1f20426..52939ae 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -424,6 +424,7 @@ port_lua_dump_plain(struct port *port, uint32_t *size);
 static const struct port_vtab port_lua_vtab = {
 	.dump_msgpack = port_lua_dump,
 	.dump_msgpack_16 = port_lua_dump_16,
+	.dump_lua = NULL,
 	.dump_plain = port_lua_dump_plain,
 	.destroy = port_lua_destroy,
 };
diff --git a/src/box/port.c b/src/box/port.c
index 266cf3d..a65a32d 100644
--- a/src/box/port.c
+++ b/src/box/port.c
@@ -36,6 +36,8 @@
 #include <small/mempool.h>
 #include <fiber.h>
 #include "errinj.h"
+#include "lua/utils.h"
+#include "lua/tuple.h"
 
 static struct mempool port_tuple_entry_pool;
 
@@ -121,6 +123,19 @@ port_tuple_dump_msgpack(struct port *base, struct obuf *out)
 	return 1;
 }
 
+static int
+port_tuple_dump_lua(struct port *base, struct lua_State *L)
+{
+	struct port_tuple *port = port_tuple(base);
+	struct port_tuple_entry *pe;
+	int i = 0;
+	for (pe = port->first; pe != NULL; pe = pe->next) {
+		luaT_pushtuple(L, pe->tuple);
+		lua_rawseti(L, -2, ++i);
+	}
+	return port->size;
+}
+
 void
 port_destroy(struct port *port)
 {
@@ -139,6 +154,12 @@ port_dump_msgpack_16(struct port *port, struct obuf *out)
 	return port->vtab->dump_msgpack_16(port, out);
 }
 
+int
+port_dump_lua(struct port *port, struct lua_State *L)
+{
+	return port->vtab->dump_lua(port, L);
+}
+
 const char *
 port_dump_plain(struct port *port, uint32_t *size)
 {
@@ -161,6 +182,7 @@ port_free(void)
 const struct port_vtab port_tuple_vtab = {
 	.dump_msgpack = port_tuple_dump_msgpack,
 	.dump_msgpack_16 = port_tuple_dump_msgpack_16,
+	.dump_lua = port_tuple_dump_lua,
 	.dump_plain = NULL,
 	.destroy = port_tuple_destroy,
 };
diff --git a/src/box/port.h b/src/box/port.h
index 882bb37..3bd83b0 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -78,6 +78,11 @@ struct port_vtab {
 	 */
 	int (*dump_msgpack_16)(struct port *port, struct obuf *out);
 	/**
+	 * Dump the content of a port to Lua stack.
+	 * On success returns number of entries dumped.
+	 */
+	int (*dump_lua)(struct port *port, struct lua_State *L);
+	/**
 	 * Dump a port content as a plain text into a buffer,
 	 * allocated inside.
 	 */
@@ -185,6 +190,13 @@ int
 port_dump_msgpack_16(struct port *port, struct obuf *out);
 
 /**
+ * Same as port_dump(), but use the legacy Tarantool 1.6
+ * format.
+ */
+int
+port_dump_lua(struct port *port, struct lua_State *L);
+
+/**
  * Dump a port content as a plain text into a buffer,
  * allocated inside.
  * @param port Port with data to dump.
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 09/10] lua: create vstream implementation for Lua
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
                   ` (7 preceding siblings ...)
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 08/10] box: add method dump_lua to port imeevma
@ 2018-11-17 14:04 ` imeevma
  2018-11-19 17:58   ` [tarantool-patches] " Vladislav Shpilevoy
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 10/10] sql: check new box.sql.execute() imeevma
  2018-11-19 12:54 ` [tarantool-patches] Re: [PATCH v1 00/10] sql: remove box.sql.execute Vladislav Shpilevoy
  10 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2018-11-17 14:04 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

Thas patch creates vstream implementation for Lua and function
box.sql.new_execute() that uses this implementation.

Part of #3505
---
 src/box/lua/sql.c |  36 +++++++++++++
 src/box/vstream.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/box/vstream.h |   3 ++
 3 files changed, 187 insertions(+)

diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 17e2694..b5e428d 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -6,6 +6,8 @@
 #include "box/info.h"
 #include "lua/utils.h"
 #include "info.h"
+#include "box/execute.h"
+#include "box/vstream.h"
 
 static void
 lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
@@ -111,6 +113,39 @@ sqlerror:
 }
 
 static int
+lbox_execute(struct lua_State *L)
+{
+	struct sqlite3 *db = sql_get();
+	if (db == NULL)
+		return luaL_error(L, "not ready");
+
+	size_t length;
+	const char *sql = lua_tolstring(L, 1, &length);
+	if (sql == NULL)
+		return luaL_error(L, "usage: box.execute(sqlstring)");
+
+	struct sql_request request = {};
+	struct sql_response response = {};
+	request.sql_text = sql;
+	request.sql_text_len = length;
+	if (sql_prepare_and_execute(&request, &response, &fiber()->gc) != 0)
+		goto sqlerror;
+
+	int keys;
+	struct vstream vstream;
+	lua_vstream_init(&vstream, L);
+	lua_newtable(L);
+	if (sql_response_dump(&response, &keys, &vstream) != 0) {
+		lua_pop(L, 1);
+		goto sqlerror;
+	}
+	return 1;
+sqlerror:
+	lua_pushstring(L, sqlite3_errmsg(db));
+	return lua_error(L);
+}
+
+static int
 lua_sql_debug(struct lua_State *L)
 {
 	struct info_handler info;
@@ -124,6 +159,7 @@ box_lua_sqlite_init(struct lua_State *L)
 {
 	static const struct luaL_Reg module_funcs [] = {
 		{"execute", lua_sql_execute},
+		{"new_execute", lbox_execute},
 		{"debug", lua_sql_debug},
 		{NULL, NULL}
 	};
diff --git a/src/box/vstream.c b/src/box/vstream.c
index d43c352..9d446ee 100644
--- a/src/box/vstream.c
+++ b/src/box/vstream.c
@@ -34,6 +34,8 @@
 #include "iproto_constants.h"
 #include "port.h"
 #include "xrow.h"
+#include "lua/utils.h"
+#include "lua/tuple.h"
 
 void
 mp_vstream_encode_array(struct vstream *stream, uint32_t size)
@@ -198,3 +200,149 @@ mp_vstream_init(struct vstream *vstream, struct mpstream *mpstream)
 	vstream->mpstream = mpstream;
 	vstream->is_flatten = false;
 }
+
+void
+lua_vstream_encode_array(struct vstream *stream, uint32_t size)
+{
+	lua_createtable(stream->L, size, 0);
+}
+
+void
+lua_vstream_encode_map(struct vstream *stream, uint32_t size)
+{
+	(void)size;
+	lua_newtable(stream->L);
+}
+
+void
+lua_vstream_encode_uint(struct vstream *stream, uint64_t num)
+{
+	luaL_pushuint64(stream->L, num);
+}
+
+void
+lua_vstream_encode_int(struct vstream *stream, int64_t num)
+{
+	luaL_pushint64(stream->L, num);
+}
+
+void
+lua_vstream_encode_float(struct vstream *stream, float num)
+{
+	lua_pushnumber(stream->L, num);
+}
+
+void
+lua_vstream_encode_double(struct vstream *stream, double num)
+{
+	lua_pushnumber(stream->L, num);
+}
+
+void
+lua_vstream_encode_strn(struct vstream *stream, const char *str, uint32_t len)
+{
+	lua_pushlstring(stream->L, str, len);
+}
+
+void
+lua_vstream_encode_nil(struct vstream *stream)
+{
+	lua_pushnil(stream->L);
+}
+
+void
+lua_vstream_encode_bool(struct vstream *stream, bool val)
+{
+	lua_pushboolean(stream->L, val);
+}
+
+int
+lua_vstream_encode_port(struct vstream *stream, struct port *port)
+{
+	if (port_dump_lua(port, stream->L) < 0)
+		return -1;
+	return 0;
+}
+
+int
+lua_vstream_encode_reply_array(struct vstream *stream, uint32_t size,
+			      uint8_t key, const char *str)
+{
+	(void)key;
+	if (stream->is_flatten)
+		return 0;
+	lua_createtable(stream->L, size, 0);
+	lua_setfield(stream->L, -2, str);
+	lua_getfield(stream->L, -1, str);
+	return 0;
+}
+
+int
+lua_vstream_encode_reply_map(struct vstream *stream, uint32_t size, uint8_t key,
+			    const char *str)
+{
+	(void)size;
+	(void)key;
+	if (stream->is_flatten)
+		return 0;
+	lua_newtable(stream->L);
+	lua_setfield(stream->L, -2, str);
+	lua_getfield(stream->L, -1, str);
+	return 0;
+}
+
+void
+lua_vstream_encode_enum(struct vstream *stream, int64_t num, const char *str)
+{
+	(void)num;
+	lua_pushlstring(stream->L, str, strlen(str));
+}
+
+void
+lua_vstream_encode_reply_commit(struct vstream *stream)
+{
+	if(!stream->is_flatten)
+		lua_pop(stream->L, 1);
+}
+
+void
+lua_vstream_encode_map_commit(struct vstream *stream)
+{
+	size_t length;
+	const char *key = lua_tolstring(stream->L, -2, &length);
+	lua_setfield(stream->L, -3, key);
+	lua_pop(stream->L, 1);
+}
+
+void
+lua_vstream_encode_array_commit(struct vstream *stream, uint32_t id)
+{
+	lua_rawseti(stream->L, -2, id + 1);
+}
+
+const struct vstream_vtab lua_vstream_vtab = {
+	/** encode_array = */ lua_vstream_encode_array,
+	/** encode_map = */ lua_vstream_encode_map,
+	/** encode_uint = */ lua_vstream_encode_uint,
+	/** encode_int = */ lua_vstream_encode_int,
+	/** encode_float = */ lua_vstream_encode_float,
+	/** encode_double = */ lua_vstream_encode_double,
+	/** encode_strn = */ lua_vstream_encode_strn,
+	/** encode_nil = */ lua_vstream_encode_nil,
+	/** encode_bool = */ lua_vstream_encode_bool,
+	/** encode_enum = */ lua_vstream_encode_enum,
+	/** encode_port = */ lua_vstream_encode_port,
+	/** encode_reply_array = */ lua_vstream_encode_reply_array,
+	/** encode_reply_map = */ lua_vstream_encode_reply_map,
+	/** encode_array_commit = */ lua_vstream_encode_array_commit,
+	/** encode_reply_commit = */ lua_vstream_encode_reply_commit,
+	/** encode_map_commit = */ lua_vstream_encode_map_commit,
+};
+
+void
+lua_vstream_init(struct vstream *vstream, struct lua_State *L)
+{
+	vstream->vtab = &lua_vstream_vtab;
+	vstream->L = L;
+	vstream->is_flatten = false;
+}
diff --git a/src/box/vstream.h b/src/box/vstream.h
index 42f9813..d5c18b8 100644
--- a/src/box/vstream.h
+++ b/src/box/vstream.h
@@ -80,6 +80,9 @@ struct vstream {
 void
 mp_vstream_init(struct vstream *vstream, struct mpstream *mpstream);
 
+void
+lua_vstream_init(struct vstream *vstream, struct lua_State *L);
+
 static inline void
 vstream_encode_array(struct vstream *stream, uint32_t size)
 {
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] [PATCH v1 10/10] sql: check new box.sql.execute()
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
                   ` (8 preceding siblings ...)
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 09/10] lua: create vstream implementation for Lua imeevma
@ 2018-11-17 14:04 ` imeevma
  2018-11-19 12:54 ` [tarantool-patches] Re: [PATCH v1 00/10] sql: remove box.sql.execute Vladislav Shpilevoy
  10 siblings, 0 replies; 18+ messages in thread
From: imeevma @ 2018-11-17 14:04 UTC (permalink / raw)
  To: tarantool-patches, v.shpilevoy

This commit checks that new implementation of box.sql.execute() is
able to pass all tests. This is temporary commit and should be
dropped later.

Needed for #3505
---
 src/box/execute.c      |  11 ++---
 src/box/execute.h      |   3 +-
 src/box/iproto.cc      |   3 +-
 src/box/lua/schema.lua |  23 +++++++++++
 src/box/lua/sql.c      | 107 +------------------------------------------------
 5 files changed, 35 insertions(+), 112 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index a7f8a54..40636d1 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -497,7 +497,7 @@ sql_get_description(struct sqlite3_stmt *stmt, struct vstream *stream,
 
 static inline int
 sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
-	    struct region *region)
+	    struct region *region, int error_id)
 {
 	int rc, column_count = sqlite3_column_count(stmt);
 	if (column_count > 0) {
@@ -514,7 +514,7 @@ sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
 		assert(rc != SQLITE_ROW && rc != SQLITE_OK);
 	}
 	if (rc != SQLITE_DONE) {
-		diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+		diag_set(ClientError, error_id, sqlite3_errmsg(db));
 		return -1;
 	}
 	return 0;
@@ -522,7 +522,8 @@ sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
 
 int
 sql_prepare_and_execute(const struct sql_request *request,
-			struct sql_response *response, struct region *region)
+			struct sql_response *response, struct region *region,
+			int error_id)
 {
 	const char *sql = request->sql_text;
 	uint32_t len = request->sql_text_len;
@@ -533,7 +534,7 @@ sql_prepare_and_execute(const struct sql_request *request,
 		return -1;
 	}
 	if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
-		diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+		diag_set(ClientError, error_id, sqlite3_errmsg(db));
 		return -1;
 	}
 	assert(stmt != NULL);
@@ -541,7 +542,7 @@ sql_prepare_and_execute(const struct sql_request *request,
 	response->prep_stmt = stmt;
 	response->sync = request->sync;
 	if (sql_bind(request, stmt) == 0 &&
-	    sql_execute(db, stmt, &response->port, region) == 0)
+	    sql_execute(db, stmt, &response->port, region, error_id) == 0)
 		return 0;
 	port_destroy(&response->port);
 	sqlite3_finalize(stmt);
diff --git a/src/box/execute.h b/src/box/execute.h
index 882334f..08efea9 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -147,7 +147,8 @@ sql_bind_list_decode(struct sql_request *request, const char *data,
  */
 int
 sql_prepare_and_execute(const struct sql_request *request,
-			struct sql_response *response, struct region *region);
+			struct sql_response *response, struct region *region,
+			int error_id);
 
 #if defined(__cplusplus)
 } /* extern "C" { */
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index f823c2d..e082c1b 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1657,7 +1657,8 @@ tx_process_sql(struct cmsg *m)
 		goto error;
 	assert(msg->header.type == IPROTO_EXECUTE);
 	tx_inject_delay();
-	if (sql_prepare_and_execute(&msg->sql, &response, &fiber()->gc) != 0)
+	if (sql_prepare_and_execute(&msg->sql, &response, &fiber()->gc,
+	    ER_SQL_EXECUTE) != 0)
 		goto error;
 	/*
 	 * Take an obuf only after execute(). Else the buffer can
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 8a804f0..02ec2fd 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2456,3 +2456,26 @@ box.feedback.save = function(file_name)
 end
 
 box.NULL = msgpack.NULL
+
+box.sql.execute = function(sql)
+    local result = box.sql.new_execute(sql)
+    if result == nil then return end
+    local ret = nil
+    if result.rows ~= nil then
+        ret = {}
+        for key, row in pairs(result.rows) do
+            if type(row) == 'cdata' then
+                table.insert(ret, row:totable())
+            end
+        end
+    end
+    if result.metadata ~= nil then
+        if ret == nil then ret = {} end
+        ret[0] = {}
+        for key, row in pairs(result.metadata) do
+            table.insert(ret[0], row['name'])
+        end
+        setmetatable(ret, {__serialize = 'sequence'})
+    end
+    if ret ~= nil then return ret end
+end
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index b5e428d..89872f6 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -9,109 +9,6 @@
 #include "box/execute.h"
 #include "box/vstream.h"
 
-static void
-lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
-{
-	int column_count = sqlite3_column_count(stmt);
-	lua_createtable(L, column_count, 0);
-	for (int i = 0; i < column_count; i++) {
-		const char *name = sqlite3_column_name(stmt, i);
-		lua_pushstring(L, name == NULL ? "" : name);
-		lua_rawseti(L, -2, i+1);
-	}
-}
-
-static void
-lua_push_row(struct lua_State *L, struct sqlite3_stmt *stmt)
-{
-	int column_count = sqlite3_column_count(stmt);
-
-	lua_createtable(L, column_count, 0);
-	lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_array_metatable_ref);
-	lua_setmetatable(L, -2);
-
-	for (int i = 0; i < column_count; i++) {
-		int type = sqlite3_column_type(stmt, i);
-		switch (type) {
-		case SQLITE_INTEGER:
-			luaL_pushint64(L, sqlite3_column_int64(stmt, i));
-			break;
-		case SQLITE_FLOAT:
-			lua_pushnumber(L, sqlite3_column_double(stmt, i));
-			break;
-		case SQLITE_TEXT: {
-			const void *text = sqlite3_column_text(stmt, i);
-			lua_pushlstring(L, text,
-					sqlite3_column_bytes(stmt, i));
-			break;
-		}
-		case SQLITE_BLOB: {
-			const void *blob = sqlite3_column_blob(stmt, i);
-			if (sql_column_subtype(stmt,i) == SQL_SUBTYPE_MSGPACK) {
-				luamp_decode(L, luaL_msgpack_default,
-					     (const char **)&blob);
-			} else {
-				lua_pushlstring(L, blob,
-					sqlite3_column_bytes(stmt, i));
-			}
-			break;
-		}
-		case SQLITE_NULL:
-			lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_nil_ref);
-			break;
-		default:
-			assert(0);
-		}
-		lua_rawseti(L, -2, i+1);
-	}
-}
-
-static int
-lua_sql_execute(struct lua_State *L)
-{
-	sqlite3 *db = sql_get();
-	if (db == NULL)
-		return luaL_error(L, "not ready");
-
-	size_t length;
-	const char *sql = lua_tolstring(L, 1, &length);
-	if (sql == NULL)
-		return luaL_error(L, "usage: box.sql.execute(sqlstring)");
-
-	struct sqlite3_stmt *stmt;
-	if (sqlite3_prepare_v2(db, sql, length, &stmt, &sql) != SQLITE_OK)
-		goto sqlerror;
-	assert(stmt != NULL);
-
-	int rc;
-	int retval_count;
-	if (sqlite3_column_count(stmt) == 0) {
-		while ((rc = sqlite3_step(stmt)) == SQLITE_ROW);
-		retval_count = 0;
-	} else {
-		lua_newtable(L);
-		lua_pushvalue(L, lua_upvalueindex(1));
-		lua_setmetatable(L, -2);
-		lua_push_column_names(L, stmt);
-		lua_rawseti(L, -2, 0);
-
-		int row_count = 0;
-		while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
-			lua_push_row(L, stmt);
-			lua_rawseti(L, -2, ++row_count);
-		}
-		retval_count = 1;
-	}
-        if (rc != SQLITE_OK && rc != SQLITE_DONE)
-		goto sqlerror;
-	sqlite3_finalize(stmt);
-	return retval_count;
-sqlerror:
-	lua_pushstring(L, sqlite3_errmsg(db));
-	sqlite3_finalize(stmt);
-	return lua_error(L);
-}
-
 static int
 lbox_execute(struct lua_State *L)
 {
@@ -128,7 +25,8 @@ lbox_execute(struct lua_State *L)
 	struct sql_response response = {};
 	request.sql_text = sql;
 	request.sql_text_len = length;
-	if (sql_prepare_and_execute(&request, &response, &fiber()->gc) != 0)
+	if (sql_prepare_and_execute(&request, &response, &fiber()->gc,
+				    ER_SYSTEM) != 0)
 		goto sqlerror;
 
 	int keys;
@@ -158,7 +56,6 @@ void
 box_lua_sqlite_init(struct lua_State *L)
 {
 	static const struct luaL_Reg module_funcs [] = {
-		{"execute", lua_sql_execute},
 		{"new_execute", lbox_execute},
 		{"debug", lua_sql_debug},
 		{NULL, NULL}
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH v1 00/10] sql: remove box.sql.execute
  2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
                   ` (9 preceding siblings ...)
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 10/10] sql: check new box.sql.execute() imeevma
@ 2018-11-19 12:54 ` Vladislav Shpilevoy
  10 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-19 12:54 UTC (permalink / raw)
  To: tarantool-patches, imeevma

https://github.com/tarantool/tarantool/tree/imeevma/gh-3505-replace-box_sql_execute-by-box_execute
https://github.com/tarantool/tarantool/issues/3505

On 17/11/2018 17:03, imeevma@tarantool.org wrote:
> The goal of this patch-set is to make functions from execute.c
> the only way to execute SQL statements. This goal includes
> identical output for executed SQL statements no matter how they
> were executed: through net.box or through box.
> 
> This version of patch-set is not complete. It doesn't have last
> part, which is replacing box.sql.execute by box.execute, because
> it will lead to massive test editing.
> 
> The main goal of this version of patch-set is to get comments
> about design of vstream and general comments about this patch-set.
> 
> A bit about patches in this patch-set:
> 
> Patch 1 allows to use SQL query as plain text in sql_request.
> 
> Patch 2 makes execute.c a bit more universal by removing
> IPROTO functions from there.
> 
> Patch 3 allows us to design vstream by wrapping mpstream
> functions.
> 
> Patch 4 creates interface vstream and its mpstream implementation.
> 
> Patch 5 fixes bug with EXPLAIN being executed through net.box
> throws SEGMENTATION FAULT.
> 
> Patch 6 fixes bug with SELECT from system spaces returns some
> columns as unpacked msgpack.
> 
> Patch 7 fixes bug with region being cleared twice during execution
> of VDBE.
> 
> Patch 8 creates method for port which will allow us to dump port
> directly to Lua stack.
> 
> Patch 9 creates vstream implementation for Lua and defines new
> box.sql.new_execute() function that will become box.execute() in
> next vesions.
> 
> Patch 10 is created to check that box.sql.new_execute() is able to
> pass through test created for box.sql.execute(). Created new
> implementation of box.sql.execute() that transforms output of
> box.sql.new_execute() to format of old box.sql.execute().
> 
> Kirill Shcherbatov (1):
>    box: store sql text and length in sql_request
> 
> Mergen Imeev (9):
>    iproto: remove iproto functions from execute.c
>    iproto: replace obuf by mpstream in execute.c
>    sql: create interface vstream
>    sql: EXPLAIN through net.box leads to SEGFAULT
>    sql: SELECT from system spaces returns unpacked msgpack.
>    sql: too many autogenerated ids leads to SEGFAULT
>    box: add method dump_lua to port
>    lua: create vstream implementation for Lua
>    sql: check new box.sql.execute()
> 
>   src/box/CMakeLists.txt   |   1 +
>   src/box/execute.c        | 218 ++++++++++-------------------
>   src/box/execute.h        |  43 +++---
>   src/box/iproto.cc        |  93 ++++++++++++-
>   src/box/lua/call.c       |   1 +
>   src/box/lua/net_box.c    |  15 +-
>   src/box/lua/schema.lua   |  23 ++++
>   src/box/lua/sql.c        | 107 +++------------
>   src/box/port.c           |  22 +++
>   src/box/port.h           |  12 ++
>   src/box/sql/vdbe.c       |   8 +-
>   src/box/sql/vdbeaux.c    |   6 -
>   src/box/vstream.c        | 348 +++++++++++++++++++++++++++++++++++++++++++++++
>   src/box/vstream.h        | 194 ++++++++++++++++++++++++++
>   src/box/xrow.c           |  39 ------
>   src/box/xrow.h           |  14 --
>   test/sql/iproto.result   |  36 +++++
>   test/sql/iproto.test.lua |  20 ++-
>   18 files changed, 870 insertions(+), 330 deletions(-)
>   create mode 100644 src/box/vstream.c
>   create mode 100644 src/box/vstream.h
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH v1 05/10] sql: EXPLAIN through net.box leads to SEGFAULT
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 05/10] sql: EXPLAIN through net.box leads to SEGFAULT imeevma
@ 2018-11-19 13:47   ` Vladislav Shpilevoy
  0 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-19 13:47 UTC (permalink / raw)
  To: imeevma, tarantool-patches

Hi! Thanks for the patch! I have changed it and moved into
new patchset to make review easier.

On 17/11/2018 17:03, imeevma@tarantool.org wrote:
> EXPLAIN envokes SEGMENTATION FAULT when being executed through
> net.box. It happens due to column type of the result of this
> function being NULL.
> 
> Needed for #3505
> ---
>   src/box/execute.c        | 11 +++++++----
>   src/box/lua/net_box.c    | 15 ++++++++-------
>   test/sql/iproto.result   | 11 +++++++++++
>   test/sql/iproto.test.lua |  9 ++++++++-
>   4 files changed, 34 insertions(+), 12 deletions(-)
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH v1 07/10] sql: too many autogenerated ids leads to SEGFAULT
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 07/10] sql: too many autogenerated ids leads to SEGFAULT imeevma
@ 2018-11-19 13:47   ` Vladislav Shpilevoy
  0 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-19 13:47 UTC (permalink / raw)
  To: tarantool-patches, imeevma

Thanks for the patch! I have moved it into
new patchset to make review easier.

On 17/11/2018 17:04, imeevma@tarantool.org wrote:
> This probleam appeared because region was cleaned twice: once in
> sqlite3VdbeHalt() and once in sqlite3VdbeDelete() which was
> executed during sqlite3_finalize(). Autogenerated ids that were
> saved there, were fetched after sqlite3VdbeHalt() and before
> sqlite3_finalize(). In this patch region cleaning in
> sqlite3VdbeHalt() were removed.
> 
> Needed for #3505
> Follow up #2618
> Follow up #3199
> ---
>   src/box/sql/vdbe.c       |  8 ++------
>   src/box/sql/vdbeaux.c    |  6 ------
>   test/sql/iproto.result   | 16 ++++++++++++++++
>   test/sql/iproto.test.lua |  7 +++++++
>   4 files changed, 25 insertions(+), 12 deletions(-)
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH v1 06/10] sql: SELECT from system spaces returns unpacked msgpack.
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 06/10] sql: SELECT from system spaces returns unpacked msgpack imeevma
@ 2018-11-19 13:48   ` Vladislav Shpilevoy
  0 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-19 13:48 UTC (permalink / raw)
  To: imeevma, tarantool-patches

Thanks for the patch! I have moved it into
new patchset to make review easier.

On 17/11/2018 17:04, imeevma@tarantool.org wrote:
> SELECT executed through net.box returns unpacked msgpask. Fixed in
> this patch.
> 
> Needed for #3505
> ---
>   src/box/execute.c        | 22 +++++++++++++++-------
>   test/sql/iproto.result   |  9 +++++++++
>   test/sql/iproto.test.lua |  4 ++++
>   3 files changed, 28 insertions(+), 7 deletions(-)
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH v1 02/10] iproto: remove iproto functions from execute.c
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 02/10] iproto: remove iproto functions from execute.c imeevma
@ 2018-11-19 17:58   ` Vladislav Shpilevoy
  0 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-19 17:58 UTC (permalink / raw)
  To: imeevma, tarantool-patches

Thanks for the patch!

On 17/11/2018 17:03, imeevma@tarantool.org wrote:
> To make functions in execute.h more universal we should reduce
> their dependence on IPROTO. This patch removes IPROTO functions
> from execute.c.
> 
> Needed for #3505
> ---
>   src/box/execute.c | 116 ++++++++++++++----------------------------------------
>   src/box/execute.h |  34 ++++++++--------
>   src/box/iproto.cc |  71 ++++++++++++++++++++++++++++++++-
>   src/box/xrow.c    |  39 ------------------
>   src/box/xrow.h    |  14 -------
>   5 files changed, 115 insertions(+), 159 deletions(-)
> 

1. To summarize the patch: you have moved xrow_decode_sql from execute.c
into iproto.cc. But I do not think we should do it. The only goal
of the whole patchset is removal of any IPROTO mentions not from
execute.c, but from sql_response_dump and functions which it calls.

I did it in a separate commit on the branch. I've attached it to the
end of the email, and pushed onto the branch.

> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index cd61393..5fb2aff 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1118,6 +1118,67 @@ static const struct cmsg_hop error_route[] = {
>   	{ net_send_error, NULL },
>   };
>   
> +/**
> + * Parse the EXECUTE request.
> + * @param row Encoded data.
> + * @param[out] request Request to decode to.
> + * @param region Allocator.
> + *
> + * @retval  0 Sucess.
> + * @retval -1 Format or memory error.
> + */
> +static inline int
> +xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
> +		struct region *region)
> +{
> +	if (row->bodycnt == 0) {
> +		diag_set(ClientError, ER_INVALID_MSGPACK, "missing request body");
> +		return 1;
> +	}
> +	assert(row->bodycnt == 1);
> +	const char *data = (const char *) row->body[0].iov_base;
> +	const char *end = data + row->body[0].iov_len;
> +	assert((end - data) > 0);
> +
> +	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
> +error:
> +		diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
> +		return -1;
> +	}
> +
> +	uint32_t map_size = mp_decode_map(&data);
> +	request->sql_text = NULL;
> +	request->bind = NULL;
> +	request->bind_count = 0;
> +	request->sync = row->sync;
> +	for (uint32_t i = 0; i < map_size; ++i) {
> +		uint8_t key = *data;
> +		if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) {
> +			mp_check(&data, end);   /* skip the key */
> +			mp_check(&data, end);   /* skip the value */
> +			continue;
> +		}
> +		const char *value = ++data;     /* skip the key */
> +		if (mp_check(&data, end) != 0)  /* check the value */
> +			goto error;
> +		if (key == IPROTO_SQL_BIND) {
> +			if (sql_bind_list_decode(request, value, region) != 0)
> +				return -1;
> +		} else {
> +			request->sql_text =
> +				mp_decode_str(&value, &request->sql_text_len);

2. You have changed this lines:

> -                       request->sql_text = value;
>                         request->sql_text =
> -                               mp_decode_str(&request->sql_text,
> -                                             &request->sql_text_len);
> +                               mp_decode_str(&value, &request->sql_text_len);

Looks like a bug of the previous patch, and should be merged into it. I
did it.

> +		}
> +	}

===============================================================================

commit b52d916348c65d35ec09d1a9842cc0244d9e6c9a
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Mon Nov 19 17:31:27 2018 +0300

     Review fixes

diff --git a/src/box/execute.c b/src/box/execute.c
index 0cba23360..8d5caf910 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -194,7 +194,23 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet)
  	return 0;
  }
  
-int
+/**
+ * Parse MessagePack array of SQL parameters and store a result
+ * into the @request->bind, bind_count.
+ * @param request Request to save decoded parameters.
+ * @param data MessagePack array of parameters. Each parameter
+ *        either must have scalar type, or must be a map with the
+ *        following format: {name: value}. Name - string name of
+ *        the named parameter, value - scalar value of the
+ *        parameter. Named and positioned parameters can be mixed.
+ *        For more details
+ *        @sa https://www.sqlite.org/lang_expr.html#varparam.
+ * @param region Allocator.
+ *
+ * @retval  0 Success.
+ * @retval -1 Client or memory error.
+ */
+static inline int
  sql_bind_list_decode(struct sql_request *request, const char *data,
  		     struct region *region)
  {
@@ -230,6 +246,58 @@ sql_bind_list_decode(struct sql_request *request, const char *data,
  	return 0;
  }
  
+int
+xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
+		struct region *region)
+{
+	if (row->bodycnt == 0) {
+		diag_set(ClientError, ER_INVALID_MSGPACK, "missing request body");
+		return 1;
+	}
+	assert(row->bodycnt == 1);
+	const char *data = (const char *) row->body[0].iov_base;
+	const char *end = data + row->body[0].iov_len;
+	assert((end - data) > 0);
+
+	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
+error:
+		diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
+		return -1;
+	}
+
+	uint32_t map_size = mp_decode_map(&data);
+	request->sql_text = NULL;
+	request->bind = NULL;
+	request->bind_count = 0;
+	request->sync = row->sync;
+	for (uint32_t i = 0; i < map_size; ++i) {
+		uint8_t key = *data;
+		if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) {
+			mp_check(&data, end);   /* skip the key */
+			mp_check(&data, end);   /* skip the value */
+			continue;
+		}
+		const char *value = ++data;     /* skip the key */
+		if (mp_check(&data, end) != 0)  /* check the value */
+			goto error;
+		if (key == IPROTO_SQL_BIND) {
+			if (sql_bind_list_decode(request, value, region) != 0)
+				return -1;
+		} else {
+			request->sql_text =
+				mp_decode_str(&value, &request->sql_text_len);
+		}
+	}
+	if (request->sql_text == NULL) {
+		diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
+			 iproto_key_name(IPROTO_SQL_TEXT));
+		return -1;
+	}
+	if (data != end)
+		goto error;
+	return 0;
+}
+
  /**
   * Serialize a single column of a result set row.
   * @param stmt Prepared and started statement. At least one
@@ -466,15 +534,15 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
  		    int column_count)
  {
  	assert(column_count > 0);
-	char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
+	int size = mp_sizeof_uint(IPROTO_METADATA) +
+		   mp_sizeof_array(column_count);
+	char *pos = (char *) obuf_alloc(out, size);
  	if (pos == NULL) {
-		diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, "obuf_alloc",
-			 "pos");
+		diag_set(OutOfMemory, size, "obuf_alloc", "pos");
  		return -1;
  	}
-	pos = mp_store_u8(pos, IPROTO_METADATA);
-	pos = mp_store_u8(pos, 0xdd);
-	pos = mp_store_u32(pos, column_count);
+	pos = mp_encode_uint(pos, IPROTO_METADATA);
+	pos = mp_encode_array(pos, column_count);
  	for (int i = 0; i < column_count; ++i) {
  		const char *name = sqlite3_column_name(stmt, i);
  		const char *type = sqlite3_column_datatype(stmt, i);
@@ -576,15 +644,15 @@ err:
  			goto finish;
  		}
  		*keys = 2;
-		char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
+		int size = mp_sizeof_uint(IPROTO_DATA) +
+			   mp_sizeof_array(port_tuple->size);
+		char *pos = (char *) obuf_alloc(out, size);
  		if (pos == NULL) {
-			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
-				 "obuf_alloc", "pos");
+			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
  			goto err;
  		}
-		pos = mp_store_u8(pos, IPROTO_DATA);
-		pos = mp_store_u8(pos, 0xdd);
-		pos = mp_store_u32(pos, port_tuple->size);
+		pos = mp_encode_uint(pos, IPROTO_DATA);
+		pos = mp_encode_array(pos, port_tuple->size);
  		/*
  		 * Just like SELECT, SQL uses output format compatible
  		 * with Tarantool 1.6
@@ -599,19 +667,19 @@ err:
  		struct stailq *autoinc_id_list =
  			vdbe_autoinc_id_list((struct Vdbe *)stmt);
  		uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
-		char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN);
+		int size = mp_sizeof_uint(IPROTO_SQL_INFO) +
+			   mp_sizeof_map(map_size);
+		char *pos = (char *) obuf_alloc(out, size);
  		if (pos == NULL) {
-			diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN,
-				 "obuf_alloc", "pos");
+			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
  			goto err;
  		}
-		pos = mp_store_u8(pos, IPROTO_SQL_INFO);
-		pos = mp_store_u8(pos, 0xdf);
-		pos = mp_store_u32(pos, map_size);
+		pos = mp_encode_uint(pos, IPROTO_SQL_INFO);
+		pos = mp_encode_map(pos, map_size);
  		uint64_t id_count = 0;
  		int changes = db->nChange;
-		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
-			   mp_sizeof_uint(changes);
+		size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
+		       mp_sizeof_uint(changes);
  		if (!stailq_empty(autoinc_id_list)) {
  			struct autoinc_id_entry *id_entry;
  			stailq_foreach_entry(id_entry, autoinc_id_list, link) {
diff --git a/src/box/execute.h b/src/box/execute.h
index 2a242e857..5f3d5eb59 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -114,24 +114,17 @@ int
  sql_response_dump(struct sql_response *response, int *keys, struct obuf *out);
  
  /**
- * Parse MessagePack array of SQL parameters and store a result
- * into the @request->bind, bind_count.
- * @param request Request to save decoded parameters.
- * @param data MessagePack array of parameters. Each parameter
- *        either must have scalar type, or must be a map with the
- *        following format: {name: value}. Name - string name of
- *        the named parameter, value - scalar value of the
- *        parameter. Named and positioned parameters can be mixed.
- *        For more details
- *        @sa https://www.sqlite.org/lang_expr.html#varparam.
+ * Parse the EXECUTE request.
+ * @param row Encoded data.
+ * @param[out] request Request to decode to.
   * @param region Allocator.
   *
- * @retval  0 Success.
- * @retval -1 Client or memory error.
+ * @retval  0 Sucess.
+ * @retval -1 Format or memory error.
   */
  int
-sql_bind_list_decode(struct sql_request *request, const char *data,
-		     struct region *region);
+xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
+		struct region *region);
  
  /**
   * Prepare and execute an SQL statement.
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 5fb2affa3..7c11d05b3 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1118,67 +1118,6 @@ static const struct cmsg_hop error_route[] = {
  	{ net_send_error, NULL },
  };
  
-/**
- * Parse the EXECUTE request.
- * @param row Encoded data.
- * @param[out] request Request to decode to.
- * @param region Allocator.
- *
- * @retval  0 Sucess.
- * @retval -1 Format or memory error.
- */
-static inline int
-xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
-		struct region *region)
-{
-	if (row->bodycnt == 0) {
-		diag_set(ClientError, ER_INVALID_MSGPACK, "missing request body");
-		return 1;
-	}
-	assert(row->bodycnt == 1);
-	const char *data = (const char *) row->body[0].iov_base;
-	const char *end = data + row->body[0].iov_len;
-	assert((end - data) > 0);
-
-	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
-error:
-		diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
-		return -1;
-	}
-
-	uint32_t map_size = mp_decode_map(&data);
-	request->sql_text = NULL;
-	request->bind = NULL;
-	request->bind_count = 0;
-	request->sync = row->sync;
-	for (uint32_t i = 0; i < map_size; ++i) {
-		uint8_t key = *data;
-		if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) {
-			mp_check(&data, end);   /* skip the key */
-			mp_check(&data, end);   /* skip the value */
-			continue;
-		}
-		const char *value = ++data;     /* skip the key */
-		if (mp_check(&data, end) != 0)  /* check the value */
-			goto error;
-		if (key == IPROTO_SQL_BIND) {
-			if (sql_bind_list_decode(request, value, region) != 0)
-				return -1;
-		} else {
-			request->sql_text =
-				mp_decode_str(&value, &request->sql_text_len);
-		}
-	}
-	if (request->sql_text == NULL) {
-		diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
-			 iproto_key_name(IPROTO_SQL_TEXT));
-		return -1;
-	}
-	if (data != end)
-		goto error;
-	return 0;
-}
-
  static void
  iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend,
  		  bool *stop_input)
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 8a589cbd6..76c6f8166 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -445,12 +445,6 @@ iproto_prepare_header(struct obuf *buf, struct obuf_svp *svp, size_t size)
  	return 0;
  }
  
-struct PACKED iproto_key_bin {
-	uint8_t key;       /* IPROTO_DATA/METADATA/SQL_INFO */
-	uint8_t mp_type;
-	uint32_t mp_len;
-};
-
  void
  iproto_reply_select(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
  		    uint32_t schema_version, uint32_t count)
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 13071ff3b..ca8d04d44 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -52,11 +52,6 @@ enum {
  	IPROTO_HEADER_LEN = 28,
  	/** 7 = sizeof(iproto_body_bin). */
  	IPROTO_SELECT_HEADER_LEN = IPROTO_HEADER_LEN + 7,
-	/**
-	 * mp_sizeof(IPROTO_DATA/METADATA/SQL_INFO) +
-	 * mp_sizeof_array(UINT32_MAX).
-	 */
-	IPROTO_KEY_HEADER_LEN = 1 + 5,
  	/**
  	 * Header of message + header of body with one or two
  	 * keys: IPROTO_DATA and IPROTO_METADATA or

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH v1 04/10] sql: create interface vstream
  2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 04/10] sql: create interface vstream imeevma
@ 2018-11-19 17:58   ` Vladislav Shpilevoy
  0 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-19 17:58 UTC (permalink / raw)
  To: imeevma, tarantool-patches

Thanks for the patch! See 4 comments below.

On 17/11/2018 17:03, imeevma@tarantool.org wrote:
> If we want to use functions from execute.h not only in IPROTO we
> should create special interface. This interface will allow us to
> create different implementations for mpstream and lua_State and
> use functions from execute.c without changing them. This patch
> creates such interface and its implementation for mpstream and
> replaces mpstream functions in execute.c by methods of this
> interface.
> 
> Needed for #3505
> ---
>   src/box/CMakeLists.txt |   1 +
>   src/box/execute.c      |  92 +++++++++--------------
>   src/box/execute.h      |   4 +-
>   src/box/iproto.cc      |  13 +++-
>   src/box/vstream.c      | 200 +++++++++++++++++++++++++++++++++++++++++++++++++
>   src/box/vstream.h      | 191 ++++++++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 439 insertions(+), 62 deletions(-)
>   create mode 100644 src/box/vstream.c
>   create mode 100644 src/box/vstream.h
> > diff --git a/src/box/vstream.c b/src/box/vstream.c
> new file mode 100644
> index 0000000..d43c352
> --- /dev/null
> +++ b/src/box/vstream.c
> @@ -0,0 +1,200 @@
> +/*
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include "vstream.h"
> +#include "mpstream.h"
> +#include "iproto_constants.h"
> +#include "port.h"
> +#include "xrow.h"

1. The whole this file should be merged into mpstream.c.
Vstream is an 'abstract' class, it even has no vstream.c
file. Only header.

> +
> +const struct vstream_vtab mp_vstream_vtab = {
> +	/** encode_array = */ mp_vstream_encode_array,
> +	/** encode_map = */ mp_vstream_encode_map,
> +	/** encode_uint = */ mp_vstream_encode_uint,
> +	/** encode_int = */ mp_vstream_encode_int,
> +	/** encode_float = */ mp_vstream_encode_float,
> +	/** encode_double = */ mp_vstream_encode_double,
> +	/** encode_strn = */ mp_vstream_encode_strn,
> +	/** encode_nil = */ mp_vstream_encode_nil,
> +	/** encode_bool = */ mp_vstream_encode_bool,
> +	/** encode_enum = */ mp_vstream_encode_enum,
> +	/** encode_port = */ mp_vstream_encode_port,
> +	/** encode_reply_array = */ mp_vstream_encode_reply_array,
> +	/** encode_reply_map = */ mp_vstream_encode_reply_map,
> +	/** encode_array_commit = */ mp_vstream_encode_array_commit,
> +	/** encode_reply_commit = */ mp_vstream_encode_reply_commit,
> +	/** encode_map_commit = */ mp_vstream_encode_map_commit,

2. Once you did it, please, do not wrap each mpstream function
with a one line wrapper which differs in first argument pointer type
only. Most of this functions you can assign with an explicit cast.

> +};
> +
> +void
> +mp_vstream_init(struct vstream *vstream, struct mpstream *mpstream)
> +{
> +	vstream->vtab = &mp_vstream_vtab;
> +	vstream->mpstream = mpstream;
> +	vstream->is_flatten = false;
> +}
> diff --git a/src/box/vstream.h b/src/box/vstream.h
> new file mode 100644
> index 0000000..42f9813
> --- /dev/null
> +++ b/src/box/vstream.h
> @@ -0,0 +1,191 @@
> +#ifndef TARANTOOL_VSTREAM_H_INCLUDED
> +#define TARANTOOL_VSTREAM_H_INCLUDED
> +/*
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include "diag.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +struct vstream;
> +struct mpstream;
> +struct lua_State;
> +struct port;
> +
> +struct vstream_vtab {
> +	void (*encode_array)(struct vstream *stream, uint32_t size);
> +	void (*encode_map)(struct vstream *stream, uint32_t size);
> +	void (*encode_uint)(struct vstream *stream, uint64_t num);
> +	void (*encode_int)(struct vstream *stream, int64_t num);
> +	void (*encode_float)(struct vstream *stream, float num);
> +	void (*encode_double)(struct vstream *stream, double num);
> +	void (*encode_strn)(struct vstream *stream, const char *str,
> +			    uint32_t len);
> +	void (*encode_nil)(struct vstream *stream);
> +	void (*encode_bool)(struct vstream *stream, bool val);
> +	void (*encode_enum)(struct vstream *stream, int64_t num,
> +			    const char *str);
> +	int (*encode_port)(struct vstream *stream, struct port *port);
> +	int (*encode_reply_array)(struct vstream *stream, uint32_t size,
> +				  uint8_t key, const char *str);
> +	int (*encode_reply_map)(struct vstream *stream, uint32_t size,
> +				uint8_t key, const char *str);
> +	void (*encode_array_commit)(struct vstream *stream, uint32_t id);
> +	void (*encode_reply_commit)(struct vstream *stream);
> +	void (*encode_map_commit)(struct vstream *stream);

3. encode_reply_* functions should not exist. As I said earlier, you
can inline them as already existing encode_* functions.

> +};
> +
> +struct vstream {
> +	/** Virtual function table. */
> +	const struct vstream_vtab *vtab;
> +	/** TODO: Write comment. */
> +	union {
> +		struct mpstream *mpstream;
> +		struct lua_State *L;
> +	};
> +	/** TODO: Write comment. */
> +	bool is_flatten;

4. This flag should not be here. It is a part of sql_request and
sql_response as I remember. And it should be used in execute.c, not
inside streams. You will see how much simpler the code will be.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH v1 09/10] lua: create vstream implementation for Lua
  2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 09/10] lua: create vstream implementation for Lua imeevma
@ 2018-11-19 17:58   ` Vladislav Shpilevoy
  0 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-19 17:58 UTC (permalink / raw)
  To: tarantool-patches, imeevma

Thanks for the patch! See 1 comment below.

> @@ -111,6 +113,39 @@ sqlerror:
>   }
>   
>   static int
> +lbox_execute(struct lua_State *L)
> +{
> +	struct sqlite3 *db = sql_get();
> +	if (db == NULL)
> +		return luaL_error(L, "not ready");
> +
> +	size_t length;
> +	const char *sql = lua_tolstring(L, 1, &length);
> +	if (sql == NULL)
> +		return luaL_error(L, "usage: box.execute(sqlstring)");
> +
> +	struct sql_request request = {};
> +	struct sql_response response = {};

Please, do not forget about binding parameters. You should write an
analogue of sql_bind_list_decode() but in Lua. I do not know how to
virtualize decoding, so lets just write a Lua version of
sql_bind_list_decode(), it does not look too hard.

> +	request.sql_text = sql;
> +	request.sql_text_len = length;
> +	if (sql_prepare_and_execute(&request, &response, &fiber()->gc) != 0)
> +		goto sqlerror;
> +
> +	int keys;
> +	struct vstream vstream;
> +	lua_vstream_init(&vstream, L);
> +	lua_newtable(L);
> +	if (sql_response_dump(&response, &keys, &vstream) != 0) {
> +		lua_pop(L, 1);
> +		goto sqlerror;
> +	}
> +	return 1;
> +sqlerror:
> +	lua_pushstring(L, sqlite3_errmsg(db));
> +	return lua_error(L);
> +}
> +
> +static int
>   lua_sql_debug(struct lua_State *L)
>   {
>   	struct info_handler info;

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-11-19 17:58 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-17 14:03 [tarantool-patches] [PATCH v1 00/10] sql: remove box.sql.execute imeevma
2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 01/10] box: store sql text and length in sql_request imeevma
2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 02/10] iproto: remove iproto functions from execute.c imeevma
2018-11-19 17:58   ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 03/10] iproto: replace obuf by mpstream in execute.c imeevma
2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 04/10] sql: create interface vstream imeevma
2018-11-19 17:58   ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-17 14:03 ` [tarantool-patches] [PATCH v1 05/10] sql: EXPLAIN through net.box leads to SEGFAULT imeevma
2018-11-19 13:47   ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 06/10] sql: SELECT from system spaces returns unpacked msgpack imeevma
2018-11-19 13:48   ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 07/10] sql: too many autogenerated ids leads to SEGFAULT imeevma
2018-11-19 13:47   ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 08/10] box: add method dump_lua to port imeevma
2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 09/10] lua: create vstream implementation for Lua imeevma
2018-11-19 17:58   ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-17 14:04 ` [tarantool-patches] [PATCH v1 10/10] sql: check new box.sql.execute() imeevma
2018-11-19 12:54 ` [tarantool-patches] Re: [PATCH v1 00/10] sql: remove box.sql.execute Vladislav Shpilevoy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox