[tarantool-patches] [PATCH v5 0/5] sql: remove box.sql.execute

[tarantool-patches] [PATCH v5 0/5] sql: remove box.sql.execute

[imeevma]

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

This is the fifth version of patch-set. It is not complete. It
still has no last part, which is replacing box.sql.execute by
box.execute, because it will lead to massive test editing.

This version uses new design. Still, the main goal is the same as
it was for all previous versions: to look at design of new
box.sql.execute().

For now this patch-set blocked by #3832. Small temporary fix added
to temporary patch of patch-set.

https://github.com/tarantool/tarantool/issues/3505
https://github.com/tarantool/tarantool/tree/imeevma/gh-3505-no-sql-execute

General information of difference from previous version of
patch-set:
- New design: instead of creation of new stream it creates new
  implementation of port.

A bit about patches of the patch-set:

Patch 1 moves map creation from xrow functions to
sql_response_dump(). It allows us to use sql_response_dump() as
method of port.

Patch 2 creates port_sql and two its methods: dump_msgpack() and
destroy().

Patch 3 creates dump_lua method for port_sql.

Patch 4 adds binding to new implementation of new_execute().

Patch 5 is temporary patch. It was created to check that
new_execute() is able to pass through tests created for execute().

v1:
https://www.freelists.org/post/tarantool-patches/PATCH-v1-0010-sql-remove-boxsqlexecute
v2:
https://www.freelists.org/post/tarantool-patches/PATCH-v2-07-Remove-boxsqlexecute
v3:
https://www.freelists.org/post/tarantool-patches/PATCH-v3-07-Remove-boxsqlexecute
v4:
https://www.freelists.org/post/tarantool-patches/PATCH-v4-05-Remove-boxsqlexecute

Mergen Imeev (5):
  iproto: move map creation to sql_response_dump()
  iproto: create port_sql
  sql: create method dump_lua for port_sql
  sql: parameter binding for new execute()
  sql: check new box.sql.execute()

 src/box/execute.c      | 391 +++++++++++++++++++++++++++++++++++++++++++++----
 src/box/execute.h      |  75 +++++-----
 src/box/iproto.cc      |  14 +-
 src/box/lua/schema.lua |  23 +++
 src/box/lua/sql.c      |  35 ++++-
 src/box/port.h         |   1 -
 src/box/xrow.c         |   8 +-
 src/box/xrow.h         |   9 +-
 8 files changed, 462 insertions(+), 94 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v5 1/5] iproto: move map creation to sql_response_dump()

[imeevma]

Currently, function sql_response_dump() puts data into an already
created map. Moving the map creation to sql_response_dump()
simplifies the code and allows us to use sql_response_dump() as
one of the port_sql methods.

Needed for #3505
---
 src/box/execute.c | 23 ++++++++++++++++-------
 src/box/execute.h |  3 +--
 src/box/iproto.cc |  8 +++-----
 src/box/xrow.c    |  8 +-------
 src/box/xrow.h    |  9 +--------
 5 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 7fff5fd..38b6cbc 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -555,22 +555,29 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 }
 
 int
-sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
+sql_response_dump(struct sql_response *response, struct obuf *out)
 {
 	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) {
+		int keys = 2;
+		int size = mp_sizeof_map(keys);
+		char *pos = (char *) obuf_alloc(out, size);
+		if (pos == NULL) {
+			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
+			goto err;
+		}
+		pos = mp_encode_map(pos, keys);
 		if (sql_get_description(stmt, out, column_count) != 0) {
 err:
 			rc = -1;
 			goto finish;
 		}
-		*keys = 2;
-		int size = mp_sizeof_uint(IPROTO_DATA) +
-			   mp_sizeof_array(port_tuple->size);
-		char *pos = (char *) obuf_alloc(out, size);
+		size = mp_sizeof_uint(IPROTO_DATA) +
+		       mp_sizeof_array(port_tuple->size);
+		pos = (char *) obuf_alloc(out, size);
 		if (pos == NULL) {
 			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
 			goto err;
@@ -586,18 +593,20 @@ err:
 			goto err;
 		}
 	} else {
-		*keys = 1;
+		int 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;
-		int size = mp_sizeof_uint(IPROTO_SQL_INFO) +
+		int size = mp_sizeof_map(keys) +
+			   mp_sizeof_uint(IPROTO_SQL_INFO) +
 			   mp_sizeof_map(map_size);
 		char *pos = (char *) obuf_alloc(out, size);
 		if (pos == NULL) {
 			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
 			goto err;
 		}
+		pos = mp_encode_map(pos, keys);
 		pos = mp_encode_uint(pos, IPROTO_SQL_INFO);
 		pos = mp_encode_map(pos, map_size);
 		uint64_t id_count = 0;
diff --git a/src/box/execute.h b/src/box/execute.h
index 9c1bc4f..60b8f31 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -105,14 +105,13 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
  * | }                                            |
  * +----------------------------------------------+
  * @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, int *keys, struct obuf *out);
+sql_response_dump(struct sql_response *response, struct obuf *out);
 
 /**
  * Prepare and execute an SQL statement.
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 3edfd8f..e179de3 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1638,17 +1638,15 @@ tx_process_sql(struct cmsg *m)
 	 * become out of date during yield.
 	 */
 	out = msg->connection->tx.p_obuf;
-	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)
+	if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
 		goto error;
