* [tarantool-patches] [PATCH v7 0/6] sql: remove box.sql.execute
@ 2019-01-15 16:10 imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield() imeevma
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: imeevma @ 2019-01-15 16:10 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
The goal of this patch-set is to make functions from execute.c
the only way to execute SQL statements. This goal includes
similar output for executed SQL statements no matter how they
were executed: through net.box or through box.
This is the seventh version of patch-set. It is not complete. It
still has no last part, which is replacing box.sql.execute by
box.execute, because it will lead to massive test editing.
For now this patch-set blocked by #3832. Small temporary fix added
to temporary patch of patch-set.
https://github.com/tarantool/tarantool/issues/3505
https://github.com/tarantool/tarantool/tree/imeevma/gh-3505-no-sql-execute
General information of difference from previous version of
patch-set:
- Added new commit that removes lua_error() from luaL_tofield().
- Added new and fixed old comments and descriptions.
- Fixed some bugs.
- Refactoring.
A bit about patches of the patch-set:
Patch 1 removes lua_error() from luaL_tofield().
Patch 2 moves map creation from xrow functions to
sql_response_dump(). It allows us to use sql_response_dump() as
method of port.
Patch 3 creates port_sql and two its methods: dump_msgpack() and
destroy().
Patch 4 creates dump_lua() method for port_sql.
Patch 5 adds binding to new_execute().
Patch 6 is temporary patch. It was created to check that
new_execute() is able to pass through tests created for execute().
v1:
https://www.freelists.org/post/tarantool-patches/PATCH-v1-0010-sql-remove-boxsqlexecute
v2:
https://www.freelists.org/post/tarantool-patches/PATCH-v2-07-Remove-boxsqlexecute
v3:
https://www.freelists.org/post/tarantool-patches/PATCH-v3-07-Remove-boxsqlexecute
v4:
https://www.freelists.org/post/tarantool-patches/PATCH-v4-05-Remove-boxsqlexecute
v5:
https://www.freelists.org/post/tarantool-patches/PATCH-v5-05-sql-remove-boxsqlexecute
v6:
https://www.freelists.org/post/tarantool-patches/PATCH-v6-05-sql-remove-boxsqlexecute
Mergen Imeev (6):
lua: remove exceptions from function luaL_tofield()
iproto: move map creation to sql_response_dump()
iproto: create port_sql
lua: create method dump_lua for port_sql
lua: parameter binding for new execute()
sql: check new box.sql.execute()
src/box/execute.c | 493 +++++++++++++++++++++++++++++++++++++++++--------
src/box/execute.h | 63 ++-----
src/box/iproto.cc | 16 +-
src/box/lua/call.c | 9 +-
src/box/lua/schema.lua | 23 +++
src/box/lua/sql.c | 36 +++-
src/box/lua/tuple.c | 3 +-
src/box/port.h | 1 -
src/box/xrow.c | 8 +-
src/box/xrow.h | 9 +-
src/lua/msgpack.c | 12 +-
src/lua/utils.c | 105 ++++++-----
src/lua/utils.h | 8 +-
13 files changed, 591 insertions(+), 195 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-15 16:10 [tarantool-patches] [PATCH v7 0/6] sql: remove box.sql.execute imeevma
@ 2019-01-15 16:10 ` imeevma
2019-01-16 18:25 ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-17 11:38 ` Konstantin Osipov
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 2/6] iproto: move map creation to sql_response_dump() imeevma
` (4 subsequent siblings)
5 siblings, 2 replies; 13+ messages in thread
From: imeevma @ 2019-01-15 16:10 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Before this commit, the luaL_tofield() function threw a Lua
exception when an error occurred. This behavior has been changed
in this commit, now it sets diag_set() and returns -1 in case of
an error.
Needed for #3505
---
src/box/lua/call.c | 9 +++--
src/box/lua/tuple.c | 3 +-
src/lua/msgpack.c | 12 ++++--
src/lua/utils.c | 105 ++++++++++++++++++++++++++++++----------------------
src/lua/utils.h | 8 +++-
5 files changed, 83 insertions(+), 54 deletions(-)
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 52939ae..39df05d 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -164,7 +164,8 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
*/
for (int i = 1; i <= nrets; ++i) {
struct luaL_field field;
- luaL_tofield(L, cfg, i, &field);
+ if (luaL_tofield(L, cfg, i, &field) < 0)
+ luaT_error(L);
struct tuple *tuple;
if (field.type == MP_EXT &&
(tuple = luaT_istuple(L, i)) != NULL) {
@@ -192,7 +193,8 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
* Inspect the first result
*/
struct luaL_field root;
- luaL_tofield(L, cfg, 1, &root);
+ if (luaL_tofield(L, cfg, 1, &root) < 0)
+ luaT_error(L);
struct tuple *tuple;
if (root.type == MP_EXT && (tuple = luaT_istuple(L, 1)) != NULL) {
/* `return box.tuple()` */
@@ -221,7 +223,8 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
for (uint32_t t = 1; t <= root.size; t++) {
lua_rawgeti(L, 1, t);
struct luaL_field field;
- luaL_tofield(L, cfg, -1, &field);
+ if (luaL_tofield(L, cfg, -1, &field) < 0)
+ luaT_error(L);
if (field.type == MP_EXT && (tuple = luaT_istuple(L, -1))) {
tuple_to_mpstream(tuple, stream);
} else if (field.type != MP_ARRAY) {
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 1867f81..f76d656 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -223,7 +223,8 @@ luamp_convert_key(struct lua_State *L, struct luaL_serializer *cfg,
return tuple_to_mpstream(tuple, stream);
struct luaL_field field;
- luaL_tofield(L, cfg, index, &field);
+ if (luaL_tofield(L, cfg, index, &field) < 0)
+ luaT_error(L);
if (field.type == MP_ARRAY) {
lua_pushvalue(L, index);
luamp_encode_r(L, cfg, stream, &field, 0);
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index b470060..d00ade9 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -149,10 +149,12 @@ restart: /* used by MP_EXT */
lua_pushnil(L); /* first key */
while (lua_next(L, top) != 0) {
lua_pushvalue(L, -2); /* push a copy of key to top */
- luaL_tofield(L, cfg, lua_gettop(L), field);
+ if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
+ luaT_error(L);
luamp_encode_r(L, cfg, stream, field, level + 1);
lua_pop(L, 1); /* pop a copy of key */
- luaL_tofield(L, cfg, lua_gettop(L), field);
+ if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
+ luaT_error(L);
luamp_encode_r(L, cfg, stream, field, level + 1);
lua_pop(L, 1); /* pop value */
}
@@ -168,7 +170,8 @@ restart: /* used by MP_EXT */
mpstream_encode_array(stream, size);
for (uint32_t i = 0; i < size; i++) {
lua_rawgeti(L, top, i + 1);
- luaL_tofield(L, cfg, top + 1, field);
+ if (luaL_tofield(L, cfg, top + 1, field) < 0)
+ luaT_error(L);
luamp_encode_r(L, cfg, stream, field, level + 1);
lua_pop(L, 1);
}
@@ -203,7 +206,8 @@ luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
}
struct luaL_field field;
- luaL_tofield(L, cfg, lua_gettop(L), &field);
+ if (luaL_tofield(L, cfg, lua_gettop(L), &field) < 0)
+ luaT_error(L);
enum mp_type top_type = luamp_encode_r(L, cfg, stream, &field, 0);
if (!on_top) {
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 978fe61..9a9fd3a 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -37,6 +37,8 @@
#include <diag.h>
#include <fiber.h>
+#include "box/error.h"
+
int luaL_nil_ref = LUA_REFNIL;
int luaL_map_metatable_ref = LUA_REFNIL;
int luaL_array_metatable_ref = LUA_REFNIL;
@@ -383,12 +385,13 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
lua_pcall(L, 1, 1, 0);
/* replace obj with the unpacked value */
lua_replace(L, idx);
- luaL_tofield(L, cfg, idx, field);
+ if (luaL_tofield(L, cfg, idx, field) < 0)
+ luaT_error(L);
} /* else ignore lua_gettable exceptions */
lua_settop(L, top); /* remove temporary objects */
}
-static void
+static int
lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
int idx, struct luaL_field *field)
{
@@ -408,10 +411,11 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
lua_call(L, 1, 1);
/* replace obj with the unpacked value */
lua_replace(L, idx);
- luaL_tofield(L, cfg, idx, field);
- return;
+ return luaL_tofield(L, cfg, idx, field);
} else if (!lua_isstring(L, -1)) {
- luaL_error(L, "invalid " LUAL_SERIALIZE " value");
+ diag_set(ClientError, ER_PROC_LUA,
+ "invalid " LUAL_SERIALIZE " value");
+ return -1;
}
type = lua_tostring(L, -1);
@@ -424,7 +428,7 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
field->compact = true;
lua_pop(L, 1); /* type */
- return;
+ return 0;
} else if (strcmp(type, "map") == 0 || strcmp(type, "mapping") == 0) {
field->type = MP_MAP; /* Override type */
field->size = luaL_maplen(L, idx);
@@ -432,9 +436,11 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
if (cfg->has_compact && type[3] == '\0')
field->compact = true;
lua_pop(L, 1); /* type */
- return;
+ return 0;
} else {
- luaL_error(L, "invalid " LUAL_SERIALIZE " value");
+ diag_set(ClientError, ER_PROC_LUA,
+ "invalid " LUAL_SERIALIZE " value");
+ return -1;
}
skip:
@@ -456,7 +462,7 @@ skip:
}
field->type = MP_MAP;
field->size = size;
- return;
+ return 0;
}
if (k > max)
max = k;
@@ -466,15 +472,19 @@ skip:
if (cfg->encode_sparse_ratio > 0 &&
max > size * (uint32_t)cfg->encode_sparse_ratio &&
max > (uint32_t)cfg->encode_sparse_safe) {
- if (!cfg->encode_sparse_convert)
- luaL_error(L, "excessively sparse array");
+ if (!cfg->encode_sparse_convert) {
+ diag_set(ClientError, ER_PROC_LUA,
+ "excessively sparse array");
+ return -1;
+ }
field->type = MP_MAP;
field->size = size;
- return;
+ return 0;
}
assert(field->type == MP_ARRAY);
field->size = max;
+ return 0;
}
static void
@@ -487,12 +497,13 @@ lua_field_tostring(struct lua_State *L, struct luaL_serializer *cfg, int idx,
lua_call(L, 1, 1);
lua_replace(L, idx);
lua_settop(L, top);
- luaL_tofield(L, cfg, idx, field);
+ if (luaL_tofield(L, cfg, idx, field) < 0)
+ luaT_error(L);
}
-void
+int
luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
- struct luaL_field *field)
+ struct luaL_field *field)
{
if (index < 0)
index = lua_gettop(L) + index + 1;
@@ -501,10 +512,13 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
double intpart;
size_t size;
-#define CHECK_NUMBER(x) ({\
- if (!isfinite(x) && !cfg->encode_invalid_numbers) { \
- if (!cfg->encode_invalid_as_nil) \
- luaL_error(L, "number must not be NaN or Inf"); \
+#define CHECK_NUMBER(x) ({ \
+ if (!isfinite(x) && !cfg->encode_invalid_numbers) { \
+ if (!cfg->encode_invalid_as_nil) { \
+ diag_set(ClientError, ER_PROC_LUA, \
+ "number must not be NaN or Inf"); \
+ return -1; \
+ } \
field->type = MP_NIL; \
}})
@@ -525,94 +539,96 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
field->dval = num;
CHECK_NUMBER(num);
}
- return;
+ break;
case LUA_TCDATA:
{
- uint32_t ctypeid = 0;
- void *cdata = luaL_checkcdata(L, index, &ctypeid);
+ GCcdata *cd = cdataV(L->base + index - 1);
+ void *cdata = (void *)cdataptr(cd);
+
int64_t ival;
- switch (ctypeid) {
+ switch (cd->ctypeid) {
case CTID_BOOL:
field->type = MP_BOOL;
field->bval = *(bool*) cdata;
- return;
+ break;
case CTID_CCHAR:
case CTID_INT8:
ival = *(int8_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- return;
+ break;
case CTID_INT16:
ival = *(int16_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- return;
+ break;
case CTID_INT32:
ival = *(int32_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- return;
+ break;
case CTID_INT64:
ival = *(int64_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- return;
+ break;
case CTID_UINT8:
field->type = MP_UINT;
field->ival = *(uint8_t *) cdata;
- return;
+ break;
case CTID_UINT16:
field->type = MP_UINT;
field->ival = *(uint16_t *) cdata;
- return;
+ break;
case CTID_UINT32:
field->type = MP_UINT;
field->ival = *(uint32_t *) cdata;
- return;
+ break;
case CTID_UINT64:
field->type = MP_UINT;
field->ival = *(uint64_t *) cdata;
- return;
+ break;
case CTID_FLOAT:
field->type = MP_FLOAT;
field->fval = *(float *) cdata;
CHECK_NUMBER(field->fval);
- return;
+ break;
case CTID_DOUBLE:
field->type = MP_DOUBLE;
field->dval = *(double *) cdata;
CHECK_NUMBER(field->dval);
- return;
+ break;
case CTID_P_CVOID:
case CTID_P_VOID:
if (*(void **) cdata == NULL) {
field->type = MP_NIL;
- return;
+ break;
}
/* Fall through */
default:
field->type = MP_EXT;
- return;
+ break;
}
- return;
+ break;
}
case LUA_TBOOLEAN:
field->type = MP_BOOL;
field->bval = lua_toboolean(L, index);
- return;
+ break;
case LUA_TNIL:
field->type = MP_NIL;
- return;
+ break;
case LUA_TSTRING:
field->sval.data = lua_tolstring(L, index, &size);
field->sval.len = (uint32_t) size;
field->type = MP_STR;
- return;
+ break;
case LUA_TTABLE:
{
field->compact = false;
- lua_field_inspect_table(L, cfg, index, field);
- return;
+ if (lua_field_inspect_table(L, cfg, index, field) < 0)
+ return -1;
+ break;
}
case LUA_TLIGHTUSERDATA:
case LUA_TUSERDATA:
@@ -620,14 +636,15 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
field->sval.len = 0;
if (lua_touserdata(L, index) == NULL) {
field->type = MP_NIL;
- return;
+ break;
}
/* Fall through */
default:
field->type = MP_EXT;
- return;
+ break;
}
#undef CHECK_NUMBER
+ return 0;
}
void
diff --git a/src/lua/utils.h b/src/lua/utils.h
index a47e3d2..fde3514 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -304,8 +304,11 @@ struct luaL_field {
* @param cfg configuration
* @param index stack index
* @param field conversion result
+ *
+ * @retval 0 Success.
+ * @retval -1 Error.
*/
-void
+int
luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
struct luaL_field *field);
@@ -345,7 +348,8 @@ static inline void
luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
struct luaL_field *field)
{
- luaL_tofield(L, cfg, idx, field);
+ if (luaL_tofield(L, cfg, idx, field) < 0)
+ luaT_error(L);
if (field->type != MP_EXT)
return;
luaL_convertfield(L, cfg, idx, field);
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] [PATCH v7 2/6] iproto: move map creation to sql_response_dump()
2019-01-15 16:10 [tarantool-patches] [PATCH v7 0/6] sql: remove box.sql.execute imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield() imeevma
@ 2019-01-15 16:10 ` imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 3/6] iproto: create port_sql imeevma
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: imeevma @ 2019-01-15 16:10 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Currently, function sql_response_dump() puts data into an already
created map. Moving the map creation to sql_response_dump()
simplifies the code and allows us to use sql_response_dump() as
one of the port_sql methods.
Needed for #3505
---
src/box/execute.c | 23 ++++++++++++++++-------
src/box/execute.h | 3 +--
src/box/iproto.cc | 8 +++-----
src/box/xrow.c | 8 +-------
src/box/xrow.h | 9 +--------
5 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 7fff5fd..38b6cbc 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -555,22 +555,29 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
}
int
-sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
+sql_response_dump(struct sql_response *response, struct obuf *out)
{
sqlite3 *db = sql_get();
struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
int rc = 0, column_count = sqlite3_column_count(stmt);
if (column_count > 0) {
+ int keys = 2;
+ int size = mp_sizeof_map(keys);
+ char *pos = (char *) obuf_alloc(out, size);
+ if (pos == NULL) {
+ diag_set(OutOfMemory, size, "obuf_alloc", "pos");
+ goto err;
+ }
+ pos = mp_encode_map(pos, keys);
if (sql_get_description(stmt, out, column_count) != 0) {
err:
rc = -1;
goto finish;
}
- *keys = 2;
- int size = mp_sizeof_uint(IPROTO_DATA) +
- mp_sizeof_array(port_tuple->size);
- char *pos = (char *) obuf_alloc(out, size);
+ size = mp_sizeof_uint(IPROTO_DATA) +
+ mp_sizeof_array(port_tuple->size);
+ pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
goto err;
@@ -586,18 +593,20 @@ err:
goto err;
}
} else {
- *keys = 1;
+ int keys = 1;
assert(port_tuple->size == 0);
struct stailq *autoinc_id_list =
vdbe_autoinc_id_list((struct Vdbe *)stmt);
uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
- int size = mp_sizeof_uint(IPROTO_SQL_INFO) +
+ int size = mp_sizeof_map(keys) +
+ mp_sizeof_uint(IPROTO_SQL_INFO) +
mp_sizeof_map(map_size);
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
goto err;
}
+ pos = mp_encode_map(pos, keys);
pos = mp_encode_uint(pos, IPROTO_SQL_INFO);
pos = mp_encode_map(pos, map_size);
uint64_t id_count = 0;
diff --git a/src/box/execute.h b/src/box/execute.h
index 9c1bc4f..60b8f31 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -105,14 +105,13 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
* | } |
* +----------------------------------------------+
* @param response EXECUTE response.
- * @param[out] keys number of keys in dumped map.
* @param out Output buffer.
*
* @retval 0 Success.
* @retval -1 Memory error.
*/
int
-sql_response_dump(struct sql_response *response, int *keys, struct obuf *out);
+sql_response_dump(struct sql_response *response, struct obuf *out);
/**
* Prepare and execute an SQL statement.
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 1debc3c..a08c8c5 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1641,17 +1641,15 @@ tx_process_sql(struct cmsg *m)
* become out of date during yield.
*/
out = msg->connection->tx.p_obuf;
- int keys;
struct obuf_svp header_svp;
/* Prepare memory for the iproto header. */
- if (iproto_prepare_header(out, &header_svp, IPROTO_SQL_HEADER_LEN) != 0)
+ if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
goto error;
- if (sql_response_dump(&response, &keys, out) != 0) {
+ if (sql_response_dump(&response, out) != 0) {
obuf_rollback_to_svp(out, &header_svp);
goto error;
}
- iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version,
- keys);
+ iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
iproto_wpos_create(&msg->wpos, out);
return;
error:
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 67019a6..c4e3073 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -508,17 +508,11 @@ error:
void
iproto_reply_sql(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
- uint32_t schema_version, int keys)
+ uint32_t schema_version)
{
char *pos = (char *) obuf_svp_to_ptr(buf, svp);
iproto_header_encode(pos, IPROTO_OK, sync, schema_version,
obuf_size(buf) - svp->used - IPROTO_HEADER_LEN);
- /*
- * MessagePack encodes value <= 15 as
- * bitwise OR with 0x80.
- */
- assert(keys <= 15);
- *(pos + IPROTO_HEADER_LEN) = 0x80 | keys;
}
void
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 6bab0a1..2654e35 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -52,12 +52,6 @@ enum {
IPROTO_HEADER_LEN = 28,
/** 7 = sizeof(iproto_body_bin). */
IPROTO_SELECT_HEADER_LEN = IPROTO_HEADER_LEN + 7,
- /**
- * Header of message + header of body with one or two
- * keys: IPROTO_DATA and IPROTO_METADATA or
- * IPROTO_SQL_INFO. 1 == mp_sizeof_map(<=15).
- */
- IPROTO_SQL_HEADER_LEN = IPROTO_HEADER_LEN + 1,
};
struct xrow_header {
@@ -491,11 +485,10 @@ xrow_decode_sql(const struct xrow_header *row, struct sql_request *request);
* @param svp Savepoint of the header beginning.
* @param sync Request sync.
* @param schema_version Schema version.
- * @param keys Count of keys in the body.
*/
void
iproto_reply_sql(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
- uint32_t schema_version, int keys);
+ uint32_t schema_version);
/**
* Write an IPROTO_CHUNK header from a specified position in a
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] [PATCH v7 3/6] iproto: create port_sql
2019-01-15 16:10 [tarantool-patches] [PATCH v7 0/6] sql: remove box.sql.execute imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield() imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 2/6] iproto: move map creation to sql_response_dump() imeevma
@ 2019-01-15 16:10 ` imeevma
2019-01-16 18:25 ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 4/6] lua: create method dump_lua for port_sql imeevma
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: imeevma @ 2019-01-15 16:10 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Hi! Thank you for review! My answers, diff between versions and
new patch below.
On 12/29/18 4:19 PM, Vladislav Shpilevoy wrote:
> Thanks for the patch! See 11 comments below.
>
> On 28/12/2018 21:11, imeevma@tarantool.org wrote:
>> Hi! Thank you for review! My answers, diff between versions and
>> new version below.
>>
>> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>>> Hi! Thanks for the patch! See 4 comments below.
>>
>>> 1. Please, write a comment about how could you use port_tuple_vtab on
>>> port_sql. Here I expected to see words about calling methods of a base
>>> structure, like BaseClass::method in C++.
>> Added. Not sure that it is enough.
>
> Than add more explanations? Hardly it is possible to 'overexplain'
> something, but it is as easy as nothing to 'underexplain'.
>
Changed comment. I think that should be enough.
>>
>>> 2. Please, rename this method to port_sql_create and call here port_tuple_create()
>>> explicitly. It is just a constructor. port_tuple_add, used in sql_execute(), will
>>> work anyway. Since port_sql is derived from port_tuple, it can be used wherever the
>>> base.
>> Done.
>>
>>> 3. I think, we should not expose port_sql to the header. It is used in execute.c
>>> only. Same about port_tuple_to_port_sql.
>> Done.
>>
>>> 4. Do not forget about comments. It still describes 'response' parameter.
>> Fixed.
>>
>>
>> Diff between versions:
>>
>> commit 414bf16c094fbb7b9e0ecb6b4b56f069f109ef86
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date: Wed Dec 26 22:00:35 2018 +0300
>>
>> Temporary: review fixes
>>
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index b07de28..c6fcb50 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -83,6 +83,36 @@ struct sql_bind {
>> };
>> /**
>> + * Port implementation used for dump tuples, stored in port_tuple,
>> + * to obuf or Lua.
>
> 1. Not only tuples, but the whole response, including headers,
> meta, info.
>
Fixed.
>> + *
>> + * All port_tuple methods can be used with port_sql, since
>> + * port_sql is a structure inherited from port_tuple. Calling
>> + * port_tuple methods we are going via port_tuple_vtab.
>
> 2. Not sure if the sentence is correct. 'Calling we are going'
> looks flawed. Maybe better 'Port_tuple methods are called via
> explicit access to port_tuple_vtab just like C++ does with
> BaseClass::method, when it is called in a child method'. ?
> Or something like this.
>
Fixed.
>> + */
>> +struct port_sql {
>> + /* port_tuple to inherit from. */
>> + struct port_tuple port_tuple;
>> + /* Prepared SQL statement. */
>> + struct sqlite3_stmt *stmt;
>> +};
>> +static_assert(sizeof(struct port_sql) <= sizeof(struct port),
>> + "sizeof(struct port_sql) must be <= sizeof(struct port)");
>> +
>> +static int
>> +port_sql_dump_msgpack(struct port *port, struct obuf *out);
>
> 3. Please, add empty line between function declarations. Also, we
> usually write a comment above a first function declaration appearance,
> so the whole comment about port_sql_dump_msgpack() should be put here.
>
Fixed.
>> +static void
>> +port_sql_destroy(struct port *base);
>
> 4. Correct me if I am wrong, but as I see, port_sql_destroy can be
> implemented right here, without pre-announcement.
You are right, fixed.
>
> Also, please, move port_sql_create here to gather all methods
> in one place.
>
Fixed.
>> +
>> +const struct port_vtab port_sql_vtab = {
>
> 5. 'static'. It is not used anywhere outside execute.c now.
>
Fixed.
>> + .dump_msgpack = port_sql_dump_msgpack,
>> + .dump_msgpack_16 = NULL,
>> + .dump_lua = NULL,
>> + .dump_plain = NULL,
>> + .destroy = port_sql_destroy,
>
> 6. Not sure why, but in new code we do not explicitly
> write attribute names. Instead, we use
>
> /* .method_name = */ func,
>
> not
>
> .method_name = func,
>
> To be honest I like your way and how it was done in old
> code, but Vladimir once forced me to use /* ... = */, so
> lets follow.
>
Fixed.
>> +};
>> +
>> +/**
>> * Return a string name of a parameter marker.
>> * @param Bind to get name.
>> * @retval Zero terminated name.
>> @@ -356,6 +386,11 @@ sql_row_to_port(struct sqlite3_stmt *stmt, int column_count,
>> if (tuple == NULL)
>> goto error;
>> region_truncate(region, svp);
>> + /*
>> + * The port_tuple_add function can be used with port_sql,
>> + * since it does not call any port_tuple methods and works
>> + * only with fields.
>
> 7. It does not matter even if it would call port_tuple_methods.
> As we stated in port_sql comment, we derived from port_tuple and
> can use it in-place of base. I do not think this particular place
> should be commented at all.
>
Removed all such comments with exception of one before definition
of struct port_sql.
>> + */
>> return port_tuple_add(port, tuple);
>> error:
>> commit 8390b09d95aa5bcaa6ef89924d9a334dff746f19
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date: Fri Dec 21 14:52:03 2018 +0300
>>
>> iproto: create port_sql
>> This patch creates port_sql implementation for the port. This will
>> allow us to dump sql responses to obuf or to Lua. Also this patch
>> defines methods dump_msgpack() and destroy() of port_sql.
>> Part of #3505
>>
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 38b6cbc..c6fcb50 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -575,26 +606,19 @@ err:
>> rc = -1;
>> goto finish;
>> }
>> - size = mp_sizeof_uint(IPROTO_DATA) +
>> - mp_sizeof_array(port_tuple->size);
>> + size = mp_sizeof_uint(IPROTO_DATA);
>> pos = (char *) obuf_alloc(out, size);
>> if (pos == NULL) {
>> diag_set(OutOfMemory, size, "obuf_alloc", "pos");
>> goto err;
>> }
>> pos = mp_encode_uint(pos, IPROTO_DATA);
>> - pos = mp_encode_array(pos, port_tuple->size);
>> - /*
>> - * Just like SELECT, SQL uses output format compatible
>> - * with Tarantool 1.6
>> - */
>> - if (port_dump_msgpack_16(&response->port, out) < 0) {
>> - /* Failed port dump destroyes the port. */
>> + /* Calling BaseStruct::methods via its vtab. */
>
> 8. Lets do not repeat this comment on each port_tuple_vtab usage. It
> it direct repetition of what is said in port_sql comment.
>
Fixed.
>> + if (port_tuple_vtab.dump_msgpack(port, out) < 0)
>> goto err;
>> - }
>> } else {
>> int keys = 1;
>> - assert(port_tuple->size == 0);
>> + assert(((struct port_tuple *)port)->size == 0);
>> struct stailq *autoinc_id_list =
>> vdbe_autoinc_id_list((struct Vdbe *)stmt);
>> uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
>> @@ -643,7 +667,63 @@ err:
>> +
>> +static inline int
>> +sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
>> + struct region *region)
>
> 9. The function is not so trivial to omit a comment. Please, write it.
> In particular what a one should do with region after sql_execute() is
> finished. Should it be cleared?
>
Added description.
>> +{
>> + int rc, column_count = sqlite3_column_count(stmt);
>> + if (column_count > 0) {
>> + /* Either ROW or DONE or ERROR. */
>> + while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
>> + if (sql_row_to_port(stmt, column_count, region,
>> + port) != 0)
>> + return -1;
>> + }
>> + assert(rc == SQLITE_DONE || rc != SQLITE_OK);
>> + } else {
>> + /* No rows. Either DONE or ERROR. */
>> + rc = sqlite3_step(stmt);
>> + assert(rc != SQLITE_ROW && rc != SQLITE_OK);
>> + }
>> + if (rc != SQLITE_DONE) {
>> + diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
>> + return -1;
>> + }
>> + return 0;
>> +}
>> +
>> +int
>> +sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
>> + uint32_t bind_count, struct port *port,
>> + struct region *region)
>> +{
>> + struct sqlite3_stmt *stmt;
>> + sqlite3 *db = sql_get();
>> + if (db == NULL) {
>> + diag_set(ClientError, ER_LOADING);
>> + return -1;
>
> 10. Not sure if it is possible. Is it?
>
Removed.
>> + }
>> + if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
>> + diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
>> + return -1;
>> + }
>> + assert(stmt != NULL);
>> + port_sql_create(port, stmt);
>> + if (sql_bind(stmt, bind, bind_count) == 0 &&
>> + sql_execute(db, stmt, port, region) == 0)
>> + return 0;
>> + port_destroy(port);
>> + return -1;
>> +}
>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
>> index a08c8c5..9dc0462 100644
>> --- a/src/box/iproto.cc
>> +++ b/src/box/iproto.cc
>> @@ -1645,7 +1645,7 @@ tx_process_sql(struct cmsg *m)
>> /* Prepare memory for the iproto header. */
>> if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
>> goto error;
>> - if (sql_response_dump(&response, out) != 0) {
>> + if (port_dump_msgpack(&port, out) != 0) {
>> obuf_rollback_to_svp(out, &header_svp);
>> goto error;
>
> 11. Port leaks if an error occurred in iproto_prepare_header.
>
Fixed.
>> }
>
> Sorry, most of comments are minor and I could fix them
> myself, but I was asked to do not rewrite patches if they
> are not urgent.
Diff between versions:
commit 20864423db67a3322e203cb69d910d3979d63c2b
Author: Mergen Imeev <imeevma@gmail.com>
Date: Wed Jan 9 17:36:15 2019 +0300
Temporary: review fix
diff --git a/src/box/execute.c b/src/box/execute.c
index c6fcb50..a81f482 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -83,12 +83,15 @@ struct sql_bind {
};
/**
- * Port implementation used for dump tuples, stored in port_tuple,
- * to obuf or Lua.
+ * Port implementation that is used to store SQL responses and
+ * output them to obuf or Lua. This port implementation is
+ * inherited from the port_tuple structure. This allows us to use
+ * this structure in the port_tuple methods instead of port_tuple
+ * itself.
*
- * All port_tuple methods can be used with port_sql, since
- * port_sql is a structure inherited from port_tuple. Calling
- * port_tuple methods we are going via port_tuple_vtab.
+ * The methods of port_tuple are called via explicit access to
+ * port_tuple_vtab just like C++ does with BaseClass::method, when
+ * it is called in a child method.
*/
struct port_sql {
/* port_tuple to inherit from. */
@@ -96,22 +99,76 @@ struct port_sql {
/* Prepared SQL statement. */
struct sqlite3_stmt *stmt;
};
+
static_assert(sizeof(struct port_sql) <= sizeof(struct port),
"sizeof(struct port_sql) must be <= sizeof(struct port)");
+/**
+ * Dump data from port to buffer. Data in port contains tuples,
+ * metadata, or information obtained from an executed SQL query.
+ * The port is destroyed.
+ *
+ * Dumped msgpack structure:
+ * +----------------------------------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_METADATA: [ |
+ * | {IPROTO_FIELD_NAME: column name1}, |
+ * | {IPROTO_FIELD_NAME: column name2}, |
+ * | ... |
+ * | ], |
+ * | |
+ * | IPROTO_DATA: [ |
+ * | tuple, tuple, tuple, ... |
+ * | ] |
+ * | } |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_SQL_INFO: { |
+ * | SQL_INFO_ROW_COUNT: number |
+ * | SQL_INFO_AUTOINCREMENT_IDS: [ |
+ * | id, id, id, ... |
+ * | ] |
+ * | } |
+ * | } |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_SQL_INFO: { |
+ * | SQL_INFO_ROW_COUNT: number |
+ * | } |
+ * | } |
+ * +----------------------------------------------+
+ * @param port Port that contains SQL response.
+ * @param[out] out Output buffer.
+ *
+ * @retval 0 Success.
+ * @retval -1 Memory error.
+ */
static int
port_sql_dump_msgpack(struct port *port, struct obuf *out);
+
static void
-port_sql_destroy(struct port *base);
+port_sql_destroy(struct port *base)
+{
+ port_tuple_vtab.destroy(base);
+ sqlite3_finalize(((struct port_sql *)base)->stmt);
+}
-const struct port_vtab port_sql_vtab = {
- .dump_msgpack = port_sql_dump_msgpack,
- .dump_msgpack_16 = NULL,
- .dump_lua = NULL,
- .dump_plain = NULL,
- .destroy = port_sql_destroy,
+static const struct port_vtab port_sql_vtab = {
+ /* .dump_msgpack = */ port_sql_dump_msgpack,
+ /* .dump_msgpack_16 = */ NULL,
+ /* .dump_lua = */ NULL,
+ /* .dump_plain = */ NULL,
+ /* .destroy = */ port_sql_destroy,
};
+static void
+port_sql_create(struct port *port, struct sqlite3_stmt *stmt)
+{
+ port_tuple_create(port);
+ ((struct port_sql *)port)->stmt = stmt;
+ port->vtab = &port_sql_vtab;
+}
+
/**
* Return a string name of a parameter marker.
* @param Bind to get name.
@@ -386,11 +443,6 @@ sql_row_to_port(struct sqlite3_stmt *stmt, int column_count,
if (tuple == NULL)
goto error;
region_truncate(region, svp);
- /*
- * The port_tuple_add function can be used with port_sql,
- * since it does not call any port_tuple methods and works
- * only with fields.
- */
return port_tuple_add(port, tuple);
error:
@@ -522,6 +574,7 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
* column_name simply returns them.
*/
assert(name != NULL);
+ assert(type != NULL);
size += mp_sizeof_str(strlen(name));
size += mp_sizeof_str(strlen(type));
char *pos = (char *) obuf_alloc(out, size);
@@ -538,53 +591,6 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
return 0;
}
-static void
-port_sql_create(struct port *port, struct sqlite3_stmt *stmt)
-{
- port_tuple_create(port);
- ((struct port_sql *)port)->stmt = stmt;
- port->vtab = &port_sql_vtab;
-}
-
-/**
- * Dump tuples, metadata, or information obtained from an excuted
- * SQL query and saved to the port in obuf. The port is destroyed.
- *
- * Dumped msgpack structure:
- * +----------------------------------------------+
- * | IPROTO_BODY: { |
- * | IPROTO_METADATA: [ |
- * | {IPROTO_FIELD_NAME: column name1}, |
- * | {IPROTO_FIELD_NAME: column name2}, |
- * | ... |
- * | ], |
- * | |
- * | IPROTO_DATA: [ |
- * | tuple, tuple, tuple, ... |
- * | ] |
- * | } |
- * +-------------------- OR ----------------------+
- * | IPROTO_BODY: { |
- * | IPROTO_SQL_INFO: { |
- * | SQL_INFO_ROW_COUNT: number |
- * | SQL_INFO_AUTOINCREMENT_IDS: [ |
- * | id, id, id, ... |
- * | ] |
- * | } |
- * | } |
- * +-------------------- OR ----------------------+
- * | IPROTO_BODY: { |
- * | IPROTO_SQL_INFO: { |
- * | SQL_INFO_ROW_COUNT: number |
- * | } |
- * | } |
- * +----------------------------------------------+
- * @param port port with tuples, metadata or info.
- * @param out Output buffer.
- *
- * @retval 0 Success.
- * @retval -1 Memory error.
- */
static int
port_sql_dump_msgpack(struct port *port, struct obuf *out)
{
@@ -613,7 +619,6 @@ err:
goto err;
}
pos = mp_encode_uint(pos, IPROTO_DATA);
- /* Calling BaseStruct::methods via its vtab. */
if (port_tuple_vtab.dump_msgpack(port, out) < 0)
goto err;
} else {
@@ -671,14 +676,21 @@ finish:
return rc;
}
-static void
-port_sql_destroy(struct port *base)
-{
- /* Calling BaseStruct::methods via its vtab. */
- port_tuple_vtab.destroy(base);
- sqlite3_finalize(((struct port_sql *)base)->stmt);
-}
-
+/**
+ * Execute prepared SQL statement.
+ *
+ * This function uses region to allocate memory for temporary
+ * objects. After this function, region will be in the same state
+ * in which it was before this function.
+ *
+ * @param db SQL handle.
+ * @param stmt Prepared statement.
+ * @param[out] port Port to store SQL response.
+ * @param region Region to allocate temporary objects.
+ *
+ * @retval 0 Success.
+ * @retval -1 Error.
+ */
static inline int
sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
struct region *region)
@@ -711,10 +723,6 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
{
struct sqlite3_stmt *stmt;
sqlite3 *db = sql_get();
- if (db == NULL) {
- diag_set(ClientError, ER_LOADING);
- return -1;
- }
if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
return -1;
diff --git a/src/box/execute.h b/src/box/execute.h
index c90789f..1ed9ab5 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -74,7 +74,7 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
* @param len Length of @a sql.
* @param bind Array of parameters.
* @param bind_count Length of @a bind.
- * @param[out] response Response to store result.
+ * @param[out] port Port to store SQL response.
* @param region Runtime allocator for temporary objects
* (columns, tuples ...).
*
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 9dc0462..09e8914 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1643,8 +1643,10 @@ tx_process_sql(struct cmsg *m)
out = msg->connection->tx.p_obuf;
struct obuf_svp header_svp;
/* Prepare memory for the iproto header. */
- if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
+ if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {
+ port_destroy(&port);
goto error;
+ }
if (port_dump_msgpack(&port, out) != 0) {
obuf_rollback_to_svp(out, &header_svp);
goto error;
New version:
commit 1a3007030ea54fa81533acbf7421f27a2399c941
Author: Mergen Imeev <imeevma@gmail.com>
Date: Fri Dec 21 14:52:03 2018 +0300
iproto: create port_sql
This patch creates port_sql implementation for the port. This will
allow us to dump sql responses to obuf or to Lua. Also this patch
defines methods dump_msgpack() and destroy() of port_sql.
Part of #3505
diff --git a/src/box/execute.c b/src/box/execute.c
index 38b6cbc..a81f482 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -83,6 +83,93 @@ struct sql_bind {
};
/**
+ * Port implementation that is used to store SQL responses and
+ * output them to obuf or Lua. This port implementation is
+ * inherited from the port_tuple structure. This allows us to use
+ * this structure in the port_tuple methods instead of port_tuple
+ * itself.
+ *
+ * The methods of port_tuple are called via explicit access to
+ * port_tuple_vtab just like C++ does with BaseClass::method, when
+ * it is called in a child method.
+ */
+struct port_sql {
+ /* port_tuple to inherit from. */
+ struct port_tuple port_tuple;
+ /* Prepared SQL statement. */
+ struct sqlite3_stmt *stmt;
+};
+
+static_assert(sizeof(struct port_sql) <= sizeof(struct port),
+ "sizeof(struct port_sql) must be <= sizeof(struct port)");
+
+/**
+ * Dump data from port to buffer. Data in port contains tuples,
+ * metadata, or information obtained from an executed SQL query.
+ * The port is destroyed.
+ *
+ * Dumped msgpack structure:
+ * +----------------------------------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_METADATA: [ |
+ * | {IPROTO_FIELD_NAME: column name1}, |
+ * | {IPROTO_FIELD_NAME: column name2}, |
+ * | ... |
+ * | ], |
+ * | |
+ * | IPROTO_DATA: [ |
+ * | tuple, tuple, tuple, ... |
+ * | ] |
+ * | } |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_SQL_INFO: { |
+ * | SQL_INFO_ROW_COUNT: number |
+ * | SQL_INFO_AUTOINCREMENT_IDS: [ |
+ * | id, id, id, ... |
+ * | ] |
+ * | } |
+ * | } |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_SQL_INFO: { |
+ * | SQL_INFO_ROW_COUNT: number |
+ * | } |
+ * | } |
+ * +----------------------------------------------+
+ * @param port Port that contains SQL response.
+ * @param[out] out Output buffer.
+ *
+ * @retval 0 Success.
+ * @retval -1 Memory error.
+ */
+static int
+port_sql_dump_msgpack(struct port *port, struct obuf *out);
+
+static void
+port_sql_destroy(struct port *base)
+{
+ port_tuple_vtab.destroy(base);
+ sqlite3_finalize(((struct port_sql *)base)->stmt);
+}
+
+static const struct port_vtab port_sql_vtab = {
+ /* .dump_msgpack = */ port_sql_dump_msgpack,
+ /* .dump_msgpack_16 = */ NULL,
+ /* .dump_lua = */ NULL,
+ /* .dump_plain = */ NULL,
+ /* .destroy = */ port_sql_destroy,
+};
+
+static void
+port_sql_create(struct port *port, struct sqlite3_stmt *stmt)
+{
+ port_tuple_create(port);
+ ((struct port_sql *)port)->stmt = stmt;
+ port->vtab = &port_sql_vtab;
+}
+
+/**
* Return a string name of a parameter marker.
* @param Bind to get name.
* @retval Zero terminated name.
@@ -487,6 +574,7 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
* column_name simply returns them.
*/
assert(name != NULL);
+ assert(type != NULL);
size += mp_sizeof_str(strlen(name));
size += mp_sizeof_str(strlen(type));
char *pos = (char *) obuf_alloc(out, size);
@@ -503,63 +591,12 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
return 0;
}
-static inline int
-sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
- struct region *region)
+static int
+port_sql_dump_msgpack(struct port *port, struct obuf *out)
{
- int rc, column_count = sqlite3_column_count(stmt);
- if (column_count > 0) {
- /* Either ROW or DONE or ERROR. */
- while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
- if (sql_row_to_port(stmt, column_count, region,
- port) != 0)
- return -1;
- }
- assert(rc == SQLITE_DONE || rc != SQLITE_OK);
- } else {
- /* No rows. Either DONE or ERROR. */
- rc = sqlite3_step(stmt);
- assert(rc != SQLITE_ROW && rc != SQLITE_OK);
- }
- if (rc != SQLITE_DONE) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
- return -1;
- }
- return 0;
-}
-
-int
-sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
- uint32_t bind_count, struct sql_response *response,
- struct region *region)
-{
- struct sqlite3_stmt *stmt;
+ assert(port->vtab == &port_sql_vtab);
sqlite3 *db = sql_get();
- if (db == NULL) {
- diag_set(ClientError, ER_LOADING);
- return -1;
- }
- if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
- return -1;
- }
- assert(stmt != NULL);
- port_tuple_create(&response->port);
- response->prep_stmt = stmt;
- if (sql_bind(stmt, bind, bind_count) == 0 &&
- sql_execute(db, stmt, &response->port, region) == 0)
- return 0;
- port_destroy(&response->port);
- sqlite3_finalize(stmt);
- return -1;
-}
-
-int
-sql_response_dump(struct sql_response *response, struct obuf *out)
-{
- sqlite3 *db = sql_get();
- struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
- struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
+ struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
int rc = 0, column_count = sqlite3_column_count(stmt);
if (column_count > 0) {
int keys = 2;
@@ -575,26 +612,18 @@ err:
rc = -1;
goto finish;
}
- size = mp_sizeof_uint(IPROTO_DATA) +
- mp_sizeof_array(port_tuple->size);
+ size = mp_sizeof_uint(IPROTO_DATA);
pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
goto err;
}
pos = mp_encode_uint(pos, IPROTO_DATA);
- pos = mp_encode_array(pos, port_tuple->size);
- /*
- * Just like SELECT, SQL uses output format compatible
- * with Tarantool 1.6
- */
- if (port_dump_msgpack_16(&response->port, out) < 0) {
- /* Failed port dump destroyes the port. */
+ if (port_tuple_vtab.dump_msgpack(port, out) < 0)
goto err;
- }
} else {
int keys = 1;
- assert(port_tuple->size == 0);
+ assert(((struct port_tuple *)port)->size == 0);
struct stailq *autoinc_id_list =
vdbe_autoinc_id_list((struct Vdbe *)stmt);
uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
@@ -643,7 +672,66 @@ err:
}
}
finish:
- port_destroy(&response->port);
- sqlite3_finalize(stmt);
+ port_destroy(port);
return rc;
}
+
+/**
+ * Execute prepared SQL statement.
+ *
+ * This function uses region to allocate memory for temporary
+ * objects. After this function, region will be in the same state
+ * in which it was before this function.
+ *
+ * @param db SQL handle.
+ * @param stmt Prepared statement.
+ * @param[out] port Port to store SQL response.
+ * @param region Region to allocate temporary objects.
+ *
+ * @retval 0 Success.
+ * @retval -1 Error.
+ */
+static inline int
+sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
+ struct region *region)
+{
+ int rc, column_count = sqlite3_column_count(stmt);
+ if (column_count > 0) {
+ /* Either ROW or DONE or ERROR. */
+ while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
+ if (sql_row_to_port(stmt, column_count, region,
+ port) != 0)
+ return -1;
+ }
+ assert(rc == SQLITE_DONE || rc != SQLITE_OK);
+ } else {
+ /* No rows. Either DONE or ERROR. */
+ rc = sqlite3_step(stmt);
+ assert(rc != SQLITE_ROW && rc != SQLITE_OK);
+ }
+ if (rc != SQLITE_DONE) {
+ diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ return -1;
+ }
+ return 0;
+}
+
+int
+sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
+ uint32_t bind_count, struct port *port,
+ struct region *region)
+{
+ struct sqlite3_stmt *stmt;
+ sqlite3 *db = sql_get();
+ if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
+ diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ return -1;
+ }
+ assert(stmt != NULL);
+ port_sql_create(port, stmt);
+ if (sql_bind(stmt, bind, bind_count) == 0 &&
+ sql_execute(db, stmt, port, region) == 0)
+ return 0;
+ port_destroy(port);
+ return -1;
+}
diff --git a/src/box/execute.h b/src/box/execute.h
index 60b8f31..1ed9ab5 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -48,18 +48,9 @@ enum sql_info_key {
extern const char *sql_info_key_strs[];
-struct obuf;
struct region;
struct sql_bind;
-/** Response on EXECUTE request. */
-struct sql_response {
- /** Port with response data if any. */
- struct port port;
- /** Prepared SQL statement with metadata. */
- void *prep_stmt;
-};
-
/**
* Parse MessagePack array of SQL parameters.
* @param data MessagePack array of parameters. Each parameter
@@ -78,48 +69,12 @@ int
sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
/**
- * Dump a built response into @an out buffer. The response is
- * destroyed.
- * Response structure:
- * +----------------------------------------------+
- * | IPROTO_OK, sync, schema_version ... | iproto_header
- * +----------------------------------------------+---------------
- * | Body - a map with one or two keys. |
- * | |
- * | IPROTO_BODY: { |
- * | IPROTO_METADATA: [ |
- * | {IPROTO_FIELD_NAME: column name1}, |
- * | {IPROTO_FIELD_NAME: column name2}, | iproto_body
- * | ... |
- * | ], |
- * | |
- * | IPROTO_DATA: [ |
- * | tuple, tuple, tuple, ... |
- * | ] |
- * | } |
- * +-------------------- OR ----------------------+
- * | IPROTO_BODY: { |
- * | IPROTO_SQL_INFO: { |
- * | SQL_INFO_ROW_COUNT: number |
- * | } |
- * | } |
- * +----------------------------------------------+
- * @param response EXECUTE response.
- * @param out Output buffer.
- *
- * @retval 0 Success.
- * @retval -1 Memory error.
- */
-int
-sql_response_dump(struct sql_response *response, struct obuf *out);
-
-/**
* Prepare and execute an SQL statement.
* @param sql SQL statement.
* @param len Length of @a sql.
* @param bind Array of parameters.
* @param bind_count Length of @a bind.
- * @param[out] response Response to store result.
+ * @param[out] port Port to store SQL response.
* @param region Runtime allocator for temporary objects
* (columns, tuples ...).
*
@@ -128,7 +83,7 @@ sql_response_dump(struct sql_response *response, struct obuf *out);
*/
int
sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
- uint32_t bind_count, struct sql_response *response,
+ uint32_t bind_count, struct port *port,
struct region *region);
#if defined(__cplusplus)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index a08c8c5..09e8914 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1616,7 +1616,7 @@ tx_process_sql(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
struct obuf *out;
- struct sql_response response;
+ struct port port;
struct sql_bind *bind;
int bind_count;
const char *sql;
@@ -1633,7 +1633,7 @@ tx_process_sql(struct cmsg *m)
goto error;
sql = msg->sql.sql_text;
sql = mp_decode_str(&sql, &len);
- if (sql_prepare_and_execute(sql, len, bind, bind_count, &response,
+ if (sql_prepare_and_execute(sql, len, bind, bind_count, &port,
&fiber()->gc) != 0)
goto error;
/*
@@ -1643,9 +1643,11 @@ tx_process_sql(struct cmsg *m)
out = msg->connection->tx.p_obuf;
struct obuf_svp header_svp;
/* Prepare memory for the iproto header. */
- if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
+ if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {
+ port_destroy(&port);
goto error;
- if (sql_response_dump(&response, out) != 0) {
+ }
+ if (port_dump_msgpack(&port, out) != 0) {
obuf_rollback_to_svp(out, &header_svp);
goto error;
}
diff --git a/src/box/port.h b/src/box/port.h
index ad1b349..f188036 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -65,7 +65,6 @@ extern const struct port_vtab port_tuple_vtab;
static inline struct port_tuple *
port_tuple(struct port *port)
{
- assert(port->vtab == &port_tuple_vtab);
return (struct port_tuple *)port;
}
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] [PATCH v7 4/6] lua: create method dump_lua for port_sql
2019-01-15 16:10 [tarantool-patches] [PATCH v7 0/6] sql: remove box.sql.execute imeevma
` (2 preceding siblings ...)
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 3/6] iproto: create port_sql imeevma
@ 2019-01-15 16:10 ` imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 5/6] lua: parameter binding for new execute() imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 6/6] sql: check new box.sql.execute() imeevma
5 siblings, 0 replies; 13+ messages in thread
From: imeevma @ 2019-01-15 16:10 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Hi! Thank you for review! My answers, diff between versions and
new patch below.
On 12/29/18 4:19 PM, Vladislav Shpilevoy wrote:
> Thanks for the patch! See 6 comments below.
>
> On 28/12/2018 21:11, imeevma@tarantool.org wrote:
>> Hi! Thank you for review! My answers, diff between versions and
>> new version below.
>>
>> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>>> Thanks for the patch! See 7 comments below.
>>
>>> 1. Use lua_pushstring.
>> Fixed.
>>
>>> 2. It is strange, that this function creates a table and sets it
>>> as a member of some externally created table. Please, move this line
>>> to a caller function.
>> Fixed.
>>
>>> 3. Just inline keys 2.
>> Fixed.
>>
>>> 4. Why 1 array element? And why do you separate fixed/optional keys?
>>> lua_createtable does not take statis/dynamic key count. It takes
>>> array/map key count.
>> Fixed. Now it takes 1 or 2 as map key count.
>>
>>> 5. I think, port_sql_destroy should call sqlite3_finalize.
>> Fixed in patch #2.
>>
>>> 6. Impossible. It isn't?
>> Removed.
>>
>>> 7. Please, start message with a capital letter.
>> Fixed.
>
> Please, next time write answered inlined into an original
> email. With bare comments/answered without a context code it
> is hard to remember what was wrong.
>
Ok, understood.
>> commit 74f30d133b34ea86dc145f6ffee347abfd489cf2
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date: Fri Dec 21 16:48:37 2018 +0300
>>
>> lua: create method dump_lua for port_sql
>> This patch creates the dump_lua method for port_sql. It also
>> defines a new function box.sql.new_execute(), which will be
>> converted to box.sql.execute().
>> Part of #3505
>>
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index c6fcb50..4606239 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -102,12 +103,14 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
>> static int
>> port_sql_dump_msgpack(struct port *port, struct obuf *out);
>> static void
>> +port_sql_dump_lua(struct port *port, struct lua_State *L);
>
> 1. Please, add empty lines between declarations, and put
> a comment above a first function appearance.
>
Fixed. Partly fixed in previous patch.
>> +static void
>> port_sql_destroy(struct port *base);
>> const struct port_vtab port_sql_vtab = {
>> .dump_msgpack = port_sql_dump_msgpack,
>> .dump_msgpack_16 = NULL,
>> - .dump_lua = NULL,
>> + .dump_lua = port_sql_dump_lua,
>> .dump_plain = NULL,
>> .destroy = port_sql_destroy,
>> };
>> @@ -671,6 +674,87 @@ finish:
>> return rc;
>> }
>> +/**
>> + * Push a metadata of the prepared statement to Lua stack.
>
> 2. Not sure if 'metadata' word is enumerable and can be
> accompanied by article 'a'.
>
Fixed.
>> + *
>> + * @param stmt Prepared statement.
>> + * @param L Lua stack.
>> + * @param column_count Statement's column count.
>> + */
>> +static inline void
>> +lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
>> + int column_count)
>> +{
>> + assert(column_count > 0);
>> + lua_createtable(L, column_count, 0);
>> + for (int i = 0; i < column_count; ++i) {
>> + lua_createtable(L, 2, 0);
>
> 3. Read carefully what parameters of lua_createtable mean.
>
> First parameter is how many indexed elements preallocate
> for indexes [1, N]. Second parameter is how many dictionary
> key/value pairs preallocate.
>
> Here table is going to be {name = ..., type = ...} = 2 dictionary
> values. So it is lua_createtable(L, 0, 2), not (L, 2, 0).
>
Thanks, fixed.
>> + const char *name = sqlite3_column_name(stmt, i);
>> + const char *type = sqlite3_column_datatype(stmt, i);
>> + /*
>> + * Can not fail, since all column names are
>> + * preallocated during prepare phase and the
>> + * column_name simply returns them.
>> + */
>> + assert(name != NULL);
>
> 4. I guess, type != NULL as well.
>
Fixed.
>> + lua_pushstring(L, name);
>> + lua_setfield(L, -2, "name");
>> + lua_pushstring(L, type);
>> + lua_setfield(L, -2, "type");
>> + lua_rawseti(L, -2, i + 1);
>> + }
>> +}
>> +
>> +/**
>> + * Dump a built response into Lua stack. The response is
>> + * destroyed.
>> + *
>> + * @param port port with EXECUTE response.
>> + * @param L Lua stack.
>> + */
>> +static void
>> +port_sql_dump_lua(struct port *port, struct lua_State *L)
>> +{
>> + assert(port->vtab == &port_sql_vtab);
>> + sqlite3 *db = sql_get();
>> + struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
>> + int column_count = sqlite3_column_count(stmt);
>> + if (column_count > 0) {
>> + lua_createtable(L, 0, 2);
>> + lua_sql_get_description(stmt, L, column_count);
>> + lua_setfield(L, -2, "metadata");
>> + /*
>> + * We can use the port_tuple methods, since
>> + * port_sql was inherited from it.
>
> 5. Drop this comment please, everywhere.
>
Fixed, partly in previous patch.
>> + */
>> + port_tuple_vtab.dump_lua(port, L);
>> + lua_setfield(L, -2, "rows");
>> + } else {
>> + assert(((struct port_tuple *)port)->size == 0);
>> + struct stailq *autoinc_id_list =
>> + vdbe_autoinc_id_list((struct Vdbe *)stmt);
>> + lua_createtable(L, 0, stailq_empty(autoinc_id_list) ? 1 : 2);
>> +
>> + luaL_pushuint64(L, db->nChange);
>> + lua_setfield(L, -2, "rowcount");
>> +
>> + if (!stailq_empty(autoinc_id_list)) {
>> + lua_newtable(L);
>> + int i = 1;
>> + struct autoinc_id_entry *id_entry;
>> + stailq_foreach_entry(id_entry, autoinc_id_list, link) {
>> + if (id_entry->id >= 0)
>> + luaL_pushuint64(L, id_entry->id);
>> + else
>> + luaL_pushint64(L, id_entry->id);
>> + lua_rawseti(L, -2, i++);
>> + }
>> + lua_setfield(L, -2, "autoincrement_ids");
>> + }
>> + }
>> + port_destroy(port);
>> +}
>> +
>> static void
>> port_sql_destroy(struct port *base)
>> {
>> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
>> index 6923666..a55566e 100644
>> --- a/src/box/lua/sql.c
>> +++ b/src/box/lua/sql.c
>> @@ -111,6 +112,28 @@ sqlerror:
>> }
>> static int
>> +lbox_execute(struct lua_State *L)
>> +{
>> + struct sql_bind *bind = NULL;
>> + int bind_count = 0;
>> + size_t length;
>> + struct port port;
>> +
>> + if (lua_type(L, 1) != LUA_TSTRING)
>> + return luaL_error(L, "Usage: box.execute(sqlstring)");
>
> 6. Maybe just add here ' || ! lua_isstring(L, 1)' and remove the
> check for 'sql == NULL' below?
>
Thanks, fixed.
>> +
>> + const char *sql = lua_tolstring(L, 1, &length);
>> + if (sql == NULL)
>> + return luaL_error(L, "Usage: box.execute(sqlstring)");
>> +
>> + if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
>> + &fiber()->gc) != 0)
>> + return luaT_error(L);
>> + port_dump_lua(&port, L);
>> + return 1;
>> +}
>> +
>> +static int
>> lua_sql_debug(struct lua_State *L)
>> {
>> struct info_handler info;
Diff between versions:
commit 20390a0d00506eb683e0bb6e9144d47acec0937e
Author: Mergen Imeev <imeevma@gmail.com>
Date: Wed Jan 9 18:35:14 2019 +0300
Temporary: review fix
diff --git a/src/box/execute.c b/src/box/execute.c
index b8a55be..10b309a 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -147,6 +147,14 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
static int
port_sql_dump_msgpack(struct port *port, struct obuf *out);
+/**
+ * Dump data from port to Lua stack. Data in port contains tuples,
+ * metadata, or information obtained from an executed SQL query.
+ * The port is destroyed.
+ *
+ * @param port Port that contains SQL response.
+ * @param L Lua stack.
+ */
static void
port_sql_dump_lua(struct port *port, struct lua_State *L);
@@ -681,7 +689,7 @@ finish:
}
/**
- * Push a metadata of the prepared statement to Lua stack.
+ * Serialize a description of the prepared statement.
*
* @param stmt Prepared statement.
* @param L Lua stack.
@@ -694,7 +702,7 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
assert(column_count > 0);
lua_createtable(L, column_count, 0);
for (int i = 0; i < column_count; ++i) {
- lua_createtable(L, 2, 0);
+ lua_createtable(L, 0, 2);
const char *name = sqlite3_column_name(stmt, i);
const char *type = sqlite3_column_datatype(stmt, i);
/*
@@ -703,6 +711,7 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
* column_name simply returns them.
*/
assert(name != NULL);
+ assert(type != NULL);
lua_pushstring(L, name);
lua_setfield(L, -2, "name");
lua_pushstring(L, type);
@@ -711,13 +720,6 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
}
}
-/**
- * Dump a built response into Lua stack. The response is
- * destroyed.
- *
- * @param port port with EXECUTE response.
- * @param L Lua stack.
- */
static void
port_sql_dump_lua(struct port *port, struct lua_State *L)
{
@@ -729,10 +731,6 @@ port_sql_dump_lua(struct port *port, struct lua_State *L)
lua_createtable(L, 0, 2);
lua_sql_get_description(stmt, L, column_count);
lua_setfield(L, -2, "metadata");
- /*
- * We can use the port_tuple methods, since
- * port_sql was inherited from it.
- */
port_tuple_vtab.dump_lua(port, L);
lua_setfield(L, -2, "rows");
} else {
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index a55566e..f18a797 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -118,14 +118,12 @@ lbox_execute(struct lua_State *L)
int bind_count = 0;
size_t length;
struct port port;
+ int top = lua_gettop(L);
- if (lua_type(L, 1) != LUA_TSTRING)
+ if (top != 1 || ! lua_isstring(L, 1))
return luaL_error(L, "Usage: box.execute(sqlstring)");
const char *sql = lua_tolstring(L, 1, &length);
- if (sql == NULL)
- return luaL_error(L, "Usage: box.execute(sqlstring)");
-
if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
&fiber()->gc) != 0)
return luaT_error(L);
New version:
commit 530bb8859686ce901b98394c4c9a8d23a9524f48
Author: Mergen Imeev <imeevma@gmail.com>
Date: Fri Dec 21 16:48:37 2018 +0300
lua: create method dump_lua for port_sql
This patch creates the dump_lua method for port_sql. It also
defines a new function box.sql.new_execute(), which will be
converted to box.sql.execute().
Part of #3505
diff --git a/src/box/execute.c b/src/box/execute.c
index a81f482..10b309a 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -43,6 +43,7 @@
#include "port.h"
#include "tuple.h"
#include "sql/vdbe.h"
+#include "lua/utils.h"
const char *sql_type_strs[] = {
NULL,
@@ -146,6 +147,17 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
static int
port_sql_dump_msgpack(struct port *port, struct obuf *out);
+/**
+ * Dump data from port to Lua stack. Data in port contains tuples,
+ * metadata, or information obtained from an executed SQL query.
+ * The port is destroyed.
+ *
+ * @param port Port that contains SQL response.
+ * @param L Lua stack.
+ */
+static void
+port_sql_dump_lua(struct port *port, struct lua_State *L);
+
static void
port_sql_destroy(struct port *base)
{
@@ -156,7 +168,7 @@ port_sql_destroy(struct port *base)
static const struct port_vtab port_sql_vtab = {
/* .dump_msgpack = */ port_sql_dump_msgpack,
/* .dump_msgpack_16 = */ NULL,
- /* .dump_lua = */ NULL,
+ /* .dump_lua = */ port_sql_dump_lua,
/* .dump_plain = */ NULL,
/* .destroy = */ port_sql_destroy,
};
@@ -677,6 +689,77 @@ finish:
}
/**
+ * Serialize a description of the prepared statement.
+ *
+ * @param stmt Prepared statement.
+ * @param L Lua stack.
+ * @param column_count Statement's column count.
+ */
+static inline void
+lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
+ int column_count)
+{
+ assert(column_count > 0);
+ lua_createtable(L, column_count, 0);
+ for (int i = 0; i < column_count; ++i) {
+ lua_createtable(L, 0, 2);
+ const char *name = sqlite3_column_name(stmt, i);
+ const char *type = sqlite3_column_datatype(stmt, i);
+ /*
+ * Can not fail, since all column names are
+ * preallocated during prepare phase and the
+ * column_name simply returns them.
+ */
+ assert(name != NULL);
+ assert(type != NULL);
+ lua_pushstring(L, name);
+ lua_setfield(L, -2, "name");
+ lua_pushstring(L, type);
+ lua_setfield(L, -2, "type");
+ lua_rawseti(L, -2, i + 1);
+ }
+}
+
+static void
+port_sql_dump_lua(struct port *port, struct lua_State *L)
+{
+ assert(port->vtab == &port_sql_vtab);
+ sqlite3 *db = sql_get();
+ struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
+ int column_count = sqlite3_column_count(stmt);
+ if (column_count > 0) {
+ lua_createtable(L, 0, 2);
+ lua_sql_get_description(stmt, L, column_count);
+ lua_setfield(L, -2, "metadata");
+ port_tuple_vtab.dump_lua(port, L);
+ lua_setfield(L, -2, "rows");
+ } else {
+ assert(((struct port_tuple *)port)->size == 0);
+ struct stailq *autoinc_id_list =
+ vdbe_autoinc_id_list((struct Vdbe *)stmt);
+ lua_createtable(L, 0, stailq_empty(autoinc_id_list) ? 1 : 2);
+
+ luaL_pushuint64(L, db->nChange);
+ lua_setfield(L, -2, "rowcount");
+
+ if (!stailq_empty(autoinc_id_list)) {
+ lua_newtable(L);
+ int i = 1;
+ struct autoinc_id_entry *id_entry;
+ stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+ if (id_entry->id >= 0)
+ luaL_pushuint64(L, id_entry->id);
+ else
+ luaL_pushint64(L, id_entry->id);
+ lua_rawseti(L, -2, i++);
+ }
+ lua_setfield(L, -2, "autoincrement_ids");
+ }
+ }
+ port_destroy(port);
+}
+
+/**
* Execute prepared SQL statement.
*
* This function uses region to allocate memory for temporary
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 6923666..f18a797 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -6,6 +6,7 @@
#include <info.h>
#include "lua/info.h"
#include "lua/utils.h"
+#include "box/execute.h"
static void
lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
@@ -111,6 +112,26 @@ sqlerror:
}
static int
+lbox_execute(struct lua_State *L)
+{
+ struct sql_bind *bind = NULL;
+ int bind_count = 0;
+ size_t length;
+ struct port port;
+ int top = lua_gettop(L);
+
+ if (top != 1 || ! lua_isstring(L, 1))
+ return luaL_error(L, "Usage: box.execute(sqlstring)");
+
+ const char *sql = lua_tolstring(L, 1, &length);
+ if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
+ &fiber()->gc) != 0)
+ return luaT_error(L);
+ port_dump_lua(&port, L);
+ return 1;
+}
+
+static int
lua_sql_debug(struct lua_State *L)
{
struct info_handler info;
@@ -124,6 +145,7 @@ box_lua_sqlite_init(struct lua_State *L)
{
static const struct luaL_Reg module_funcs [] = {
{"execute", lua_sql_execute},
+ {"new_execute", lbox_execute},
{"debug", lua_sql_debug},
{NULL, NULL}
};
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] [PATCH v7 5/6] lua: parameter binding for new execute()
2019-01-15 16:10 [tarantool-patches] [PATCH v7 0/6] sql: remove box.sql.execute imeevma
` (3 preceding siblings ...)
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 4/6] lua: create method dump_lua for port_sql imeevma
@ 2019-01-15 16:10 ` imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 6/6] sql: check new box.sql.execute() imeevma
5 siblings, 0 replies; 13+ messages in thread
From: imeevma @ 2019-01-15 16:10 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Hi! Thank you for review! My answers, diff between versions and
new patch below.
On 12/29/18 4:19 PM, Vladislav Shpilevoy wrote:
> Thanks for the patch! See 10 comments below.
>
> On 28/12/2018 21:11, imeevma@tarantool.org wrote:
>> Hi! Thank you for review! My answers, diff between versions and
>> new version below.
>>
>>
>> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>>> Thanks for the patch! See 9 comments below.
>>
>>> 1. It is not msgpack.
>> Fixed.
>>
>>> 2. In Lua syntax {'name' : value} is incorrect.
>> Fixed.
>>
>>> 3. Just -1. Stack indexes can be negative.
>> lua_next() throws an assert when I change arg value to -1:
>>
>> tarantool: lj_api.c:892: lua_next: Assertion `(((t)->it) == (~11u))' failed.
>
> It is suspicious. I looked at lua_next source and it does
> convert negative index to positive with ease. Also, it is used
> already in cosole.c, space.cc, httpc. and other places.
>
> I tried it myself, and -2 value worked for me. Yes, -1 was
> incorrect, my fault.
>
>>
>>> 4. Please, do not pop the value stored in a luaL_field. Pop 2
>>> values at one time below, on line 294.
>> Fixed. Actually, I found out that my popping policy was quite a
>> mess in previous version. Now everything will be popped at the
>> end of the function.
>>
>>> 5. As I know, luaL_checklstring never returns NULL. It throws. Please,
>>> check for string type explicitly and throw normal usage error.
>> Fixed.
>>
>>> 6. As I said above, you've already popped it.
>> Fixed.
>>
>>> 7. Please, check for overflow.
>> Fixed.
>>
>>> 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().
>> After some discussion it was decided to left it as it is now. The
>> main reason for this is that if we remove luaL_field() than this
>> function become downgraded version of luaL_field().
>>
>>> 9. Not msgpack. Please, do not blindly copy-paste code. Check out the
>>> commit more accurately. I will not point out other places.
>> Fixed. Moved this check to sql.c.
>>
>>
>> Diff between versions:
>>
>> commit 93e4ea0ffcc195c4a904dab99c5cc516db33fa44
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date: Thu Dec 27 17:00:51 2018 +0300
>>
>> Temporary: review fixes
>>
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 457403b..ade6e6c 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -278,43 +278,43 @@ static inline int
>> lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
>> {
>> struct luaL_field field;
>> + struct region *region = &fiber()->gc;
>> 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
>> - * one key - {'name': value}.
>> - * Report parse error otherwise.
>> + * A named parameter is a single-row table that
>> + * contains one value associated with a key. The
>> + * key must be a string. This key is the name of
>> + * the parameter, and the value associated with
>> + * the key is the value of the parameter.
>
> 1. Or just change MP_MAP to table, and {'name': value} to {name = value},
> and in other places too. But it is up to you. Your comment is ok as well.
>
Fixed.
>> */
>> if (field.size != 1) {
>> diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
>> - "parameter should be {'name': value}");
>> + "named parameter should be a single-row "\
>> + "table with one key-value pair");
>> return -1;
>> }
>> /*
>> - * Get key and value of the only map element to
>> + * Get key and value of the only table element to
>> * lua stack.
>> */
>> lua_pushnil(L);
>> lua_next(L, lua_gettop(L) - 1);
>> - /* At first we should deal with the value. */
>> - luaL_tofield(L, luaL_msgpack_default, -1, &field);
>> - lua_pop(L, 1);
>> - /* 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) {
>> - diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
>> - "parameter should be {'name': value}");
>> + if (lua_type(L, -2) != LUA_TSTRING) {
>
> 2. Why so complex? Why not "! lua_isstring()" ?
>
Fixed.
>> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
>> + "parameter should be a string.");
>> return -1;
>> }
>> + size_t name_len;
>> + bind->name = lua_tolstring(L, -2, &name_len);
>> /*
>> * Name should be saved in allocated memory as it
>> * will be poped from Lua stack.
>> */
>> - buf = region_alloc(&fiber()->gc, name_len + 1);
>> + buf = region_alloc(region, name_len + 1);
>> if (buf == NULL) {
>> diag_set(OutOfMemory, name_len + 1, "region_alloc",
>> "buf");
>> @@ -323,13 +323,18 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
>> memcpy(buf, bind->name, name_len + 1);
>> bind->name = buf;
>> bind->name_len = name_len;
>> - lua_pop(L, 1);
>> + luaL_tofield(L, luaL_msgpack_default, -1, &field);
>> } else {
>> bind->name = NULL;
>> bind->name_len = 0;
>> }
>> switch (field.type) {
>> case MP_UINT: {
>> + if (field.ival < 0) {
>> + diag_set(ClientError, ER_SQL_BIND_VALUE,
>> + sql_bind_name(bind), "INTEGER");
>> + return -1;
>> + }
>
> 3. How is it possible? Lua field reports that it is an unsigned
> integer. If it is a way of checking for overflow, then please,
> make it more straightforward. I looked into the source of
> luaL_tofield and I saw that ival stores uint64_t just casting it
> to int64_t. So here you should cast it back and check if it
> is > INT64_MAX.
>
> (uint64_t) field.ival > INT64_MAX
>
> Also, you do not need braces {} for 'case MP_UINT'.
>
Fixed.
>> bind->i64 = field.ival;
>> bind->type = SQLITE_INTEGER;
>> bind->bytes = sizeof(bind->i64);
>> commit a1fe2aa7edc82097b2fcf63a41fb7fa2145b9d3c
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date: Fri Dec 21 17:54:16 2018 +0300
>>
>> lua: parameter binding for new execute()
>> This patch defines parameters binding for SQL statements executed
>> through box.
>> Part of #3505
>> Closes #3401
>>
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 4606239..ade6e6c 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -261,6 +262,196 @@ 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;
>> + struct region *region = &fiber()->gc;
>> + char *buf;
>> + lua_rawgeti(L, idx, i + 1);
>> + luaL_tofield(L, luaL_msgpack_default, -1, &field);
>
> 4. I remembered why I do not like luaL_field. When it meets a
> non-scalar value like map, it traverses it without necessity.
> Lets refactor it a bit.
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index ade6e6c95..3a45fc524 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -281,33 +281,28 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
> struct region *region = &fiber()->gc;
> 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 a single-row table that
> - * contains one value associated with a key. The
> - * key must be a string. This key is the name of
> - * the parameter, and the value associated with
> - * the key is the value of the parameter.
> - */
> - if (field.size != 1) {
> - diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> - "named parameter should be a single-row "\
> - "table with one key-value pair");
> - return -1;
> - }
> + if (lua_istable(L, -1)) {
> /*
> * Get key and value of the only table element to
> * lua stack.
> */
> lua_pushnil(L);
> - lua_next(L, lua_gettop(L) - 1);
> + lua_next(L, -2);
> if (lua_type(L, -2) != LUA_TSTRING) {
> diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
> "parameter should be a string.");
> return -1;
> }
> + /* Check that the table is one-row sized. */
> + lua_pushvalue(L, -2);
> + if (lua_next(L, -4) != 0) {
> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> + "named parameter should be a single-row "\
> + "table with one key-value pair");
> + return -1;
> + }
> + lua_pop(L, 1);
> size_t name_len;
> bind->name = lua_tolstring(L, -2, &name_len);
> /*
> @@ -323,11 +318,11 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
> memcpy(buf, bind->name, name_len + 1);
> bind->name = buf;
> bind->name_len = name_len;
> - luaL_tofield(L, luaL_msgpack_default, -1, &field);
> } else {
> bind->name = NULL;
> bind->name_len = 0;
> }
> + luaL_tofield(L, luaL_msgpack_default, -1, &field);
> switch (field.type) {
> case MP_UINT: {
> if (field.ival < 0) {
>
> Here I removed common traversal and added an explicit
> check that a table consists of one key-value pair.
>
> Are you ok with these changes? If you are - squash. But
> I did not test it though.
>
Thanks, squashed. The only change: I removed lua_pop(L, 1) that
was after lua_next(L, -4) as lua_next() pops in any case.
>> + bind->pos = i + 1;
>> + if (field.type == MP_MAP) {
>> + /*
>> + * A named parameter is a single-row table that
>> + * contains one value associated with a key. The
>> + * key must be a string. This key is the name of
>> + * the parameter, and the value associated with
>> + * the key is the value of the parameter.
>> + */
>
> 5. Are you sure this comment is necessary? It just repeats the
> error message below.
>
Removed.
>> + if (field.size != 1) {
>> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
>> + "named parameter should be a single-row "\
>> + "table with one key-value pair");
>> + return -1;
>> + }
>> + /*
>> + * Get key and value of the only table element to
>> + * lua stack.
>> + */
>> + lua_pushnil(L);
>> + lua_next(L, lua_gettop(L) - 1);
>> + if (lua_type(L, -2) != LUA_TSTRING) {
>> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
>> + "parameter should be a string.");
>> + return -1;
>> + }
>> + size_t name_len;
>> + bind->name = lua_tolstring(L, -2, &name_len);
>> + /*
>> + * Name should be saved in allocated memory as it
>> + * will be poped from Lua stack.
>> + */
>> + buf = region_alloc(region, 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;
>> + luaL_tofield(L, luaL_msgpack_default, -1, &field);
>> + } else {
>> + bind->name = NULL;
>> + bind->name_len = 0;
>> + }
>> + switch (field.type) {
>> + case MP_UINT: {
>> + if (field.ival < 0) {
>> + diag_set(ClientError, ER_SQL_BIND_VALUE,
>> + sql_bind_name(bind), "INTEGER");
>> + return -1;
>> + }
>> + bind->i64 = field.ival;
>> + bind->type = SQLITE_INTEGER;
>> + bind->bytes = sizeof(bind->i64);
>> + break;
>> + }
>> + case MP_INT:
>> + bind->i64 = field.ival;
>> + bind->type = SQLITE_INTEGER;
>> + bind->bytes = sizeof(bind->i64);
>> + break;
>
> 6. How about this?
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index ade6e6c95..c1b06b345 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -335,11 +335,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
> sql_bind_name(bind), "INTEGER");
> return -1;
> }
> - bind->i64 = field.ival;
> - bind->type = SQLITE_INTEGER;
> - bind->bytes = sizeof(bind->i64);
> - break;
> - }
> + FALLTHROUGH;
> case MP_INT:
> bind->i64 = field.ival;
> bind->type = SQLITE_INTEGER;
>
Squashed.
>> + case MP_STR:
>> + /*
>> + * Data should be saved in allocated memory as it
>> + * will be poped from Lua stack.
>> + */
>> + buf = region_alloc(region, 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;
>
> 7.
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index ade6e6c95..fb2e5ea29 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -362,10 +362,6 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
> 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;
>
Squashed.
>> + 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(region, sizeof(field));
>> + if (buf == NULL) {
>> + diag_set(OutOfMemory, sizeof(field), "region_alloc",
>> + "buf");
>> + return -1;
>> + }
>> + memcpy(buf, &field, sizeof(field));
>
> 8. Why? What a point of copying struct luaL_field onto region? As I
> see, MP_EXT is set for unknown cdata like just void *, and the same
> for unknown userdata. When void * is met, we do not even know its
> size, nor what a value it stores. When we meet userdata, we can learn
> its size, but we do not know what a value is stored here and can not
> decode it.
>
> Please, set and error when such thing happens.
>
Fixed. I used word "USERDATA" in error message to describe data
that cannot be used for binding.
>> + 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, lua_gettop(L) - idx);
>> + return 0;
>> +}
>
> 9. Also, I noticed one more problem with luaL_tofield. It can
> deliberately throw during a number decoding, see at CHECK_NUMBER
> macro. Moreover, it throws during table/map traversing in
> lua_field_inspect_table. I once tried to remove these exceptions
> but failed.
>
> Here on a throw region leaks.
>
> Also port_sql_dump_lua can throw during pushing data onto the stack
> and the whole port_sql leaks.
>
> Both problems can be solved using lua_cpcall, but please, ask in
> the server team chat if it is worth doing. As I remember, Kostja
> says 'let it crash' in such cases, but if a caller will use
>
> pcall(box.execute)
>
> then it will not crash, but just leak.
>
After discussion, it was decided that exceptions generated by
lua_error() should be removed. Any other exceptions should be left
as is.
>> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
>> index a55566e..20ba43b 100644
>> --- a/src/box/lua/sql.c
>> +++ b/src/box/lua/sql.c
>> @@ -118,13 +118,22 @@ lbox_execute(struct lua_State *L)
>> int bind_count = 0;
>> size_t length;
>> struct port port;
>> + int top = lua_gettop(L);
>> - if (lua_type(L, 1) != LUA_TSTRING)
>> - return luaL_error(L, "Usage: box.execute(sqlstring)");
>> + if ((top != 1 && top != 2) || lua_type(L, 1) != LUA_TSTRING)
>> + return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
>> const char *sql = lua_tolstring(L, 1, &length);
>> if (sql == NULL)
>> - return luaL_error(L, "Usage: box.execute(sqlstring)");
>> + return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
>
> 10. How can it be NULL, if you have checked above that here a string
> is stored?
>
Fixed.
>> +
>> + if (top == 2) {
>> + if (! lua_istable(L, 2))
>> + return luaL_error(L, "Second argument must be a table");
>> + bind_count = lua_sql_bind_list_decode(L, &bind, 2);
>> + if (bind_count < 0)
>> + return luaT_error(L);
>> + }
>> if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
>> &fiber()->gc) != 0)
>>
Diff between versions:
commit 10613d0febaca704155c57e73f3810fcc7cd2eaf
Author: Mergen Imeev <imeevma@gmail.com>
Date: Thu Jan 10 20:36:42 2019 +0300
Temporary: review fix
diff --git a/src/box/execute.c b/src/box/execute.c
index 8e76234..1e9ee32 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -347,33 +347,27 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
struct region *region = &fiber()->gc;
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 a single-row table that
- * contains one value associated with a key. The
- * key must be a string. This key is the name of
- * the parameter, and the value associated with
- * the key is the value of the parameter.
- */
- if (field.size != 1) {
- diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
- "named parameter should be a single-row "\
- "table with one key-value pair");
- return -1;
- }
+ if (lua_istable(L, -1)) {
/*
* Get key and value of the only table element to
* lua stack.
*/
lua_pushnil(L);
- lua_next(L, lua_gettop(L) - 1);
- if (lua_type(L, -2) != LUA_TSTRING) {
+ lua_next(L, -2);
+ if (! lua_isstring(L, -2)) {
diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
"parameter should be a string.");
return -1;
}
+ /* Check that the table is one-row sized. */
+ lua_pushvalue(L, -2);
+ if (lua_next(L, -4) != 0) {
+ diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
+ "named parameter should be a table with "\
+ "one key - {name = value}");
+ return -1;
+ }
size_t name_len;
bind->name = lua_tolstring(L, -2, &name_len);
/*
@@ -389,23 +383,20 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
memcpy(buf, bind->name, name_len + 1);
bind->name = buf;
bind->name_len = name_len;
- luaL_tofield(L, luaL_msgpack_default, -1, &field);
} else {
bind->name = NULL;
bind->name_len = 0;
}
+ if (luaL_tofield(L, luaL_msgpack_default, -1, &field) < 0)
+ return -1;
switch (field.type) {
- case MP_UINT: {
- if (field.ival < 0) {
+ case MP_UINT:
+ if ((uint64_t) field.ival > INT64_MAX) {
diag_set(ClientError, ER_SQL_BIND_VALUE,
sql_bind_name(bind), "INTEGER");
return -1;
}
- bind->i64 = field.ival;
- bind->type = SQLITE_INTEGER;
- bind->bytes = sizeof(bind->i64);
- break;
- }
+ FALLTHROUGH;
case MP_INT:
bind->i64 = field.ival;
bind->type = SQLITE_INTEGER;
@@ -428,10 +419,6 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
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;
@@ -452,21 +439,9 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
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(region, 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;
+ diag_set(ClientError, ER_SQL_BIND_TYPE, "USERDATA",
+ sql_bind_name(bind));
+ return -1;
case MP_ARRAY:
diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY",
sql_bind_name(bind));
New version:
commit 544b9b2a0b1f1fa5b30323282d60ac46ca7db98c
Author: Mergen Imeev <imeevma@gmail.com>
Date: Fri Dec 21 17:54:16 2018 +0300
lua: parameter binding for new execute()
This patch defines parameters binding for SQL statements executed
through box.
Part of #3505
Closes #3401
diff --git a/src/box/execute.c b/src/box/execute.c
index 10b309a..1e9ee32 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -44,6 +44,7 @@
#include "tuple.h"
#include "sql/vdbe.h"
#include "lua/utils.h"
+#include "lua/msgpack.h"
const char *sql_type_strs[] = {
NULL,
@@ -327,6 +328,171 @@ 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;
+ struct region *region = &fiber()->gc;
+ char *buf;
+ lua_rawgeti(L, idx, i + 1);
+ bind->pos = i + 1;
+ if (lua_istable(L, -1)) {
+ /*
+ * Get key and value of the only table element to
+ * lua stack.
+ */
+ lua_pushnil(L);
+ lua_next(L, -2);
+ if (! lua_isstring(L, -2)) {
+ diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
+ "parameter should be a string.");
+ return -1;
+ }
+ /* Check that the table is one-row sized. */
+ lua_pushvalue(L, -2);
+ if (lua_next(L, -4) != 0) {
+ diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
+ "named parameter should be a table with "\
+ "one key - {name = value}");
+ return -1;
+ }
+ size_t name_len;
+ bind->name = lua_tolstring(L, -2, &name_len);
+ /*
+ * Name should be saved in allocated memory as it
+ * will be poped from Lua stack.
+ */
+ buf = region_alloc(region, 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;
+ } else {
+ bind->name = NULL;
+ bind->name_len = 0;
+ }
+ if (luaL_tofield(L, luaL_msgpack_default, -1, &field) < 0)
+ return -1;
+ switch (field.type) {
+ case MP_UINT:
+ if ((uint64_t) field.ival > INT64_MAX) {
+ diag_set(ClientError, ER_SQL_BIND_VALUE,
+ sql_bind_name(bind), "INTEGER");
+ return -1;
+ }
+ FALLTHROUGH;
+ 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(region, 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:
+ 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:
+ diag_set(ClientError, ER_SQL_BIND_TYPE, "USERDATA",
+ sql_bind_name(bind));
+ return -1;
+ 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, lua_gettop(L) - idx);
+ return 0;
+}
+
+int
+lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
+ int idx)
+{
+ assert(out_bind != NULL);
+ 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
diff --git a/src/box/execute.h b/src/box/execute.h
index 1ed9ab5..2b6cd4e 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -69,6 +69,27 @@ int
sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
/**
+ * Parse Lua table of SQL parameters.
+ *
+ * @param L Lua stack contains table with parameters. Each
+ * parameter either must have scalar type, or must be a
+ * single-row table with the following format:
+ * table[name] = value. Name - string name of the named
+ * parameter, value - scalar value of the parameter.
+ * Named and positioned parameters can be mixed.
+ * For more details
+ * @sa https://www.sqlite.org/lang_expr.html#varparam.
+ * @param[out] out_bind Pointer to save decoded parameters.
+ * @param idx Position of table with parameters on Lua stack.
+ *
+ * @retval >= 0 Number of decoded parameters.
+ * @retval -1 Client or memory error.
+ */
+int
+lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
+ int idx);
+
+/**
* Prepare and execute an SQL statement.
* @param sql SQL statement.
* @param len Length of @a sql.
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index f18a797..2999b3e 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -120,10 +120,18 @@ lbox_execute(struct lua_State *L)
struct port port;
int top = lua_gettop(L);
- if (top != 1 || ! lua_isstring(L, 1))
- return luaL_error(L, "Usage: box.execute(sqlstring)");
-
+ if (! (top == 1 || top == 2) || ! lua_isstring(L, 1))
+ return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
const char *sql = lua_tolstring(L, 1, &length);
+
+ if (top == 2) {
+ if (! lua_istable(L, 2))
+ return luaL_error(L, "Second argument must be a table");
+ bind_count = lua_sql_bind_list_decode(L, &bind, 2);
+ if (bind_count < 0)
+ return luaT_error(L);
+ }
+
if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
&fiber()->gc) != 0)
return luaT_error(L);
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] [PATCH v7 6/6] sql: check new box.sql.execute()
2019-01-15 16:10 [tarantool-patches] [PATCH v7 0/6] sql: remove box.sql.execute imeevma
` (4 preceding siblings ...)
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 5/6] lua: parameter binding for new execute() imeevma
@ 2019-01-15 16:10 ` imeevma
5 siblings, 0 replies; 13+ messages in thread
From: imeevma @ 2019-01-15 16:10 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
This commit checks that new implementation of box.sql.execute() is
able to pass all tests. This is temporary commit and should be
dropped later. Even though this commit is temporary it shows that
this patch-set should be pushed after patch for issue #3832.
Needed for #3505
---
src/box/execute.c | 15 +++++++++------
src/box/execute.h | 2 +-
src/box/iproto.cc | 2 +-
src/box/lua/schema.lua | 23 +++++++++++++++++++++++
src/box/lua/sql.c | 10 +++++++---
5 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 1e9ee32..336405a 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -752,7 +752,8 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
* column_name simply returns them.
*/
assert(name != NULL);
- assert(type != NULL);
+ if (type == NULL)
+ type = "UNKNOWN";
size += mp_sizeof_str(strlen(name));
size += mp_sizeof_str(strlen(type));
char *pos = (char *) obuf_alloc(out, size);
@@ -877,6 +878,8 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
* column_name simply returns them.
*/
assert(name != NULL);
+ if (type == NULL)
+ type = "UNKNOWN";
assert(type != NULL);
lua_pushstring(L, name);
lua_setfield(L, -2, "name");
@@ -942,7 +945,7 @@ port_sql_dump_lua(struct port *port, struct lua_State *L)
*/
static inline int
sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
- struct region *region)
+ struct region *region, int error_id)
{
int rc, column_count = sqlite3_column_count(stmt);
if (column_count > 0) {
@@ -959,7 +962,7 @@ sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
assert(rc != SQLITE_ROW && rc != SQLITE_OK);
}
if (rc != SQLITE_DONE) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ diag_set(ClientError, error_id, sqlite3_errmsg(db));
return -1;
}
return 0;
@@ -968,18 +971,18 @@ sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
int
sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
uint32_t bind_count, struct port *port,
- struct region *region)
+ struct region *region, int error_id)
{
struct sqlite3_stmt *stmt;
sqlite3 *db = sql_get();
if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ diag_set(ClientError, error_id, sqlite3_errmsg(db));
return -1;
}
assert(stmt != NULL);
port_sql_create(port, stmt);
if (sql_bind(stmt, bind, bind_count) == 0 &&
- sql_execute(db, stmt, port, region) == 0)
+ sql_execute(db, stmt, port, region, error_id) == 0)
return 0;
port_destroy(port);
return -1;
diff --git a/src/box/execute.h b/src/box/execute.h
index 2b6cd4e..85efebb 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -105,7 +105,7 @@ lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
int
sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
uint32_t bind_count, struct port *port,
- struct region *region);
+ struct region *region, int error_id);
#if defined(__cplusplus)
} /* extern "C" { */
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 09e8914..a6382d8 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1634,7 +1634,7 @@ tx_process_sql(struct cmsg *m)
sql = msg->sql.sql_text;
sql = mp_decode_str(&sql, &len);
if (sql_prepare_and_execute(sql, len, bind, bind_count, &port,
- &fiber()->gc) != 0)
+ &fiber()->gc, ER_SQL_EXECUTE) != 0)
goto error;
/*
* Take an obuf only after execute(). Else the buffer can
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 8a804f0..02ec2fd 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2456,3 +2456,26 @@ box.feedback.save = function(file_name)
end
box.NULL = msgpack.NULL
+
+box.sql.execute = function(sql)
+ local result = box.sql.new_execute(sql)
+ if result == nil then return end
+ local ret = nil
+ if result.rows ~= nil then
+ ret = {}
+ for key, row in pairs(result.rows) do
+ if type(row) == 'cdata' then
+ table.insert(ret, row:totable())
+ end
+ end
+ end
+ if result.metadata ~= nil then
+ if ret == nil then ret = {} end
+ ret[0] = {}
+ for key, row in pairs(result.metadata) do
+ table.insert(ret[0], row['name'])
+ end
+ setmetatable(ret, {__serialize = 'sequence'})
+ end
+ if ret ~= nil then return ret end
+end
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 2999b3e..72e5c93 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -133,10 +133,14 @@ lbox_execute(struct lua_State *L)
}
if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
- &fiber()->gc) != 0)
- return luaT_error(L);
+ &fiber()->gc, ER_SYSTEM) != 0)
+ goto sqlerror;
port_dump_lua(&port, L);
return 1;
+
+sqlerror:
+ lua_pushstring(L, sqlite3_errmsg(sql_get()));
+ return lua_error(L);
}
static int
@@ -152,7 +156,7 @@ void
box_lua_sqlite_init(struct lua_State *L)
{
static const struct luaL_Reg module_funcs [] = {
- {"execute", lua_sql_execute},
+ {"old_execute", lua_sql_execute},
{"new_execute", lbox_execute},
{"debug", lua_sql_debug},
{NULL, NULL}
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] Re: [PATCH v7 3/6] iproto: create port_sql
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 3/6] iproto: create port_sql imeevma
@ 2019-01-16 18:25 ` Vladislav Shpilevoy
0 siblings, 0 replies; 13+ messages in thread
From: Vladislav Shpilevoy @ 2019-01-16 18:25 UTC (permalink / raw)
To: tarantool-patches, imeevma
Thanks for the patch! See 2 comments below.
> commit 1a3007030ea54fa81533acbf7421f27a2399c941
> Author: Mergen Imeev <imeevma@gmail.com>
> Date: Fri Dec 21 14:52:03 2018 +0300
>
> iproto: create port_sql
>
> This patch creates port_sql implementation for the port. This will
> allow us to dump sql responses to obuf or to Lua. Also this patch
> defines methods dump_msgpack() and destroy() of port_sql.
>
> Part of #3505
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 38b6cbc..a81f482 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -643,7 +672,66 @@ err:
> +/**
> + * Execute prepared SQL statement.
> + *
> + * This function uses region to allocate memory for temporary
> + * objects. After this function, region will be in the same state
> + * in which it was before this function.
> + *
> + * @param db SQL handle.
> + * @param stmt Prepared statement.
> + * @param[out] port Port to store SQL response.
1. It is not [out]. Port should be created before calling this function,
so it is [in][out], that we usually omit.
> + * @param region Region to allocate temporary objects.
> + *
> + * @retval 0 Success.
> + * @retval -1 Error.
> + */
> +static inline int
> +sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
> + struct region *region)
> +{
> + int rc, column_count = sqlite3_column_count(stmt);
> + if (column_count > 0) {
> + /* Either ROW or DONE or ERROR. */
> + while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
> + if (sql_row_to_port(stmt, column_count, region,
> + port) != 0)
> + return -1;
> + }
> + assert(rc == SQLITE_DONE || rc != SQLITE_OK);
> + } else {
> + /* No rows. Either DONE or ERROR. */
> + rc = sqlite3_step(stmt);
> + assert(rc != SQLITE_ROW && rc != SQLITE_OK);
> + }
> + if (rc != SQLITE_DONE) {
> + diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
> + return -1;
> + }
> + return 0;
> +}
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index a08c8c5..09e8914 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1643,9 +1643,11 @@ tx_process_sql(struct cmsg *m)
> out = msg->connection->tx.p_obuf;
> struct obuf_svp header_svp;
> /* Prepare memory for the iproto header. */
> - if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
> + if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {
> + port_destroy(&port);
> goto error;
> - if (sql_response_dump(&response, out) != 0) {
> + }
> + if (port_dump_msgpack(&port, out) != 0) {
2. port_sql_dump_msgpack destroys port on an error, but port_tuple_dump_msgpack
does not. I think, they should work similar, so port_sql_dump_msgpack does
not destroy the port as well. Lets do it, and add here port_destroy() on an
error. The same about port_dump_lua.
> obuf_rollback_to_svp(out, &header_svp);
> goto error;
> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] Re: [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield() imeevma
@ 2019-01-16 18:25 ` Vladislav Shpilevoy
2019-01-17 11:40 ` Konstantin Osipov
2019-01-17 11:41 ` Konstantin Osipov
2019-01-17 11:38 ` Konstantin Osipov
1 sibling, 2 replies; 13+ messages in thread
From: Vladislav Shpilevoy @ 2019-01-16 18:25 UTC (permalink / raw)
To: tarantool-patches, imeevma
Hi! Thanks for the patch! See 4 comments below.
On 15/01/2019 19:10, imeevma@tarantool.org wrote:
> Before this commit, the luaL_tofield() function threw a Lua
> exception when an error occurred. This behavior has been changed
> in this commit, now it sets diag_set() and returns -1 in case of
> an error.
>
> Needed for #3505
> ---
> src/box/lua/call.c | 9 +++--
> src/box/lua/tuple.c | 3 +-
> src/lua/msgpack.c | 12 ++++--
> src/lua/utils.c | 105 ++++++++++++++++++++++++++++++----------------------
> src/lua/utils.h | 8 +++-
> 5 files changed, 83 insertions(+), 54 deletions(-)
>
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 52939ae..39df05d 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -164,7 +164,8 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
> */
> for (int i = 1; i <= nrets; ++i) {
> struct luaL_field field;
> - luaL_tofield(L, cfg, i, &field);
> + if (luaL_tofield(L, cfg, i, &field) < 0)
> + luaT_error(L);
1. I know, it is weird, but usually we write return luaT_error/luaL_error().
Difference is in 'return' usage. Do not ask me why, I do not know. Maybe to
emphasize that this statement finishes the function. So please, write 'return'
where possible. Here and in other places.
> struct tuple *tuple;
> if (field.type == MP_EXT &&
> (tuple = luaT_istuple(L, i)) != NULL) {
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 978fe61..9a9fd3a 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -37,6 +37,8 @@
> #include <diag.h>
> #include <fiber.h>
>
2. Unnecessary empty line.
> +#include "box/error.h"
> +
> int luaL_nil_ref = LUA_REFNIL;
> int luaL_map_metatable_ref = LUA_REFNIL;
> int luaL_array_metatable_ref = LUA_REFNIL;
> @@ -408,10 +411,11 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
> lua_call(L, 1, 1);
> /* replace obj with the unpacked value */
> lua_replace(L, idx);
3. In my opinion there is still a problem with lua_* methods,
throwing exceptions on OOM *and on a simple stack overflow*.
Even fixed GC will not fix the problem if all memory is
occupied by non-garbage memory. I adhere to my opinion that it
is better to use lua_cpcall. There is nothing bad with it. *_pcall
just remembers a couple of registers, it is not too expensive for
such a vast function. Also lua_cpcall causes much less diff.
We can do it, for example, by renaming luaL_tofield to luaL_tofield_xc
and introducing new luaL_tofield, calling luaL_tofield_xc via
lua_cpcall. Just like we used to work with C++ *_xc wrappers.
Please, ask again in the server team chat about the proposal above.
Do not forget about stack overflow error, which also is possible and
does not mean panic. Moreover, it is worth noting that diff is going
to be much less and simpler. If they decline the proposal, I give in.
> - luaL_tofield(L, cfg, idx, field);
> - return;
> + return luaL_tofield(L, cfg, idx, field);
> } else if (!lua_isstring(L, -1)) {
> - luaL_error(L, "invalid " LUAL_SERIALIZE " value");
> + diag_set(ClientError, ER_PROC_LUA,
> + "invalid " LUAL_SERIALIZE " value");
> + return -1;
> }
>
> type = lua_tostring(L, -1);
> @@ -525,94 +539,96 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
> field->dval = num;
> CHECK_NUMBER(num);
> }
> - return;
> + break;
4. Why no to return 0? Anyway all your breaks lead to a simple 'return 0'.
> @@ -620,14 +636,15 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
> field->sval.len = 0;
> if (lua_touserdata(L, index) == NULL) {
> field->type = MP_NIL;
> - return;
> + break;
> }
> /* Fall through */
> default:
> field->type = MP_EXT;
> - return;
> + break;
> }
> #undef CHECK_NUMBER
> + return 0;
> }
>
> void
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] Re: [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield() imeevma
2019-01-16 18:25 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-01-17 11:38 ` Konstantin Osipov
2019-01-17 13:15 ` Vladislav Shpilevoy
1 sibling, 1 reply; 13+ messages in thread
From: Konstantin Osipov @ 2019-01-17 11:38 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
* imeevma@tarantool.org <imeevma@tarantool.org> [19/01/15 19:14]:
> Before this commit, the luaL_tofield() function threw a Lua
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 978fe61..9a9fd3a 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -37,6 +37,8 @@
> #include <diag.h>
> #include <fiber.h>
>
> +#include "box/error.h"
Please don't. This violates encapsulation.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] Re: [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-16 18:25 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-01-17 11:40 ` Konstantin Osipov
2019-01-17 11:41 ` Konstantin Osipov
1 sibling, 0 replies; 13+ messages in thread
From: Konstantin Osipov @ 2019-01-17 11:40 UTC (permalink / raw)
To: tarantool-patches; +Cc: imeevma
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/01/16 21:29]:
> 3. In my opinion there is still a problem with lua_* methods,
> throwing exceptions on OOM *and on a simple stack overflow*.
> Even fixed GC will not fix the problem if all memory is
> occupied by non-garbage memory. I adhere to my opinion that it
> is better to use lua_cpcall. There is nothing bad with it. *_pcall
> just remembers a couple of registers, it is not too expensive for
> such a vast function. Also lua_cpcall causes much less diff.
>
> We can do it, for example, by renaming luaL_tofield to luaL_tofield_xc
> and introducing new luaL_tofield, calling luaL_tofield_xc via
> lua_cpcall. Just like we used to work with C++ *_xc wrappers.
>
> Please, ask again in the server team chat about the proposal above.
> Do not forget about stack overflow error, which also is possible and
> does not mean panic. Moreover, it is worth noting that diff is going
> to be much less and simpler. If they decline the proposal, I give in.
Would cpcall help in this case? The patch itself is on track. For
stack issues we should use checkstack(). For general out of memory
issues it's OK to use cpcall, but AFAIU we can add cpcall any time
we need - it's a one line change. The problem is with errors which
can not be caught with cpcall(), isn't it.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] Re: [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-16 18:25 ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-17 11:40 ` Konstantin Osipov
@ 2019-01-17 11:41 ` Konstantin Osipov
1 sibling, 0 replies; 13+ messages in thread
From: Konstantin Osipov @ 2019-01-17 11:41 UTC (permalink / raw)
To: tarantool-patches; +Cc: imeevma
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/01/16 21:29]:
I don't mind if we have a cpcall even in scope of this patch - but
let's test these errors, not just rewrite the code and hope for
the best.
Can we have any kind of test which runs in short time for lua
errors?
> 3. In my opinion there is still a problem with lua_* methods,
> throwing exceptions on OOM *and on a simple stack overflow*.
> Even fixed GC will not fix the problem if all memory is
> occupied by non-garbage memory. I adhere to my opinion that it
> is better to use lua_cpcall. There is nothing bad with it. *_pcall
> just remembers a couple of registers, it is not too expensive for
> such a vast function. Also lua_cpcall causes much less diff.
>
> We can do it, for example, by renaming luaL_tofield to luaL_tofield_xc
> and introducing new luaL_tofield, calling luaL_tofield_xc via
> lua_cpcall. Just like we used to work with C++ *_xc wrappers.
>
> Please, ask again in the server team chat about the proposal above.
> Do not forget about stack overflow error, which also is possible and
> does not mean panic. Moreover, it is worth noting that diff is going
> to be much less and simpler. If they decline the proposal, I give in.
>
> > - luaL_tofield(L, cfg, idx, field);
> > - return;
> > + return luaL_tofield(L, cfg, idx, field);
> > } else if (!lua_isstring(L, -1)) {
> > - luaL_error(L, "invalid " LUAL_SERIALIZE " value");
> > + diag_set(ClientError, ER_PROC_LUA,
> > + "invalid " LUAL_SERIALIZE " value");
> > + return -1;
> > }
> > type = lua_tostring(L, -1);
> > @@ -525,94 +539,96 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
> > field->dval = num;
> > CHECK_NUMBER(num);
> > }
> > - return;
> > + break;
>
> 4. Why no to return 0? Anyway all your breaks lead to a simple 'return 0'.
>
> > @@ -620,14 +636,15 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
> > field->sval.len = 0;
> > if (lua_touserdata(L, index) == NULL) {
> > field->type = MP_NIL;
> > - return;
> > + break;
> > }
> > /* Fall through */
> > default:
> > field->type = MP_EXT;
> > - return;
> > + break;
> > }
> > #undef CHECK_NUMBER
> > + return 0;
> > }
> > void
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tarantool-patches] Re: [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-17 11:38 ` Konstantin Osipov
@ 2019-01-17 13:15 ` Vladislav Shpilevoy
0 siblings, 0 replies; 13+ messages in thread
From: Vladislav Shpilevoy @ 2019-01-17 13:15 UTC (permalink / raw)
To: Konstantin Osipov, tarantool-patches
On 17/01/2019 14:38, Konstantin Osipov wrote:
> * imeevma@tarantool.org <imeevma@tarantool.org> [19/01/15 19:14]:
>> Before this commit, the luaL_tofield() function threw a Lua
>> diff --git a/src/lua/utils.c b/src/lua/utils.c
>> index 978fe61..9a9fd3a 100644
>> --- a/src/lua/utils.c
>> +++ b/src/lua/utils.c
>> @@ -37,6 +37,8 @@
>> #include <diag.h>
>> #include <fiber.h>
>>
>> +#include "box/error.h"
>
> Please don't. This violates encapsulation.
>
>
Oh, of course. Sorry, dismissed it. Mergen, we can not
use ClientError outside of box/ module. See exception.h for
available errors. I think, LuajitError is the most suitable
here.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-01-17 13:15 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-15 16:10 [tarantool-patches] [PATCH v7 0/6] sql: remove box.sql.execute imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield() imeevma
2019-01-16 18:25 ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-17 11:40 ` Konstantin Osipov
2019-01-17 11:41 ` Konstantin Osipov
2019-01-17 11:38 ` Konstantin Osipov
2019-01-17 13:15 ` Vladislav Shpilevoy
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 2/6] iproto: move map creation to sql_response_dump() imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 3/6] iproto: create port_sql imeevma
2019-01-16 18:25 ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 4/6] lua: create method dump_lua for port_sql imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 5/6] lua: parameter binding for new execute() imeevma
2019-01-15 16:10 ` [tarantool-patches] [PATCH v7 6/6] sql: check new box.sql.execute() imeevma
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox