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 EE8A52071D for ; Tue, 25 Dec 2018 09:57:09 -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 7pi8jKEhF-_T for ; Tue, 25 Dec 2018 09:57:09 -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 A5DEC20715 for ; Tue, 25 Dec 2018 09:57:09 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v5 4/5] sql: parameter binding for new execute() References: From: Vladislav Shpilevoy Message-ID: <9a1f4e1e-7c77-6f55-4850-90b72b52fa96@tarantool.org> Date: Tue, 25 Dec 2018 17:57:07 +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 patch! See 9 comments below. On 22/12/2018 14:31, imeevma@tarantool.org wrote: > This patch defines parameters binding for SQL statements executed > through box. > > Part of #3505 > Closes #3401 > --- > src/box/execute.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > src/box/execute.h | 15 +++++ > src/box/lua/sql.c | 4 ++ > 3 files changed, 209 insertions(+) > > diff --git a/src/box/execute.c b/src/box/execute.c > index 3cbee4f..f538441 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -229,6 +230,195 @@ 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; > + 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 1. It is not msgpack. > + * one key - {'name': value}. > + * Report parse error otherwise. > + */ > + if (field.size != 1) { > + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ > + "parameter should be {'name': value}"); 2. In Lua syntax {'name' : value} is incorrect. > + return -1; > + } > + /* > + * Get key and value of the only map element to > + * lua stack. > + */ > + lua_pushnil(L); > + lua_next(L, lua_gettop(L) - 1); 3. Just -1. Stack indexes can be negative. > + /* At first we should deal with the value. */ > + luaL_tofield(L, luaL_msgpack_default, -1, &field); > + lua_pop(L, 1); 4. Please, do not pop the value stored in a luaL_field. Pop 2 values at one time below, on line 294. > + /* 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) { 5. As I know, luaL_checklstring never returns NULL. It throws. Please, check for string type explicitly and throw normal usage error. > + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ > + "parameter should be {'name': value}"); > + return -1; > + } > + /* > + * Name should be saved in allocated memory as it > + * will be poped from Lua stack. 6. As I said above, you've already popped it. > + */ > + buf = region_alloc(&fiber()->gc, 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; > + lua_pop(L, 1); > + } else { > + bind->name = NULL; > + bind->name_len = 0; > + } > + switch (field.type) { > + case MP_UINT: { > + bind->i64 = field.ival; > + bind->type = SQLITE_INTEGER; > + bind->bytes = sizeof(bind->i64); 7. Please, check for overflow. > + break; > + } > + 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(&fiber()->gc, 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; > + 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(&fiber()->gc, 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; > + 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, 1); > + return 0; 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(). > +} > + > +int > +lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind, > + int idx) > +{ > + assert(out_bind != NULL); > + if (! lua_istable(L, idx)) { > + diag_set(ClientError, ER_INVALID_MSGPACK, "SQL parameter list"); 9. Not msgpack. Please, do not blindly copy-paste code. Check out the commit more accurately. I will not point out other places. > + return -1; > + } > + 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