-	if (sql_response_dump(&response, &keys, out) != 0) {
+	if (sql_response_dump(&response, out) != 0) {
 		obuf_rollback_to_svp(out, &header_svp);
 		goto error;
 	}
-	iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version,
-			 keys);
+	iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
 	iproto_wpos_create(&msg->wpos, out);
 	return;
 error:
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 67019a6..c4e3073 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -508,17 +508,11 @@ error:
 
 void
 iproto_reply_sql(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
-		 uint32_t schema_version, int keys)
+		 uint32_t schema_version)
 {
 	char *pos = (char *) obuf_svp_to_ptr(buf, svp);
 	iproto_header_encode(pos, IPROTO_OK, sync, schema_version,
 			     obuf_size(buf) - svp->used - IPROTO_HEADER_LEN);
-	/*
-	 * MessagePack encodes value <= 15 as
-	 * bitwise OR with 0x80.
-	 */
-	assert(keys <= 15);
-	*(pos + IPROTO_HEADER_LEN) = 0x80 | keys;
 }
 
 void
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 6bab0a1..2654e35 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -52,12 +52,6 @@ enum {
 	IPROTO_HEADER_LEN = 28,
 	/** 7 = sizeof(iproto_body_bin). */
 	IPROTO_SELECT_HEADER_LEN = IPROTO_HEADER_LEN + 7,
-	/**
-	 * Header of message + header of body with one or two
-	 * keys: IPROTO_DATA and IPROTO_METADATA or
-	 * IPROTO_SQL_INFO. 1 == mp_sizeof_map(<=15).
-	 */
-	IPROTO_SQL_HEADER_LEN = IPROTO_HEADER_LEN + 1,
 };
 
 struct xrow_header {
@@ -491,11 +485,10 @@ xrow_decode_sql(const struct xrow_header *row, struct sql_request *request);
  * @param svp Savepoint of the header beginning.
  * @param sync Request sync.
  * @param schema_version Schema version.
- * @param keys Count of keys in the body.
  */
 void
 iproto_reply_sql(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
-		 uint32_t schema_version, int keys);
+		 uint32_t schema_version);
 
 /**
  * Write an IPROTO_CHUNK header from a specified position in a
-- 
2.7.4

[tarantool-patches] [PATCH v5 2/5] iproto: create port_sql

[imeevma]

This patch creates port_sql implementation for the port. This will
allow us to dump sql responses to obuf or to Lua. Also this patch
defines methods dump_msgpack() and destroy() of port_sql.

Part of #3505
---
 src/box/execute.c | 90 ++++++++++++++++++++++++++++++++++++++++++-------------
 src/box/execute.h | 67 +++++++++++++++--------------------------
 src/box/iproto.cc |  6 ++--
 src/box/port.h    |  1 -
 4 files changed, 96 insertions(+), 68 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 38b6cbc..b07de28 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -530,7 +530,7 @@ sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
 
 int
 sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
-			uint32_t bind_count, struct sql_response *response,
+			uint32_t bind_count, struct port *port,
 			struct region *region)
 {
 	struct sqlite3_stmt *stmt;
@@ -544,22 +544,56 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 		return -1;
 	}
 	assert(stmt != NULL);
-	port_tuple_create(&response->port);
-	response->prep_stmt = stmt;
+	port_tuple_create(port);
 	if (sql_bind(stmt, bind, bind_count) == 0 &&
-	    sql_execute(db, stmt, &response->port, region) == 0)
+	    sql_execute(db, stmt, port, region) == 0) {
+		port_tuple_to_port_sql(port, stmt);
 		return 0;
-	port_destroy(&response->port);
+	}
+	port_destroy(port);
 	sqlite3_finalize(stmt);
 	return -1;
 }
 
-int
-sql_response_dump(struct sql_response *response, struct obuf *out)
+/**
+ * Dump a built response into @an out buffer. The response is
+ * destroyed.
+ * Response structure:
+ * +----------------------------------------------+
+ * | IPROTO_OK, sync, schema_version   ...        | iproto_header
+ * +----------------------------------------------+---------------
+ * | Body - a map with one or two keys.           |
+ * |                                              |
+ * | IPROTO_BODY: {                               |
+ * |     IPROTO_METADATA: [                       |
+ * |         {IPROTO_FIELD_NAME: column name1},   |
+ * |         {IPROTO_FIELD_NAME: column name2},   | iproto_body
+ * |         ...                                  |
+ * |     ],                                       |
+ * |                                              |
+ * |     IPROTO_DATA: [                           |
+ * |         tuple, tuple, tuple, ...             |
+ * |     ]                                        |
+ * | }                                            |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: {                               |
+ * |     IPROTO_SQL_INFO: {                       |
+ * |         SQL_INFO_ROW_COUNT: number           |
+ * |     }                                        |
+ * | }                                            |
+ * +----------------------------------------------+
+ * @param port port with EXECUTE response.
+ * @param out Output buffer.
+ *
+ * @retval  0 Success.
+ * @retval -1 Memory error.
+ */
+static int
+port_sql_dump_msgpack(struct port *port, struct obuf *out)
 {
+	assert(port->vtab == &port_sql_vtab);
 	sqlite3 *db = sql_get();
-	struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
-	struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
+	struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
 	int rc = 0, column_count = sqlite3_column_count(stmt);
 	if (column_count > 0) {
 		int keys = 2;
@@ -575,26 +609,18 @@ err:
 			rc = -1;
 			goto finish;
 		}
-		size = mp_sizeof_uint(IPROTO_DATA) +
-		       mp_sizeof_array(port_tuple->size);
+		size = mp_sizeof_uint(IPROTO_DATA);
 		pos = (char *) obuf_alloc(out, size);
 		if (pos == NULL) {
 			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
 			goto err;
 		}
 		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
-		 */
-		if (port_dump_msgpack_16(&response->port, out) < 0) {
-			/* Failed port dump destroyes the port. */
+		if (port_tuple_vtab.dump_msgpack(port, out) < 0)
 			goto err;
-		}
 	} else {
 		int keys = 1;
-		assert(port_tuple->size == 0);
+		assert(((struct port_tuple *)port)->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;
@@ -643,7 +669,29 @@ err:
 		}
 	}
 finish:
-	port_destroy(&response->port);
+	port_destroy(port);
 	sqlite3_finalize(stmt);
 	return rc;
 }
+
+static void
+port_sql_destroy(struct port *base)
+{
+	port_tuple_vtab.destroy(base);
+}
+
+void
+port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt)
+{
+	assert(port->vtab == &port_tuple_vtab);
+	((struct port_sql *)port)->stmt = stmt;
+	port->vtab = &port_sql_vtab;
+}
+
+const struct port_vtab port_sql_vtab = {
+	.dump_msgpack = port_sql_dump_msgpack,
+	.dump_msgpack_16 = NULL,
+	.dump_lua = NULL,
+	.dump_plain = NULL,
+	.destroy = port_sql_destroy,
+};
diff --git a/src/box/execute.h b/src/box/execute.h
index 60b8f31..5aef546 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -51,14 +51,31 @@ extern const char *sql_info_key_strs[];
 struct obuf;
 struct region;
 struct sql_bind;
+struct sqlite3_stmt;
 
