[tarantool-patches] Re: [PATCH v2 6/7] lua: create vstream implementation for Lua

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes! See my 5 comments below, fix
at the end of the email and on the branch.

Also note, that I did not run tests. So before squashing
please, check the tests.

> diff --git a/src/box/execute.h b/src/box/execute.h
> index 5a11a8a..8ee0a89 100644
> --- a/src/box/execute.h
> +++ b/src/box/execute.h
> @@ -131,6 +131,21 @@ xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
>   		struct region *region);
>   
>   /**
> + * 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 request Request to save decoded parameters.
> + * @param idx Position of table with parameters on Lua stack.
> + * @param region Allocator.
> + *
> + * @retval  0 Success.
> + * @retval -1 Client or memory error.
> + */
> +int
> +lua_sql_bind_list_decode(struct lua_State *L, struct sql_request *request,
> +			 int idx, struct region *region);
1. You do not need to pass region here. Get fiber()->gc inside this
function. In original implementation it is passed because back in
those days we hoped to remove fiber()->gc, but now we decided not to
do it.

Also, it leaks now. You allocate sql_bind parameters on region, but
never truncate it. Iproto has the same bug, but you should not
duplicate it.

But you can not truncate it as well, since sql_prepare_and_execute
could save some transactional data onto it. And this bug iproto does
not have since it uses region of iproto thread.

I think, here you should use malloc.

> 		lua_pushnil(L);
> 		lua_next(L, lua_gettop(L) - 1);
> 		struct luaL_field field_name;
> 		lua_pushvalue(L, -2);
> 		luaL_tofield(L, luaL_msgpack_default, -1, &field_name);
> 		lua_pop(L, 1);
> 		assert(field_name.type == MP_STR);
> 		luaL_tofield(L, luaL_msgpack_default, -1, field);
> 		lua_pop(L, 1);
> 		bind->name = field_name.sval.data;
> 		bind->name_len = field_name.sval.len;

2. Please, do not use luaL_tofield for each scalar value. Here
it is much simpler to just use lua_tostring or something.

> 	for (uint32_t i = 0; i < bind_count; ++i) {
> 		struct luaL_field field;
> 		lua_rawgeti(L, idx, i + 1);
> 		luaL_tofield(L, luaL_msgpack_default, -1, &field);
> 		if (lua_sql_bind_decode(L, &bind[i], i, &field) != 0) {
> 			region_truncate(region, used);
> 			return -1;
> 		}
> 		lua_pop(L, 1);
> 	}

3. Why not to do lua_rawgeti and luaL_tofield inside lua_sql_bind_decode?

4. Lua vstream should not be implemented in sql.c. It is a separate file
luastream.c or something. You should have separate luastream implementation
without any vtabs and lua_vstream_vtab specially for vstream. Just like
it is done now for mpstream.

5. API of creating mp_vstream and lua_vstream is asymmetrical: to create
mp_vstream you call two functions to initialize mpstream and then vtab,
but to create lua_vstream you call one function. I think, it should be
fixed automatically once you've fixed the previous comment.

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

commit f14e975bfe49f982ac0efd61e3d67d1ced90c5a2
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Fri Nov 23 00:44:56 2018 +0300

     Review fixes

diff --git a/src/box/execute.c b/src/box/execute.c
index ca957a870..0a3ca15e0 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -312,8 +312,8 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int i,
  		 * Report parse error otherwise.
  		 */
  		if (field->size != 1) {
-			diag_set(ClientError, ER_INVALID_MSGPACK,
-				 "SQL bind parameter");
+			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
+				 "parameter should be {'name': value}");
  			return -1;
  		}
  		lua_pushnil(L);
@@ -393,7 +393,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int i,
  
  int
  lua_sql_bind_list_decode(struct lua_State *L, struct sql_request *request,
-			 int idx, struct region *region)
+			 int idx)
  {
  	assert(request != NULL);
  	if (! lua_istable(L, idx)) {
@@ -408,11 +408,12 @@ lua_sql_bind_list_decode(struct lua_State *L, struct sql_request *request,
  			 (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;
  	struct sql_bind *bind = (struct sql_bind *) region_alloc(region, size);
  	if (bind == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc", "struct sql_bind");
+		diag_set(OutOfMemory, size, "region_alloc", "bind");
  		return -1;
  	}
  	for (uint32_t i = 0; i < bind_count; ++i) {
diff --git a/src/box/execute.h b/src/box/execute.h
index afd0011da..ff468420b 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -148,14 +148,13 @@ xrow_decode_sql(const struct xrow_header *row, struct sql_request *request,
   * @param L Lua stack to get data from.
   * @param request Request to save decoded parameters.
   * @param idx Position of table with parameters on Lua stack.
- * @param region Allocator.
   *
   * @retval  0 Success.
   * @retval -1 Client or memory error.
   */
  int
  lua_sql_bind_list_decode(struct lua_State *L, struct sql_request *request,
-			 int idx, struct region *region);
+			 int idx);
  
  /**
   * Prepare and execute an SQL statement.
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 05556f98f..9245f85c9 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -67,8 +67,7 @@ lua_vstream_encode_bool(struct vstream *stream, bool val)
  int
  lua_vstream_encode_port(struct vstream *stream, struct port *port)
  {
-	if (port_dump_lua(port, stream->L) < 0)
-		return -1;
+	port_dump_lua(port, stream->L);
  	return 0;
  }
  
@@ -235,10 +234,9 @@ lbox_execute(struct lua_State *L)
  	struct sql_request request = {};
  	request.sql_text = sql;
  	request.sql_text_len = length;
-	if (lua_gettop(L) == 2)
-		if (lua_sql_bind_list_decode(L, &request, 2, &fiber()->gc) != 0)
-			return luaT_error(L);
-	struct sql_response response = {.is_flatten = true};
+	if (lua_gettop(L) == 2 && lua_sql_bind_list_decode(L, &request, 2) != 0)
+		return luaT_error(L);
+	struct sql_response response = {.is_info_flattened = true};
  	if (sql_prepare_and_execute(&request, &response, &fiber()->gc) != 0)
  		return luaT_error(L);