From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A90AA2DA4B for ; Thu, 22 Nov 2018 16:49:21 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id x4c77Hs76lat for ; Thu, 22 Nov 2018 16:49:21 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 7ECE72DDAF for ; Thu, 22 Nov 2018 16:49:20 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 6/7] lua: create vstream implementation for Lua References: From: Vladislav Shpilevoy Message-ID: <2f7e9873-8558-b7f6-2fd0-6ec01815ae1f@tarantool.org> Date: Fri, 23 Nov 2018 00:49:14 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, imeevma@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 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);