-/** Response on EXECUTE request. */
-struct sql_response {
-	/** Port with response data if any. */
-	struct port port;
-	/** Prepared SQL statement with metadata. */
-	void *prep_stmt;
+/**
+ * Port implementation used for dump tuples, stored in port_tuple,
+ * to obuf or Lua.
+ */
+struct port_sql {
+	/* port_tuple to inherit from. */
+	struct port_tuple port_tuple;
+	/* Prepared SQL statement. */
+	struct sqlite3_stmt *stmt;
 };
+static_assert(sizeof(struct port_sql) <= sizeof(struct port),
+	      "sizeof(struct port_sql) must be <= sizeof(struct port)");
+
+extern const struct port_vtab port_sql_vtab;
+
+/**
+ * Transform port_tuple with already stored tuples to port_sql
+ * that will dump these tuples into obut or Lua.
+ *
+ * @param port port_tuple to transform into port_sql.
+ */
+void
+port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt);
 
 /**
  * Parse MessagePack array of SQL parameters.
@@ -78,42 +95,6 @@ int
 sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
 
 /**
- * Dump a built response into @an out buffer. The response is
- * destroyed.
- * Response structure:
- * +----------------------------------------------+
- * | IPROTO_OK, sync, schema_version   ...        | iproto_header
- * +----------------------------------------------+---------------
- * | Body - a map with one or two keys.           |
- * |                                              |
- * | IPROTO_BODY: {                               |
- * |     IPROTO_METADATA: [                       |
- * |         {IPROTO_FIELD_NAME: column name1},   |
- * |         {IPROTO_FIELD_NAME: column name2},   | iproto_body
- * |         ...                                  |
- * |     ],                                       |
- * |                                              |
- * |     IPROTO_DATA: [                           |
- * |         tuple, tuple, tuple, ...             |
- * |     ]                                        |
- * | }                                            |
- * +-------------------- OR ----------------------+
- * | IPROTO_BODY: {                               |
- * |     IPROTO_SQL_INFO: {                       |
- * |         SQL_INFO_ROW_COUNT: number           |
- * |     }                                        |
- * | }                                            |
- * +----------------------------------------------+
- * @param response EXECUTE response.
- * @param out Output buffer.
- *
- * @retval  0 Success.
- * @retval -1 Memory error.
- */
-int
-sql_response_dump(struct sql_response *response, struct obuf *out);
-
-/**
  * Prepare and execute an SQL statement.
  * @param sql SQL statement.
  * @param len Length of @a sql.
@@ -128,7 +109,7 @@ sql_response_dump(struct sql_response *response, struct obuf *out);
  */
 int
 sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
-			uint32_t bind_count, struct sql_response *response,
+			uint32_t bind_count, struct port *port,
 			struct region *region);
 
 #if defined(__cplusplus)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index e179de3..ee704e0 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1613,7 +1613,7 @@ tx_process_sql(struct cmsg *m)
 {
 	struct iproto_msg *msg = tx_accept_msg(m);
 	struct obuf *out;
-	struct sql_response response;
+	struct port port;
 	struct sql_bind *bind;
 	int bind_count;
 	const char *sql;
@@ -1630,7 +1630,7 @@ tx_process_sql(struct cmsg *m)
 		goto error;
 	sql = msg->sql.sql_text;
 	sql = mp_decode_str(&sql, &len);
-	if (sql_prepare_and_execute(sql, len, bind, bind_count, &response,
+	if (sql_prepare_and_execute(sql, len, bind, bind_count, &port,
 				    &fiber()->gc) != 0)
 		goto error;
 	/*
@@ -1642,7 +1642,7 @@ tx_process_sql(struct cmsg *m)
 	/* Prepare memory for the iproto header. */
 	if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
 		goto error;
-	if (sql_response_dump(&response, out) != 0) {
+	if (port_dump_msgpack(&port, out) != 0) {
 		obuf_rollback_to_svp(out, &header_svp);
 		goto error;
 	}
diff --git a/src/box/port.h b/src/box/port.h
index ad1b349..f188036 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -65,7 +65,6 @@ extern const struct port_vtab port_tuple_vtab;
 static inline struct port_tuple *
 port_tuple(struct port *port)
 {
-	assert(port->vtab == &port_tuple_vtab);
 	return (struct port_tuple *)port;
 }
 
-- 
2.7.4

[tarantool-patches] [PATCH v5 3/5] sql: create method dump_lua for port_sql

[imeevma]

This patch creates the dump_lua method for port_sql. It also
defines a new function box.sql.new_execute(), which will be
converted to box.sql.execute().

Part of #3505
---
 src/box/execute.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/box/lua/sql.c | 25 +++++++++++++++++
 2 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index b07de28..3cbee4f 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -43,6 +43,7 @@
 #include "port.h"
 #include "tuple.h"
 #include "sql/vdbe.h"
+#include "lua/utils.h"
 
 const char *sql_type_strs[] = {
 	NULL,
@@ -674,6 +675,87 @@ finish:
 	return rc;
 }
 
+/**
+ * Push a metadata of the prepared statement to Lua stack.
+ *
+ * @param stmt Prepared statement.
+ * @param L Lua stack.
+ * @param column_count Statement's column count.
+ */
+static inline void
+lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
+			int column_count)
+{
+	assert(column_count > 0);
+	lua_createtable(L, column_count, 0);
+	for (int i = 0; i < column_count; ++i) {
+		lua_createtable(L, 2, 0);
+		const char *name = sqlite3_column_name(stmt, i);
+		const char *type = sqlite3_column_datatype(stmt, i);
+		/*
+		 * Can not fail, since all column names are
+		 * preallocated during prepare phase and the
+		 * column_name simply returns them.
+		 */
+		assert(name != NULL);
+		lua_pushlstring(L, name, strlen(name));
+		lua_setfield(L, -2, "name");
+		lua_pushlstring(L, type, strlen(type));
+		lua_setfield(L, -2, "type");
+		lua_rawseti(L, -2, i + 1);
+	}
+	lua_setfield(L, -2, "metadata");
+}
+
+/**
+ * Dump a built response into Lua stack. The response is
+ * destroyed.
+ *
+ * @param port port with EXECUTE response.
+ * @param L Lua stack.
+ */
+static void
+port_sql_dump_lua(struct port *port, struct lua_State *L)
+{
+	assert(port->vtab == &port_sql_vtab);
+	sqlite3 *db = sql_get();
+	struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
+	int column_count = sqlite3_column_count(stmt);
+	if (column_count > 0) {
+		int keys = 2;
+		lua_createtable(L, 0, keys);
+		lua_sql_get_description(stmt, L, column_count);
+		port_tuple_vtab.dump_lua(port, L);
+		lua_setfield(L, -2, "rows");
+	} else {
+		assert(((struct port_tuple *)port)->size == 0);
+		struct stailq *autoinc_id_list =
+			vdbe_autoinc_id_list((struct Vdbe *)stmt);
+
+		int dynamic_keys = stailq_empty(autoinc_id_list) ? 0 : 1;
+		lua_createtable(L, 1, dynamic_keys);
+
+		luaL_pushuint64(L, db->nChange);
+		lua_setfield(L, -2, "rowcount");
+
+		if (!stailq_empty(autoinc_id_list)) {
+			lua_newtable(L);
+			int i = 1;
+			struct autoinc_id_entry *id_entry;
+			stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+				if (id_entry->id >= 0)
+					luaL_pushuint64(L, id_entry->id);
+				else
+					luaL_pushint64(L, id_entry->id);
+				lua_rawseti(L, -2, i++);
+			}
+			lua_setfield(L, -2, "autoincrement_ids");
+		}
+	}
+	port_destroy(port);
+	sqlite3_finalize(stmt);
+}
+
 static void
 port_sql_destroy(struct port *base)
 {
@@ -691,7 +773,7 @@ port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt)
 const struct port_vtab port_sql_vtab = {
 	.dump_msgpack = port_sql_dump_msgpack,
 	.dump_msgpack_16 = NULL,
-	.dump_lua = NULL,
+	.dump_lua = port_sql_dump_lua,
 	.dump_plain = NULL,
 	.destroy = port_sql_destroy,
 };
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 6923666..318f331 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -6,6 +6,7 @@
 #include <info.h>
 #include "lua/info.h"
 #include "lua/utils.h"
+#include "box/execute.h"
 
 static void
 lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
@@ -111,6 +112,29 @@ sqlerror:
 }
 
 static int
+lbox_execute(struct lua_State *L)
+{
+	struct sql_bind *bind = NULL;
+	int bind_count = 0;
+	size_t length;
+	struct port port;
+
+	struct sqlite3 *db = sql_get();
+	if (db == NULL)
+		return luaL_error(L, "not ready");
+
+	const char *sql = lua_tolstring(L, 1, &length);
+	if (sql == NULL)
+		return luaL_error(L, "usage: box.execute(sqlstring)");
+
+	if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
+				    &fiber()->gc) != 0)
+		return luaT_error(L);
+	port_dump_lua(&port, L);
+	return 1;
+}
+
+static int
 lua_sql_debug(struct lua_State *L)
 {
 	struct info_handler info;
@@ -124,6 +148,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}
 	};
