[tarantool-patches] Re: [PATCH v5 4/5] sql: parameter binding for new execute()

[Vladislav Shpilevoy <v.shpilevoy@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