[tarantool-patches] Re: [PATCH v2 6/7] lua: create vstream implementation for Lua
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Nov 23 00:49:14 MSK 2018
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 at 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);
More information about the Tarantool-patches
mailing list