-- 
2.7.4

[tarantool-patches] [PATCH v5 4/5] sql: parameter binding for new execute()

[imeevma]

This patch defines parameters binding for SQL statements executed
through box.

Part of #3505
Closes #3401
---
 src/box/execute.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/box/execute.h |  15 +++++
 src/box/lua/sql.c |   4 ++
 3 files changed, 209 insertions(+)

diff --git a/src/box/execute.c b/src/box/execute.c
index 3cbee4f..f538441 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -44,6 +44,7 @@
 #include "tuple.h"
 #include "sql/vdbe.h"
 #include "lua/utils.h"
+#include "lua/msgpack.h"
 
 const char *sql_type_strs[] = {
 	NULL,
@@ -229,6 +230,195 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind)
 	return bind_count;
 }
 
+
+/**
+ * Decode a single bind column from Lua stack.
+ *
+ * @param L Lua stack.
+ * @param[out] bind Bind to decode to.
+ * @param idx Position of table with bind columns on Lua stack.
+ * @param i Ordinal bind number.
+ *
+ * @retval  0 Success.
+ * @retval -1 Memory or client error.
+ */
+static inline int
+lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
+{
+	struct luaL_field field;
+	char *buf;
+	lua_rawgeti(L, idx, i + 1);
+	luaL_tofield(L, luaL_msgpack_default, -1, &field);
+	bind->pos = i + 1;
+	if (field.type == MP_MAP) {
+		/*
+		 * A named parameter is an MP_MAP with
+		 * one key - {'name': value}.
+		 * Report parse error otherwise.
+		 */
+		if (field.size != 1) {
+			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
+				 "parameter should be {'name': value}");
+			return -1;
+		}
+		/*
+		 * Get key and value of the only map element to
+		 * lua stack.
+		 */
+		lua_pushnil(L);
+		lua_next(L, lua_gettop(L) - 1);
+		/* At first we should deal with the value. */
+		luaL_tofield(L, luaL_msgpack_default, -1, &field);
+		lua_pop(L, 1);
+		/* Now key is on the top of Lua stack. */
+		size_t name_len = 0;
+		bind->name = luaL_checklstring(L, -1, &name_len);
+		if (bind->name == NULL) {
+			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
+				 "parameter should be {'name': value}");
+			return -1;
+		}
+		/*
+		 * Name should be saved in allocated memory as it
+		 * will be poped from Lua stack.
+		 */
+		buf = region_alloc(&fiber()->gc, name_len + 1);
+		if (buf == NULL) {
+			diag_set(OutOfMemory, name_len + 1, "region_alloc",
+				 "buf");
+			return -1;
+		}
+		memcpy(buf, bind->name, name_len + 1);
+		bind->name = buf;
+		bind->name_len = name_len;
+		lua_pop(L, 1);
+	} else {
+		bind->name = NULL;
+		bind->name_len = 0;
+	}
+	switch (field.type) {
+	case MP_UINT: {
+		bind->i64 = field.ival;
+		bind->type = SQLITE_INTEGER;
+		bind->bytes = sizeof(bind->i64);
+		break;
+	}
+	case MP_INT:
+		bind->i64 = field.ival;
+		bind->type = SQLITE_INTEGER;
+		bind->bytes = sizeof(bind->i64);
+		break;
+	case MP_STR:
+		/*
+		 * Data should be saved in allocated memory as it
+		 * will be poped from Lua stack.
+		 */
+		buf = region_alloc(&fiber()->gc, field.sval.len + 1);
+		if (buf == NULL) {
+			diag_set(OutOfMemory, field.sval.len + 1,
+				 "region_alloc", "buf");
+			return -1;
+		}
+		memcpy(buf, field.sval.data, field.sval.len + 1);
+		bind->s = buf;
+		bind->type = SQLITE_TEXT;
+		bind->bytes = field.sval.len;
+		break;
+	case MP_DOUBLE:
+		bind->d = field.dval;
+		bind->type = SQLITE_FLOAT;
+		bind->bytes = sizeof(bind->d);
+		break;
+	case MP_FLOAT:
+		bind->d = field.dval;
+		bind->type = SQLITE_FLOAT;
+		bind->bytes = sizeof(bind->d);
+		break;
+	case MP_NIL:
+		bind->type = SQLITE_NULL;
+		bind->bytes = 1;
+		break;
+	case MP_BOOL:
+		/* SQLite doesn't support boolean. Use int instead. */
+		bind->i64 = field.bval ? 1 : 0;
+		bind->type = SQLITE_INTEGER;
+		bind->bytes = sizeof(bind->i64);
+		break;
+	case MP_BIN:
+		bind->s = mp_decode_bin(&field.sval.data, &bind->bytes);
+		bind->type = SQLITE_BLOB;
+		break;
+	case MP_EXT:
+		/*
+		 * Data should be saved in allocated memory as it
+		 * will be poped from Lua stack.
+		 */
+		buf = region_alloc(&fiber()->gc, sizeof(field));
+		if (buf == NULL) {
+			diag_set(OutOfMemory, sizeof(field), "region_alloc",
+				 "buf");
+			return -1;
+		}
+		memcpy(buf, &field, sizeof(field));
+		bind->s = buf;
+		bind->bytes = sizeof(field);
+		bind->type = SQLITE_BLOB;
+		break;
+	case MP_ARRAY:
+		diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY",
+			 sql_bind_name(bind));
+		return -1;
+	case MP_MAP:
+		diag_set(ClientError, ER_SQL_BIND_TYPE, "MAP",
+			 sql_bind_name(bind));
+		return -1;
+	default:
+		unreachable();
+	}
+	lua_pop(L, 1);
+	return 0;
+}
+
+int
+lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
+			 int idx)
+{
+	assert(out_bind != NULL);
+	if (! lua_istable(L, idx)) {
+		diag_set(ClientError, ER_INVALID_MSGPACK, "SQL parameter list");
+		return -1;
+	}
+	uint32_t bind_count = lua_objlen(L, idx);
+	if (bind_count == 0)
+		return 0;
+	if (bind_count > SQL_BIND_PARAMETER_MAX) {
+		diag_set(ClientError, ER_SQL_BIND_PARAMETER_MAX,
+			 (int) bind_count);
+		return -1;
+	}
+	struct region *region = &fiber()->gc;
+	uint32_t used = region_used(region);
+	size_t size = sizeof(struct sql_bind) * bind_count;
+	/*
+	 * Memory allocated here will be freed in
+	 * sqlite3_finalize() or in txn_commit()/txn_rollback() if
+	 * there is an active transaction.
+	 */
+	struct sql_bind *bind = (struct sql_bind *) region_alloc(region, size);
+	if (bind == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "bind");
+		return -1;
+	}
+	for (uint32_t i = 0; i < bind_count; ++i) {
+		if (lua_sql_bind_decode(L, &bind[i], idx, i) != 0) {
+			region_truncate(region, used);
+			return -1;
+		}
+	}
+	*out_bind = bind;
+	return bind_count;
+}
+
 /**
  * Serialize a single column of a result set row.
  * @param stmt Prepared and started statement. At least one
diff --git a/src/box/execute.h b/src/box/execute.h
index 5aef546..bc2d6a5 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -95,6 +95,21 @@ int
 sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
 
 /**
+ * Parse Lua table of SQL parameters and store a result
+ * into the @request->bind, bind_count.
+ *
+ * @param L Lua stack to get data from.
+ * @param[out] out_bind Pointer to save decoded parameters.
+ * @param idx Position of table with parameters on Lua stack.
+ *
+ * @retval  >= 0 Number of decoded parameters.
+ * @retval -1 Client or memory error.
+ */
+int
+lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
+			 int idx);
+
+/**
  * Prepare and execute an SQL statement.
  * @param sql SQL statement.
  * @param len Length of @a sql.
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 318f331..354321f 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -127,6 +127,10 @@ lbox_execute(struct lua_State *L)
 	if (sql == NULL)
 		return luaL_error(L, "usage: box.execute(sqlstring)");
 
+	if (lua_gettop(L) == 2 &&
+	    (bind_count = lua_sql_bind_list_decode(L, &bind, 2)) < 0)
+		return luaT_error(L);
+
 	if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
 				    &fiber()->gc) != 0)
 		return luaT_error(L);
-- 
2.7.4

[tarantool-patches] [PATCH v5 5/5] sql: check new box.sql.execute()

[imeevma]

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. Even though this commit is temporary it shows that
this patch-set should be pushed after patch for issue #3832.

Needed for #3505
---
 src/box/execute.c      | 14 +++++++++-----
 src/box/execute.h      |  2 +-
 src/box/iproto.cc      |  2 +-
 src/box/lua/schema.lua | 23 +++++++++++++++++++++++
 src/box/lua/sql.c      | 12 ++++++++----
 5 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index f538441..8a70bc4 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -678,6 +678,8 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
 		 * column_name simply returns them.
 		 */
 		assert(name != NULL);
