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

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

Thanks for the patch! See 10 comments below.

On 28/12/2018 21:11, imeevma@tarantool.org wrote:
> Hi! Thank you for review! My answers, diff between versions and
> new version below.
> 
> 
> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>> Thanks for the patch! See 9 comments below.
> 
>> 1. It is not msgpack.
> Fixed.
> 
>> 2. In Lua syntax {'name' : value} is incorrect.
> Fixed.
> 
>> 3. Just -1. Stack indexes can be negative.
> lua_next() throws an assert when I change arg value to -1:
> 
> tarantool: lj_api.c:892: lua_next: Assertion `(((t)->it) == (~11u))' failed.

It is suspicious. I looked at lua_next source and it does
convert negative index to positive with ease. Also, it is used
already in cosole.c, space.cc, httpc. and other places.

I tried it myself, and -2 value worked for me. Yes, -1 was
incorrect, my fault.

> 
>> 4. Please, do not pop the value stored in a luaL_field. Pop 2
>> values at one time below, on line 294.
> Fixed. Actually, I found out that my popping policy was quite a
> mess in previous version. Now everything will be popped at the
> end of the function.
> 
>> 5. As I know, luaL_checklstring never returns NULL. It throws. Please,
>> check for string type explicitly and throw normal usage error.
> Fixed.
> 
>> 6. As I said above, you've already popped it.
> Fixed.
> 
>> 7. Please, check for overflow.
> Fixed.
> 
>> 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().
> After some discussion it was decided to left it as it is now. The
> main reason for this is that if we remove luaL_field() than this
> function become downgraded version of luaL_field().
> 
>> 9. Not msgpack. Please, do not blindly copy-paste code. Check out the
>> commit more accurately. I will not point out other places.
> Fixed. Moved this check to sql.c.
> 
> 
> Diff between versions:
> 
> commit 93e4ea0ffcc195c4a904dab99c5cc516db33fa44
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Thu Dec 27 17:00:51 2018 +0300
> 
>      Temporary: review fixes
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 457403b..ade6e6c 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -278,43 +278,43 @@ static inline int
>   lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
>   {
>   	struct luaL_field field;
> +	struct region *region = &fiber()->gc;
>   	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.
> +		 * A named parameter is a single-row table that
> +		 * contains one value associated with a key. The
> +		 * key must be a string. This key is the name of
> +		 * the parameter, and the value associated with
> +		 * the key is the value of the parameter.

1. Or just change MP_MAP to table, and {'name': value} to {name = value},
and in other places too. But it is up to you. Your comment is ok as well.

>   		 */
>   		if (field.size != 1) {
>   			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> -				 "parameter should be {'name': value}");
> +				 "named parameter should be a single-row "\
> +				 "table with one key-value pair");
>   			return -1;
>   		}
>   		/*
> -		 * Get key and value of the only map element to
> +		 * Get key and value of the only table 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}");
> +		if (lua_type(L, -2) != LUA_TSTRING) {

2. Why so complex? Why not "! lua_isstring()" ?

> +			diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
> +				 "parameter should be a string.");
>   			return -1;
>   		}
> +		size_t name_len;
> +		bind->name = lua_tolstring(L, -2, &name_len);
>   		/*
>   		 * Name should be saved in allocated memory as it
>   		 * will be poped from Lua stack.
>   		 */
> -		buf = region_alloc(&fiber()->gc, name_len + 1);
> +		buf = region_alloc(region, name_len + 1);
>   		if (buf == NULL) {
>   			diag_set(OutOfMemory, name_len + 1, "region_alloc",
>   				 "buf");
> @@ -323,13 +323,18 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
>   		memcpy(buf, bind->name, name_len + 1);
>   		bind->name = buf;
>   		bind->name_len = name_len;
> -		lua_pop(L, 1);
> +		luaL_tofield(L, luaL_msgpack_default, -1, &field);
>   	} else {
>   		bind->name = NULL;
>   		bind->name_len = 0;
>   	}
>   	switch (field.type) {
>   	case MP_UINT: {
> +		if (field.ival < 0) {
> +			diag_set(ClientError, ER_SQL_BIND_VALUE,
> +				 sql_bind_name(bind), "INTEGER");
> +			return -1;
> +		}

3. How is it possible? Lua field reports that it is an unsigned
integer. If it is a way of checking for overflow, then please,
make it more straightforward. I looked into the source of
luaL_tofield and I saw that ival stores uint64_t just casting it
to int64_t. So here you should cast it back and check if it
is > INT64_MAX.

	(uint64_t) field.ival > INT64_MAX

Also, you do not need braces {} for 'case MP_UINT'.

>   		bind->i64 = field.ival;
>   		bind->type = SQLITE_INTEGER;
>   		bind->bytes = sizeof(bind->i64);
> commit a1fe2aa7edc82097b2fcf63a41fb7fa2145b9d3c
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Dec 21 17:54:16 2018 +0300
> 
>      lua: parameter binding for new execute()
>      
>      This patch defines parameters binding for SQL statements executed
>      through box.
>      
>      Part of #3505
>      Closes #3401
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 4606239..ade6e6c 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -261,6 +262,196 @@ 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;
> +	struct region *region = &fiber()->gc;
> +	char *buf;
> +	lua_rawgeti(L, idx, i + 1);
> +	luaL_tofield(L, luaL_msgpack_default, -1, &field);

4. I remembered why I do not like luaL_field. When it meets a
non-scalar value like map, it traverses it without necessity.
Lets refactor it a bit.

diff --git a/src/box/execute.c b/src/box/execute.c
index ade6e6c95..3a45fc524 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -281,33 +281,28 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
  	struct region *region = &fiber()->gc;
  	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 a single-row table that
-		 * contains one value associated with a key. The
-		 * key must be a string. This key is the name of
-		 * the parameter, and the value associated with
-		 * the key is the value of the parameter.
-		 */
-		if (field.size != 1) {
-			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
-				 "named parameter should be a single-row "\
-				 "table with one key-value pair");
-			return -1;
-		}
+	if (lua_istable(L, -1)) {
  		/*
  		 * Get key and value of the only table element to
  		 * lua stack.
  		 */
  		lua_pushnil(L);
-		lua_next(L, lua_gettop(L) - 1);
+		lua_next(L, -2);
  		if (lua_type(L, -2) != LUA_TSTRING) {
  			diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
  				 "parameter should be a string.");
  			return -1;
  		}
+		/* Check that the table is one-row sized. */
+		lua_pushvalue(L, -2);
+		if (lua_next(L, -4) != 0) {
+			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
+				 "named parameter should be a single-row "\
+				 "table with one key-value pair");
+			return -1;
+		}
+		lua_pop(L, 1);
  		size_t name_len;
  		bind->name = lua_tolstring(L, -2, &name_len);
  		/*
@@ -323,11 +318,11 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
  		memcpy(buf, bind->name, name_len + 1);
  		bind->name = buf;
  		bind->name_len = name_len;
-		luaL_tofield(L, luaL_msgpack_default, -1, &field);
  	} else {
  		bind->name = NULL;
  		bind->name_len = 0;
  	}
+	luaL_tofield(L, luaL_msgpack_default, -1, &field);
  	switch (field.type) {
  	case MP_UINT: {
  		if (field.ival < 0) {

Here I removed common traversal and added an explicit
check that a table consists of one key-value pair.

Are you ok with these changes? If you are - squash. But
I did not test it though.

> +	bind->pos = i + 1;
> +	if (field.type == MP_MAP) {
> +		/*
> +		 * A named parameter is a single-row table that
> +		 * contains one value associated with a key. The
> +		 * key must be a string. This key is the name of
> +		 * the parameter, and the value associated with
> +		 * the key is the value of the parameter.
> +		 */

5. Are you sure this comment is necessary? It just repeats the
error message below.

> +		if (field.size != 1) {
> +			diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> +				 "named parameter should be a single-row "\
> +				 "table with one key-value pair");
> +			return -1;
> +		}
> +		/*
> +		 * Get key and value of the only table element to
> +		 * lua stack.
> +		 */
> +		lua_pushnil(L);
> +		lua_next(L, lua_gettop(L) - 1);
> +		if (lua_type(L, -2) != LUA_TSTRING) {
> +			diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
> +				 "parameter should be a string.");
> +			return -1;
> +		}
> +		size_t name_len;
> +		bind->name = lua_tolstring(L, -2, &name_len);
> +		/*
> +		 * Name should be saved in allocated memory as it
> +		 * will be poped from Lua stack.
> +		 */
> +		buf = region_alloc(region, 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;
> +		luaL_tofield(L, luaL_msgpack_default, -1, &field);
> +	} else {
> +		bind->name = NULL;
> +		bind->name_len = 0;
> +	}
> +	switch (field.type) {
> +	case MP_UINT: {
> +		if (field.ival < 0) {
> +			diag_set(ClientError, ER_SQL_BIND_VALUE,
> +				 sql_bind_name(bind), "INTEGER");
> +			return -1;
> +		}
> +		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;

6. How about this?

diff --git a/src/box/execute.c b/src/box/execute.c
index ade6e6c95..c1b06b345 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -335,11 +335,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
  				 sql_bind_name(bind), "INTEGER");
  			return -1;
  		}
-		bind->i64 = field.ival;
-		bind->type = SQLITE_INTEGER;
-		bind->bytes = sizeof(bind->i64);
-		break;
-	}
+		FALLTHROUGH;
  	case MP_INT:
  		bind->i64 = field.ival;
  		bind->type = SQLITE_INTEGER;

> +	case MP_STR:
> +		/*
> +		 * Data should be saved in allocated memory as it
> +		 * will be poped from Lua stack.
> +		 */
> +		buf = region_alloc(region, 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;

7.

diff --git a/src/box/execute.c b/src/box/execute.c
index ade6e6c95..fb2e5ea29 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -362,10 +362,6 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
  		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;

> +	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(region, sizeof(field));
> +		if (buf == NULL) {
> +			diag_set(OutOfMemory, sizeof(field), "region_alloc",
> +				 "buf");
> +			return -1;
> +		}
> +		memcpy(buf, &field, sizeof(field));

8. Why? What a point of copying struct luaL_field onto region? As I
see, MP_EXT is set for unknown cdata like just void *, and the same
for unknown userdata. When void * is met, we do not even know its
size, nor what a value it stores. When we meet userdata, we can learn
its size, but we do not know what a value is stored here and can not
decode it.

Please, set and error when such thing happens.

> +		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, lua_gettop(L) - idx);
> +	return 0;
> +}

9. Also, I noticed one more problem with luaL_tofield. It can
deliberately throw during a number decoding, see at CHECK_NUMBER
macro. Moreover, it throws during table/map traversing in
lua_field_inspect_table. I once tried to remove these exceptions
but failed.

Here on a throw region leaks.

Also port_sql_dump_lua can throw during pushing data onto the stack
and the whole port_sql leaks.

Both problems can be solved using lua_cpcall, but please, ask in
the server team chat if it is worth doing. As I remember, Kostja
says 'let it crash' in such cases, but if a caller will use

	pcall(box.execute)

then it will not crash, but just leak.

> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
> index a55566e..20ba43b 100644
> --- a/src/box/lua/sql.c
> +++ b/src/box/lua/sql.c
> @@ -118,13 +118,22 @@ lbox_execute(struct lua_State *L)
>   	int bind_count = 0;
>   	size_t length;
>   	struct port port;
> +	int top = lua_gettop(L);
>   
> -	if (lua_type(L, 1) != LUA_TSTRING)
> -		return luaL_error(L, "Usage: box.execute(sqlstring)");
> +	if ((top != 1 && top != 2) || lua_type(L, 1) != LUA_TSTRING)
> +		return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
>   
>   	const char *sql = lua_tolstring(L, 1, &length);
>   	if (sql == NULL)
> -		return luaL_error(L, "Usage: box.execute(sqlstring)");
> +		return luaL_error(L, "Usage: box.execute(sqlstring[, params])");

10. How can it be NULL, if you have checked above that here a string
is stored?

> +
> +	if (top == 2) {
> +		if (! lua_istable(L, 2))
> +			return luaL_error(L, "Second argument must be a table");
> +		bind_count = lua_sql_bind_list_decode(L, &bind, 2);
> +		if (bind_count < 0)
> +			return luaT_error(L);
> +	}
>   
>   	if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
>   				    &fiber()->gc) != 0)
>