[tarantool-patches] [PATCH v7 5/6] lua: parameter binding for new execute()

[imeevma@tarantool.org]

Hi! Thank you for review! My answers, diff between versions and
new patch below.

On 12/29/18 4:19 PM, Vladislav Shpilevoy wrote:
> 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.
>
Fixed.

>>            */
>>           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()" ?
>
Fixed.

>> +            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'.
>
Fixed.

>>           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.
>
Thanks, squashed. The only change: I removed lua_pop(L, 1) that
was after lua_next(L, -4) as lua_next() pops in any case.

>> +    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.
>
Removed.

>> +        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;
>
Squashed.

>> +    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;
>
Squashed.

>> +    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.
>
Fixed. I used word "USERDATA" in error message to describe data
that cannot be used for binding.

>> +        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.
>
After discussion, it was decided that exceptions generated by
lua_error() should be removed. Any other exceptions should be left
as is.

>> 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?
>
Fixed.

>> +
>> +    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)
>>


Diff between versions:

commit 10613d0febaca704155c57e73f3810fcc7cd2eaf
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Jan 10 20:36:42 2019 +0300

    Temporary: review fix

diff --git a/src/box/execute.c b/src/box/execute.c
index 8e76234..1e9ee32 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -347,33 +347,27 @@ 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);
-		if (lua_type(L, -2) != LUA_TSTRING) {
+		lua_next(L, -2);
+		if (! lua_isstring(L, -2)) {
 			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 table with "\
+				 "one key - {name = value}");
+			return -1;
+		}
 		size_t name_len;
 		bind->name = lua_tolstring(L, -2, &name_len);
 		/*
@@ -389,23 +383,20 @@ 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;
 	}
+	if (luaL_tofield(L, luaL_msgpack_default, -1, &field) < 0)
+		return -1;
 	switch (field.type) {
-	case MP_UINT: {
-		if (field.ival < 0) {
+	case MP_UINT:
+		if ((uint64_t) field.ival > INT64_MAX) {
 			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;
-	}
+		FALLTHROUGH;
 	case MP_INT:
 		bind->i64 = field.ival;
 		bind->type = SQLITE_INTEGER;
@@ -428,10 +419,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;
@@ -452,21 +439,9 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
 		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));
-		bind->s = buf;
-		bind->bytes = sizeof(field);
-		bind->type = SQLITE_BLOB;
-		break;
+		diag_set(ClientError, ER_SQL_BIND_TYPE, "USERDATA",
+			 sql_bind_name(bind));
+		return -1;
 	case MP_ARRAY:
 		diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY",
 			 sql_bind_name(bind));


New version:

commit 544b9b2a0b1f1fa5b30323282d60ac46ca7db98c
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 10b309a..1e9ee32 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,
@@ -327,6 +328,171 @@ 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);
+	bind->pos = i + 1;
+	if (lua_istable(L, -1)) {
+		/*
+		 * Get key and value of the only table element to
+		 * lua stack.
+		 */
+		lua_pushnil(L);
+		lua_next(L, -2);
+		if (! lua_isstring(L, -2)) {
+			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 table with "\
+				 "one key - {name = value}");
+			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;
+	} else {
+		bind->name = NULL;
+		bind->name_len = 0;
+	}
+	if (luaL_tofield(L, luaL_msgpack_default, -1, &field) < 0)
+		return -1;
+	switch (field.type) {
+	case MP_UINT:
+		if ((uint64_t) field.ival > INT64_MAX) {
+			diag_set(ClientError, ER_SQL_BIND_VALUE,
+				 sql_bind_name(bind), "INTEGER");
+			return -1;
+		}
+		FALLTHROUGH;
+	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(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:
+	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:
+		diag_set(ClientError, ER_SQL_BIND_TYPE, "USERDATA",
+			 sql_bind_name(bind));
+		return -1;
+	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;
+}
+
+int
+lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
+			 int idx)
+{
+	assert(out_bind != NULL);
+	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 1ed9ab5..2b6cd4e 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -69,6 +69,27 @@ int
 sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
 
 /**
+ * Parse Lua table of SQL parameters.
+ *
+ * @param L Lua stack contains table with parameters. Each
+ *        parameter either must have scalar type, or must be a
+ *        single-row table with the following format:
+ *        table[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[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 f18a797..2999b3e 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -120,10 +120,18 @@ lbox_execute(struct lua_State *L)
 	struct port port;
 	int top = lua_gettop(L);
 
-	if (top != 1 || ! lua_isstring(L, 1))
-		return luaL_error(L, "Usage: box.execute(sqlstring)");
-
+	if (! (top == 1 || top == 2) || ! lua_isstring(L, 1))
+		return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
 	const char *sql = lua_tolstring(L, 1, &length);
+
+	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)
 		return luaT_error(L);
-- 
2.7.4