+		if (type == NULL)
+			type = "UNKNOWN";
 		size += mp_sizeof_str(strlen(name));
 		size += mp_sizeof_str(strlen(type));
 		char *pos = (char *) obuf_alloc(out, size);
@@ -696,7 +698,7 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
 
 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) {
@@ -713,7 +715,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;
@@ -722,7 +724,7 @@ sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
 int
 sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 			uint32_t bind_count, struct port *port,
-			struct region *region)
+			struct region *region, int error_id)
 {
 	struct sqlite3_stmt *stmt;
 	sqlite3 *db = sql_get();
@@ -731,13 +733,13 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 		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);
 	port_tuple_create(port);
 	if (sql_bind(stmt, bind, bind_count) == 0 &&
-	    sql_execute(db, stmt, port, region) == 0) {
+	    sql_execute(db, stmt, port, region, error_id) == 0) {
 		port_tuple_to_port_sql(port, stmt);
 		return 0;
 	}
@@ -888,6 +890,8 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
 		 * column_name simply returns them.
 		 */
 		assert(name != NULL);
+		if (type == NULL)
+			type = "UNKNOWN";
 		lua_pushlstring(L, name, strlen(name));
 		lua_setfield(L, -2, "name");
 		lua_pushlstring(L, type, strlen(type));
diff --git a/src/box/execute.h b/src/box/execute.h
index bc2d6a5..81f96bf 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -125,7 +125,7 @@ lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
 int
 sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 			uint32_t bind_count, struct port *port,
