[tarantool-patches] Re: [PATCH v5 4/5] sql: parameter binding for new execute()
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Dec 25 17:57:07 MSK 2018
Thanks for the patch! See 9 comments below.
On 22/12/2018 14:31, imeevma at 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
More information about the Tarantool-patches
mailing list