-			struct region *region);
+			struct region *region, int error_id);
 
 #if defined(__cplusplus)
 } /* extern "C" { */
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index ee704e0..fe890e7 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1631,7 +1631,7 @@ tx_process_sql(struct cmsg *m)
 	sql = msg->sql.sql_text;
 	sql = mp_decode_str(&sql, &len);
 	if (sql_prepare_and_execute(sql, len, bind, bind_count, &port,
-				    &fiber()->gc) != 0)
+				    &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 354321f..2a2e566 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -129,13 +129,17 @@ lbox_execute(struct lua_State *L)
 
 	if (lua_gettop(L) == 2 &&
 	    (bind_count = lua_sql_bind_list_decode(L, &bind, 2)) < 0)
-		return luaT_error(L);
+		goto sqlerror;
 
 	if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
-				    &fiber()->gc) != 0)
-		return luaT_error(L);
+				    &fiber()->gc, ER_SYSTEM) != 0)
+		goto sqlerror;
 	port_dump_lua(&port, L);
 	return 1;
+
+sqlerror:
+	lua_pushstring(L, sqlite3_errmsg(db));
+	return lua_error(L);
 }
 
 static int
@@ -151,7 +155,7 @@ void
 box_lua_sqlite_init(struct lua_State *L)
 {
 	static const struct luaL_Reg module_funcs [] = {
-		{"execute", lua_sql_execute},
+		{"old_execute", lua_sql_execute},
 		{"new_execute", lbox_execute},
 		{"debug", lua_sql_debug},
 		{NULL, NULL}
-- 
2.7.4

[tarantool-patches] Re: [PATCH v5 3/5] sql: create method dump_lua for port_sql

[Vladislav Shpilevoy]

Thanks for the patch! See 7 comments below.

On 22/12/2018 14:31, imeevma@tarantool.org wrote:
> This patch creates the dump_lua method for port_sql. It also
> defines a new function box.sql.new_execute(), which will be
> converted to box.sql.execute().
> 
> Part of #3505
> ---
>   src/box/execute.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   src/box/lua/sql.c | 25 +++++++++++++++++
>   2 files changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index b07de28..3cbee4f 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -674,6 +675,87 @@ finish:
>   	return rc;
>   }
>   
> +/**
> + * Push a metadata of the prepared statement to Lua stack.
> + *
> + * @param stmt Prepared statement.
> + * @param L Lua stack.
> + * @param column_count Statement's column count.
> + */
> +static inline void
> +lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
> +			int column_count)
> +{
> +	assert(column_count > 0);
> +	lua_createtable(L, column_count, 0);
> +	for (int i = 0; i < column_count; ++i) {
> +		lua_createtable(L, 2, 0);
> +		const char *name = sqlite3_column_name(stmt, i);
> +		const char *type = sqlite3_column_datatype(stmt, i);
> +		/*
> +		 * Can not fail, since all column names are
> +		 * preallocated during prepare phase and the
> +		 * column_name simply returns them.
> +		 */
> +		assert(name != NULL);
> +		lua_pushlstring(L, name, strlen(name));
> +		lua_setfield(L, -2, "name");
> +		lua_pushlstring(L, type, strlen(type));

1. Use lua_pushstring.

> +		lua_setfield(L, -2, "type");
> +		lua_rawseti(L, -2, i + 1);
> +	}
> +	lua_setfield(L, -2, "metadata");

2. It is strange, that this function creates a table and sets it
as a member of some externally created table. Please, move this line
to a caller function.

> +}
> +
> +/**
> + * Dump a built response into Lua stack. The response is
> + * destroyed.
> + *
> + * @param port port with EXECUTE response.
> + * @param L Lua stack.
> + */
> +static void
> +port_sql_dump_lua(struct port *port, struct lua_State *L)
> +{
> +	assert(port->vtab == &port_sql_vtab);
> +	sqlite3 *db = sql_get();
> +	struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
> +	int column_count = sqlite3_column_count(stmt);
> +	if (column_count > 0) {
> +		int keys = 2;
> +		lua_createtable(L, 0, keys);

3. Just inline keys 2.

> +		lua_sql_get_description(stmt, L, column_count);
> +		port_tuple_vtab.dump_lua(port, L);
> +		lua_setfield(L, -2, "rows");
> +	} else {
> +		assert(((struct port_tuple *)port)->size == 0);
> +		struct stailq *autoinc_id_list =
> +			vdbe_autoinc_id_list((struct Vdbe *)stmt);
> +
> +		int dynamic_keys = stailq_empty(autoinc_id_list) ? 0 : 1;
> +		lua_createtable(L, 1, dynamic_keys);

4. Why 1 array element? And why do you separate fixed/optional keys?
lua_createtable does not take statis/dynamic key count. It takes
array/map key count.

> +
> +		luaL_pushuint64(L, db->nChange);
> +		lua_setfield(L, -2, "rowcount");
> +
> +		if (!stailq_empty(autoinc_id_list)) {
> +			lua_newtable(L);
> +			int i = 1;
> +			struct autoinc_id_entry *id_entry;
> +			stailq_foreach_entry(id_entry, autoinc_id_list, link) {
> +				if (id_entry->id >= 0)
> +					luaL_pushuint64(L, id_entry->id);
> +				else
> +					luaL_pushint64(L, id_entry->id);
> +				lua_rawseti(L, -2, i++);
> +			}
> +			lua_setfield(L, -2, "autoincrement_ids");
> +		}
> +	}
> +	port_destroy(port);
> +	sqlite3_finalize(stmt);

5. I think, port_sql_destroy should call sqlite3_finalize.

> +}
> +
>   static void
>   port_sql_destroy(struct port *base)
>   {
> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
> index 6923666..318f331 100644
> --- a/src/box/lua/sql.c
> +++ b/src/box/lua/sql.c
> @@ -111,6 +112,29 @@ sqlerror:
>   }
>   
>   static int
> +lbox_execute(struct lua_State *L)
> +{
> +	struct sql_bind *bind = NULL;
> +	int bind_count = 0;
> +	size_t length;
> +	struct port port;
> +
> +	struct sqlite3 *db = sql_get();
> +	if (db == NULL)
> +		return luaL_error(L, "not ready");

6. Impossible. It isn't?

> +
> +	const char *sql = lua_tolstring(L, 1, &length);
> +	if (sql == NULL)
> +		return luaL_error(L, "usage: box.execute(sqlstring)");

7. Please, start message with a capital letter.

> +
> +	if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
> +				    &fiber()->gc) != 0)
> +		return luaT_error(L);
> +	port_dump_lua(&port, L);
> +	return 1;
> +}
> +
> +static int
>   lua_sql_debug(struct lua_State *L)
>   {
>   	struct info_handler info;

[tarantool-patches] Re: [PATCH v5 2/5] iproto: create port_sql

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 4 comments below.

On 22/12/2018 14:31, imeevma@tarantool.org wrote:
> This patch creates port_sql implementation for the port. This will
> allow us to dump sql responses to obuf or to Lua. Also this patch
> defines methods dump_msgpack() and destroy() of port_sql.
> 
> Part of #3505
> ---
>   src/box/execute.c | 90 ++++++++++++++++++++++++++++++++++++++++++-------------
>   src/box/execute.h | 67 +++++++++++++++--------------------------
>   src/box/iproto.cc |  6 ++--
>   src/box/port.h    |  1 -
>   4 files changed, 96 insertions(+), 68 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 38b6cbc..b07de28 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -575,26 +609,18 @@ err:
>   			rc = -1;
>   			goto finish;
>   		}
> -		size = mp_sizeof_uint(IPROTO_DATA) +
> -		       mp_sizeof_array(port_tuple->size);
> +		size = mp_sizeof_uint(IPROTO_DATA);
>   		pos = (char *) obuf_alloc(out, size);
>   		if (pos == NULL) {
>   			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
>   			goto err;
>   		}
>   		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
> -		 */
> -		if (port_dump_msgpack_16(&response->port, out) < 0) {
> -			/* Failed port dump destroyes the port. */
> +		if (port_tuple_vtab.dump_msgpack(port, out) < 0)

1. Please, write a comment about how could you use port_tuple_vtab on
port_sql. Here I expected to see words about calling methods of a base
structure, like BaseClass::method in C++.

>   			goto err;
> -		}
>   	} else {
>   		int keys = 1;
> -		assert(port_tuple->size == 0);
> +		assert(((struct port_tuple *)port)->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;
> @@ -643,7 +669,29 @@ err:
> +
> +void
> +port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt)
> +{
> +	assert(port->vtab == &port_tuple_vtab);
> +	((struct port_sql *)port)->stmt = stmt;
> +	port->vtab = &port_sql_vtab;
> +}

2. Please, rename this method to port_sql_create and call here port_tuple_create()
explicitly. It is just a constructor. port_tuple_add, used in sql_execute(), will
work anyway. Since port_sql is derived from port_tuple, it can be used wherever the
base.

> +
> +const struct port_vtab port_sql_vtab = {
> +	.dump_msgpack = port_sql_dump_msgpack,
> +	.dump_msgpack_16 = NULL,
> +	.dump_lua = NULL,
> +	.dump_plain = NULL,
> +	.destroy = port_sql_destroy,
> +};
> diff --git a/src/box/execute.h b/src/box/execute.h
> index 60b8f31..5aef546 100644
> --- a/src/box/execute.h
> +++ b/src/box/execute.h
> @@ -51,14 +51,31 @@ extern const char *sql_info_key_strs[];
>   struct obuf;
>   struct region;
>   struct sql_bind;
> +struct sqlite3_stmt;
>   
> -/** Response on EXECUTE request. */
> -struct sql_response {
> -	/** Port with response data if any. */
> -	struct port port;
> -	/** Prepared SQL statement with metadata. */
> -	void *prep_stmt;
> +/**
> + * Port implementation used for dump tuples, stored in port_tuple,
> + * to obuf or Lua.
> + */
> +struct port_sql {
> +	/* port_tuple to inherit from. */
> +	struct port_tuple port_tuple;
> +	/* Prepared SQL statement. */
> +	struct sqlite3_stmt *stmt;
>   };
> +static_assert(sizeof(struct port_sql) <= sizeof(struct port),
> +	      "sizeof(struct port_sql) must be <= sizeof(struct port)");
> +
> +extern const struct port_vtab port_sql_vtab;
> +
> +/**
> + * Transform port_tuple with already stored tuples to port_sql
> + * that will dump these tuples into obut or Lua.
> + *
> + * @param port port_tuple to transform into port_sql.
> + */
> +void
> +port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt);

3. I think, we should not expose port_sql to the header. It is used in execute.c
only. Same about port_tuple_to_port_sql.

>   
>   /**
>    * Parse MessagePack array of SQL parameters.
> @@ -128,7 +109,7 @@ sql_response_dump(struct sql_response *response, struct obuf *out);
>    */
>   int
>   sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
> -			uint32_t bind_count, struct sql_response *response,
> +			uint32_t bind_count, struct port *port,
>   			struct region *region);

4. Do not forget about comments. It still describes 'response' parameter.

>   
>   #if defined(__cplusplus)

[tarantool-patches] Re: [PATCH v5 4/5] sql: parameter binding for new execute()

[Vladislav Shpilevoy]

Thanks for the patch! See 9 comments below.

On 22/12/2018 14:31, imeevma@tarantool.org wrote:
> This patch defines parameters binding for SQL statements executed
> through box.
> 
> Part of #3505
> Closes #3401
> ---
>   src/box/execute.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/box/execute.h |  15 +++++
>   src/box/lua/sql.c |   4 ++
>   3 files changed, 209 insertions(+)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 3cbee4f..f538441 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -229,6 +230,195 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind)
>   	return bind_count;
>   }
>   
> +
> +/**
> + * Decode a single bind column from Lua stack.
> + *
> + * @param L Lua stack.
> + * @param[out] bind Bind to decode to.
> + * @param idx Position of table with bind columns on Lua stack.
> + * @param i Ordinal bind number.
> + *
> + * @retval  0 Success.
> + * @retval -1 Memory or client error.
> + */
> +static inline int
> +lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
> +{
> +	struct luaL_field field;
> +	char *buf;
> +	lua_rawgeti(L, idx, i + 1);
> +	luaL_tofield(L, luaL_msgpack_default, -1, &field);
> +	bind->pos = i + 1;
> +	if (field.type == MP_MAP) {
> +		/*
> +		 * A named parameter is an MP_MAP with

1. It is not msgpack.

> +		 * one key - {'name': value}.
> +		 * Report parse error otherwise.
> +		 */
> +		if (field.size != 1) {
> +			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> +				 "parameter should be {'name': value}");

2. In Lua syntax {'name' : value} is incorrect.

> +			return -1;
> +		}
> +		/*
> +		 * Get key and value of the only map element to
> +		 * lua stack.
> +		 */
> +		lua_pushnil(L);
> +		lua_next(L, lua_gettop(L) - 1);

3. Just -1. Stack indexes can be negative.

> +		/* At first we should deal with the value. */
> +		luaL_tofield(L, luaL_msgpack_default, -1, &field);
> +		lua_pop(L, 1);

4. Please, do not pop the value stored in a luaL_field. Pop 2
values at one time below, on line 294.

> +		/* Now key is on the top of Lua stack. */
> +		size_t name_len = 0;
> +		bind->name = luaL_checklstring(L, -1, &name_len);
> +		if (bind->name == NULL) {

5. As I know, luaL_checklstring never returns NULL. It throws. Please,
check for string type explicitly and throw normal usage error.

> +			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> +				 "parameter should be {'name': value}");
> +			return -1;
> +		}
> +		/*
> +		 * Name should be saved in allocated memory as it
> +		 * will be poped from Lua stack.

6. As I said above, you've already popped it.

> +		 */
> +		buf = region_alloc(&fiber()->gc, name_len + 1);
> +		if (buf == NULL) {
> +			diag_set(OutOfMemory, name_len + 1, "region_alloc",
> +				 "buf");
> +			return -1;
> +		}
> +		memcpy(buf, bind->name, name_len + 1);
> +		bind->name = buf;
> +		bind->name_len = name_len;
> +		lua_pop(L, 1);
> +	} else {
> +		bind->name = NULL;
> +		bind->name_len = 0;
> +	}
> +	switch (field.type) {
> +	case MP_UINT: {
> +		bind->i64 = field.ival;
> +		bind->type = SQLITE_INTEGER;
> +		bind->bytes = sizeof(bind->i64);

7. Please, check for overflow.

> +		break;
> +	}
> +	case MP_INT:
> +		bind->i64 = field.ival;
> +		bind->type = SQLITE_INTEGER;
> +		bind->bytes = sizeof(bind->i64);
> +		break;
> +	case MP_STR:
> +		/*
> +		 * Data should be saved in allocated memory as it
> +		 * will be poped from Lua stack.
> +		 */
> +		buf = region_alloc(&fiber()->gc, field.sval.len + 1);
> +		if (buf == NULL) {
> +			diag_set(OutOfMemory, field.sval.len + 1,
> +				 "region_alloc", "buf");
> +			return -1;
> +		}
> +		memcpy(buf, field.sval.data, field.sval.len + 1);
> +		bind->s = buf;
> +		bind->type = SQLITE_TEXT;
> +		bind->bytes = field.sval.len;
> +		break;
> +	case MP_DOUBLE:
> +		bind->d = field.dval;
> +		bind->type = SQLITE_FLOAT;
> +		bind->bytes = sizeof(bind->d);
> +		break;
> +	case MP_FLOAT:
> +		bind->d = field.dval;
> +		bind->type = SQLITE_FLOAT;
> +		bind->bytes = sizeof(bind->d);
> +		break;
> +	case MP_NIL:
> +		bind->type = SQLITE_NULL;
> +		bind->bytes = 1;
> +		break;
> +	case MP_BOOL:
> +		/* SQLite doesn't support boolean. Use int instead. */
> +		bind->i64 = field.bval ? 1 : 0;
> +		bind->type = SQLITE_INTEGER;
> +		bind->bytes = sizeof(bind->i64);
> +		break;
> +	case MP_BIN:
> +		bind->s = mp_decode_bin(&field.sval.data, &bind->bytes);
> +		bind->type = SQLITE_BLOB;
> +		break;
> +	case MP_EXT:
> +		/*
> +		 * Data should be saved in allocated memory as it
> +		 * will be poped from Lua stack.
> +		 */
> +		buf = region_alloc(&fiber()->gc, sizeof(field));
> +		if (buf == NULL) {
> +			diag_set(OutOfMemory, sizeof(field), "region_alloc",
> +				 "buf");
> +			return -1;
> +		}
> +		memcpy(buf, &field, sizeof(field));
> +		bind->s = buf;
> +		bind->bytes = sizeof(field);
> +		bind->type = SQLITE_BLOB;
> +		break;
> +	case MP_ARRAY:
> +		diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY",
> +			 sql_bind_name(bind));
> +		return -1;
> +	case MP_MAP:
> +		diag_set(ClientError, ER_SQL_BIND_TYPE, "MAP",
> +			 sql_bind_name(bind));
> +		return -1;
> +	default:
> +		unreachable();
> +	}
> +	lua_pop(L, 1);
> +	return 0;

8. To be honest, I feel utterly uncomfortable with luaL_field. IMO,
this vast structure is too overloaded. Can you please avoid its usage?
Almost nothing will change as I see. For switch on types you can use
lua_type().

> +}
> +
> +int
> +lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
> +			 int idx)
> +{
> +	assert(out_bind != NULL);
> +	if (! lua_istable(L, idx)) {
> +		diag_set(ClientError, ER_INVALID_MSGPACK, "SQL parameter list");

9. Not msgpack. Please, do not blindly copy-paste code. Check out the
commit more accurately. I will not point out other places.

> +		return -1;
> +	}
> +	uint32_t bind_count = lua_objlen(L, idx);
> +	if (bind_count == 0)
> +		return 0;
> +	if (bind_count > SQL_BIND_PARAMETER_MAX) {
> +		diag_set(ClientError, ER_SQL_BIND_PARAMETER_MAX,
> +			 (int) bind_count);
> +		return -1;
> +	}
> +	struct region *region = &fiber()->gc;
> +	uint32_t used = region_used(region);
> +	size_t size = sizeof(struct sql_bind) * bind_count;
> +	/*
> +	 * Memory allocated here will be freed in
> +	 * sqlite3_finalize() or in txn_commit()/txn_rollback() if
> +	 * there is an active transaction.
> +	 */
> +	struct sql_bind *bind = (struct sql_bind *) region_alloc(region, size);
> +	if (bind == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "bind");
> +		return -1;
> +	}
> +	for (uint32_t i = 0; i < bind_count; ++i) {
> +		if (lua_sql_bind_decode(L, &bind[i], idx, i) != 0) {
> +			region_truncate(region, used);
> +			return -1;
> +		}
> +	}
> +	*out_bind = bind;
> +	return bind_count;
> +}
> +
>   /**
>    * Serialize a single column of a result set row.
>    * @param stmt Prepared and started statement. At least one