* [tarantool-patches] [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-19 13:20 [tarantool-patches] [PATCH v8 0/6] sql: remove box.sql.execute imeevma
@ 2019-01-19 13:20 ` imeevma
2019-01-22 19:58 ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-29 20:42 ` Vladislav Shpilevoy
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 2/6] iproto: move map creation to sql_response_dump() imeevma
` (5 subsequent siblings)
6 siblings, 2 replies; 15+ messages in thread
From: imeevma @ 2019-01-19 13:20 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Hi! Thank you for review. My answers, diff between versions and
new version below.
On 1/16/19 9:25 PM, Vladislav Shpilevoy wrote:
> 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.
>
Fixed in functions that return int.
>> 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.
>
Removed header.
>> +#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.
>
As I understood, this suggestion was rejected.
>> - 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'.
>
Fixed.
>> @@ -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
On 1/17/19 4:15 PM, Vladislav Shpilevoy wrote:
>
>
> 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.
>
Thanks, fixed.
Diff between versions:
commit 07bcff72336fe861fd51d895fe088662a9d09fe4
Author: Mergen Imeev <imeevma@gmail.com>
Date: Thu Jan 17 15:00:27 2019 +0300
Temporary: Review fixes
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 39df05d..2049ee5 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -165,7 +165,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
for (int i = 1; i <= nrets; ++i) {
struct luaL_field field;
if (luaL_tofield(L, cfg, i, &field) < 0)
- luaT_error(L);
+ return luaT_error(L);
struct tuple *tuple;
if (field.type == MP_EXT &&
(tuple = luaT_istuple(L, i)) != NULL) {
@@ -194,7 +194,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
*/
struct luaL_field root;
if (luaL_tofield(L, cfg, 1, &root) < 0)
- luaT_error(L);
+ return luaT_error(L);
struct tuple *tuple;
if (root.type == MP_EXT && (tuple = luaT_istuple(L, 1)) != NULL) {
/* `return box.tuple()` */
@@ -224,7 +224,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
lua_rawgeti(L, 1, t);
struct luaL_field field;
if (luaL_tofield(L, cfg, -1, &field) < 0)
- luaT_error(L);
+ return 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/lua/msgpack.c b/src/lua/msgpack.c
index d00ade9..1b1874e 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -150,11 +150,11 @@ restart: /* used by MP_EXT */
while (lua_next(L, top) != 0) {
lua_pushvalue(L, -2); /* push a copy of key to top */
if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
- luaT_error(L);
+ return luaT_error(L);
luamp_encode_r(L, cfg, stream, field, level + 1);
lua_pop(L, 1); /* pop a copy of key */
if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
- luaT_error(L);
+ return luaT_error(L);
luamp_encode_r(L, cfg, stream, field, level + 1);
lua_pop(L, 1); /* pop value */
}
@@ -171,7 +171,7 @@ restart: /* used by MP_EXT */
for (uint32_t i = 0; i < size; i++) {
lua_rawgeti(L, top, i + 1);
if (luaL_tofield(L, cfg, top + 1, field) < 0)
- luaT_error(L);
+ return luaT_error(L);
luamp_encode_r(L, cfg, stream, field, level + 1);
lua_pop(L, 1);
}
@@ -207,7 +207,7 @@ luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
struct luaL_field field;
if (luaL_tofield(L, cfg, lua_gettop(L), &field) < 0)
- luaT_error(L);
+ return 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 9a9fd3a..c10403d 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -37,8 +37,6 @@
#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;
@@ -413,8 +411,7 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
lua_replace(L, idx);
return luaL_tofield(L, cfg, idx, field);
} else if (!lua_isstring(L, -1)) {
- diag_set(ClientError, ER_PROC_LUA,
- "invalid " LUAL_SERIALIZE " value");
+ diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
return -1;
}
@@ -438,8 +435,7 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
lua_pop(L, 1); /* type */
return 0;
} else {
- diag_set(ClientError, ER_PROC_LUA,
- "invalid " LUAL_SERIALIZE " value");
+ diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
return -1;
}
@@ -473,8 +469,7 @@ skip:
max > size * (uint32_t)cfg->encode_sparse_ratio &&
max > (uint32_t)cfg->encode_sparse_safe) {
if (!cfg->encode_sparse_convert) {
- diag_set(ClientError, ER_PROC_LUA,
- "excessively sparse array");
+ diag_set(LuajitError, "excessively sparse array");
return -1;
}
field->type = MP_MAP;
@@ -515,8 +510,7 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
#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"); \
+ diag_set(LuajitError, "number must not be NaN or Inf"); \
return -1; \
} \
field->type = MP_NIL; \
@@ -539,7 +533,7 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
field->dval = num;
CHECK_NUMBER(num);
}
- break;
+ return 0;
case LUA_TCDATA:
{
GCcdata *cd = cdataV(L->base + index - 1);
@@ -550,85 +544,84 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
case CTID_BOOL:
field->type = MP_BOOL;
field->bval = *(bool*) cdata;
- break;
+ return 0;
case CTID_CCHAR:
case CTID_INT8:
ival = *(int8_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- break;
+ return 0;
case CTID_INT16:
ival = *(int16_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- break;
+ return 0;
case CTID_INT32:
ival = *(int32_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- break;
+ return 0;
case CTID_INT64:
ival = *(int64_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- break;
+ return 0;
case CTID_UINT8:
field->type = MP_UINT;
field->ival = *(uint8_t *) cdata;
- break;
+ return 0;
case CTID_UINT16:
field->type = MP_UINT;
field->ival = *(uint16_t *) cdata;
- break;
+ return 0;
case CTID_UINT32:
field->type = MP_UINT;
field->ival = *(uint32_t *) cdata;
- break;
+ return 0;
case CTID_UINT64:
field->type = MP_UINT;
field->ival = *(uint64_t *) cdata;
- break;
+ return 0;
case CTID_FLOAT:
field->type = MP_FLOAT;
field->fval = *(float *) cdata;
CHECK_NUMBER(field->fval);
- break;
+ return 0;
case CTID_DOUBLE:
field->type = MP_DOUBLE;
field->dval = *(double *) cdata;
CHECK_NUMBER(field->dval);
- break;
+ return 0;
case CTID_P_CVOID:
case CTID_P_VOID:
if (*(void **) cdata == NULL) {
field->type = MP_NIL;
- break;
+ return 0;
}
/* Fall through */
default:
field->type = MP_EXT;
- break;
}
- break;
+ return 0;
}
case LUA_TBOOLEAN:
field->type = MP_BOOL;
field->bval = lua_toboolean(L, index);
- break;
+ return 0;
case LUA_TNIL:
field->type = MP_NIL;
- break;
+ return 0;
case LUA_TSTRING:
field->sval.data = lua_tolstring(L, index, &size);
field->sval.len = (uint32_t) size;
field->type = MP_STR;
- break;
+ return 0;
case LUA_TTABLE:
{
field->compact = false;
if (lua_field_inspect_table(L, cfg, index, field) < 0)
return -1;
- break;
+ return 0;
}
case LUA_TLIGHTUSERDATA:
case LUA_TUSERDATA:
@@ -636,12 +629,11 @@ 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;
- break;
+ return 0;
}
/* Fall through */
default:
field->type = MP_EXT;
- break;
}
#undef CHECK_NUMBER
return 0;
New versions:
commit 3080ab1d7d2e8d8716ac91fc71150db7cde4ffe4
Author: Mergen Imeev <imeevma@gmail.com>
Date: Fri Jan 11 21:16:12 2019 +0300
lua: remove exceptions from function luaL_tofield()
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
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 52939ae..2049ee5 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)
+ return 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)
+ return 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)
+ return 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..1b1874e 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)
+ return 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)
+ return 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)
+ return 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)
+ return 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..c10403d 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -383,12 +383,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 +409,10 @@ 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(LuajitError, "invalid " LUAL_SERIALIZE " value");
+ return -1;
}
type = lua_tostring(L, -1);
@@ -424,7 +425,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 +433,10 @@ 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(LuajitError, "invalid " LUAL_SERIALIZE " value");
+ return -1;
}
skip:
@@ -456,7 +458,7 @@ skip:
}
field->type = MP_MAP;
field->size = size;
- return;
+ return 0;
}
if (k > max)
max = k;
@@ -466,15 +468,18 @@ 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(LuajitError, "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 +492,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 +507,12 @@ 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(LuajitError, "number must not be NaN or Inf"); \
+ return -1; \
+ } \
field->type = MP_NIL; \
}})
@@ -525,94 +533,95 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
field->dval = num;
CHECK_NUMBER(num);
}
- return;
+ return 0;
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;
+ return 0;
case CTID_CCHAR:
case CTID_INT8:
ival = *(int8_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- return;
+ return 0;
case CTID_INT16:
ival = *(int16_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- return;
+ return 0;
case CTID_INT32:
ival = *(int32_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- return;
+ return 0;
case CTID_INT64:
ival = *(int64_t *) cdata;
field->type = (ival >= 0) ? MP_UINT : MP_INT;
field->ival = ival;
- return;
+ return 0;
case CTID_UINT8:
field->type = MP_UINT;
field->ival = *(uint8_t *) cdata;
- return;
+ return 0;
case CTID_UINT16:
field->type = MP_UINT;
field->ival = *(uint16_t *) cdata;
- return;
+ return 0;
case CTID_UINT32:
field->type = MP_UINT;
field->ival = *(uint32_t *) cdata;
- return;
+ return 0;
case CTID_UINT64:
field->type = MP_UINT;
field->ival = *(uint64_t *) cdata;
- return;
+ return 0;
case CTID_FLOAT:
field->type = MP_FLOAT;
field->fval = *(float *) cdata;
CHECK_NUMBER(field->fval);
- return;
+ return 0;
case CTID_DOUBLE:
field->type = MP_DOUBLE;
field->dval = *(double *) cdata;
CHECK_NUMBER(field->dval);
- return;
+ return 0;
case CTID_P_CVOID:
case CTID_P_VOID:
if (*(void **) cdata == NULL) {
field->type = MP_NIL;
- return;
+ return 0;
}
/* Fall through */
default:
field->type = MP_EXT;
- return;
}
- return;
+ return 0;
}
case LUA_TBOOLEAN:
field->type = MP_BOOL;
field->bval = lua_toboolean(L, index);
- return;
+ return 0;
case LUA_TNIL:
field->type = MP_NIL;
- return;
+ return 0;
case LUA_TSTRING:
field->sval.data = lua_tolstring(L, index, &size);
field->sval.len = (uint32_t) size;
field->type = MP_STR;
- return;
+ return 0;
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;
+ return 0;
}
case LUA_TLIGHTUSERDATA:
case LUA_TUSERDATA:
@@ -620,14 +629,14 @@ 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;
+ return 0;
}
/* Fall through */
default:
field->type = MP_EXT;
- return;
}
#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] 15+ messages in thread* [tarantool-patches] Re: [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield() imeevma
@ 2019-01-22 19:58 ` Vladislav Shpilevoy
2019-01-24 7:27 ` Imeev Mergen
2019-01-29 20:42 ` Vladislav Shpilevoy
1 sibling, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2019-01-22 19:58 UTC (permalink / raw)
To: tarantool-patches, imeevma
Thanks for the fixes!
> Diff between versions:
>
> commit 07bcff72336fe861fd51d895fe088662a9d09fe4
> Author: Mergen Imeev <imeevma@gmail.com>
> Date: Thu Jan 17 15:00:27 2019 +0300
>
> Temporary: Review fixes
Please, do not merge diff of all comments into a single one. It
is hardly possible to restore which part of this diff corresponds
to which comment. Inline diff, fixing one comment, under this comment,
like Nikita does.
Also, I see that the branch still stores the old patchset. Please,
push it. Until that I can not review it.
> commit 3080ab1d7d2e8d8716ac91fc71150db7cde4ffe4
> Author: Mergen Imeev <imeevma@gmail.com>
> Date: Fri Jan 11 21:16:12 2019 +0300
>
> lua: remove exceptions from function luaL_tofield()
>
> 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
Please, use #, not ... .
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-22 19:58 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-01-24 7:27 ` Imeev Mergen
0 siblings, 0 replies; 15+ messages in thread
From: Imeev Mergen @ 2019-01-24 7:27 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Hi! Thank you for comments. My answers below.
On 1/22/19 10:58 PM, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
>> Diff between versions:
>>
>> commit 07bcff72336fe861fd51d895fe088662a9d09fe4
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date: Thu Jan 17 15:00:27 2019 +0300
>>
>> Temporary: Review fixes
>
> Please, do not merge diff of all comments into a single one. It
> is hardly possible to restore which part of this diff corresponds
> to which comment. Inline diff, fixing one comment, under this comment,
> like Nikita does.
>
> Also, I see that the branch still stores the old patchset. Please,
> push it. Until that I can not review it.
Ok, understood.
>
>> commit 3080ab1d7d2e8d8716ac91fc71150db7cde4ffe4
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date: Fri Jan 11 21:16:12 2019 +0300
>>
>> lua: remove exceptions from function luaL_tofield()
>> 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
>
> Please, use #, not ... .
Sorry, I actually wrote wrong branch name. Right branch is:
https://github.com/tarantool/tarantool/tree/imeevma/gh-3505-replace-box_sql_execute-by-box_execute
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield()
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield() imeevma
2019-01-22 19:58 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-01-29 20:42 ` Vladislav Shpilevoy
1 sibling, 0 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2019-01-29 20:42 UTC (permalink / raw)
To: tarantool-patches, imeevma
Hi! Thanks for the patch! See 2 comments below.
> @@ -525,94 +533,95 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
> case LUA_TBOOLEAN:
> field->type = MP_BOOL;
> field->bval = lua_toboolean(L, index);
> - return;
> + return 0;
> case LUA_TNIL:
> field->type = MP_NIL;
> - return;
> + return 0;
> case LUA_TSTRING:
> field->sval.data = lua_tolstring(L, index, &size);
> field->sval.len = (uint32_t) size;
> field->type = MP_STR;
> - return;
> + return 0;
> 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;
> + return 0;
1. Why not simply 'return lua_field_inspect_table' ?
> }
> case LUA_TLIGHTUSERDATA:
> case LUA_TUSERDATA:
> 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
> @@ -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
>
>
2. Looking at lua_field_inspect_table I found that if a table has __serialize
metamethod, it is called without a protection (utils.c:409). __serialize is an
arbitrary unprotected user code, that can throw an error deliberately. What are
we gonna do with that? Personally I've already faced with some user code, throwing
an error from __serialize, deliberately. On not critical errors not meaning panic.
As a solution we could 1) do not care, again; 2) finally accept the fact that
wrap into a pcall was not so bad and use it; 3) use lua_pcall in that
particular place. Please, consult Kostja, what does he choose.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] [PATCH v8 2/6] iproto: move map creation to sql_response_dump()
2019-01-19 13:20 [tarantool-patches] [PATCH v8 0/6] sql: remove box.sql.execute imeevma
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield() imeevma
@ 2019-01-19 13:20 ` imeevma
2019-01-25 16:07 ` [tarantool-patches] " Konstantin Osipov
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 3/6] iproto: create port_sql imeevma
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: imeevma @ 2019-01-19 13:20 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] 15+ messages in thread* [tarantool-patches] Re: [PATCH v8 2/6] iproto: move map creation to sql_response_dump()
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 2/6] iproto: move map creation to sql_response_dump() imeevma
@ 2019-01-25 16:07 ` Konstantin Osipov
2019-01-29 20:42 ` Vladislav Shpilevoy
0 siblings, 1 reply; 15+ messages in thread
From: Konstantin Osipov @ 2019-01-25 16:07 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
* imeevma@tarantool.org <imeevma@tarantool.org> [19/01/19 23:21]:
> 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.
This patch seems to be trivial, Vlad, please push if you're OK
with it.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH v8 2/6] iproto: move map creation to sql_response_dump()
2019-01-25 16:07 ` [tarantool-patches] " Konstantin Osipov
@ 2019-01-29 20:42 ` Vladislav Shpilevoy
0 siblings, 0 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2019-01-29 20:42 UTC (permalink / raw)
To: Konstantin Osipov, tarantool-patches
Pushed to 2.1.
On 25/01/2019 19:07, Konstantin Osipov wrote:
> * imeevma@tarantool.org <imeevma@tarantool.org> [19/01/19 23:21]:
>> 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.
>
> This patch seems to be trivial, Vlad, please push if you're OK
> with it.
>
> --
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] [PATCH v8 3/6] iproto: create port_sql
2019-01-19 13:20 [tarantool-patches] [PATCH v8 0/6] sql: remove box.sql.execute imeevma
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield() imeevma
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 2/6] iproto: move map creation to sql_response_dump() imeevma
@ 2019-01-19 13:20 ` imeevma
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 4/6] lua: create method dump_lua for port_sql imeevma
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: imeevma @ 2019-01-19 13:20 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Hi! Thank you for review. My answers, diff between versions and
new version below.
On 1/16/19 9:25 PM, Vladislav Shpilevoy wrote:
> 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.
>
Fixed.
>> + * @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.
>
Fixed. Fix for port_dump_lua() in next patch.
>> obuf_rollback_to_svp(out, &header_svp);
>> goto error;
>> }
Diff between versions:
commit 12718781c2a23ceb8b20b6a23e71ec1deeb8e3be
Author: Mergen Imeev <imeevma@gmail.com>
Date: Thu Jan 17 14:24:02 2019 +0300
Temporary: Review fixes
diff --git a/src/box/execute.c b/src/box/execute.c
index a81f482..1658d9b 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -106,7 +106,6 @@ static_assert(sizeof(struct port_sql) <= 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:
* +----------------------------------------------+
@@ -597,30 +596,27 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
assert(port->vtab == &port_sql_vtab);
sqlite3 *db = sql_get();
struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
- int rc = 0, column_count = sqlite3_column_count(stmt);
+ int 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;
+ return -1;
}
pos = mp_encode_map(pos, keys);
- if (sql_get_description(stmt, out, column_count) != 0) {
-err:
- rc = -1;
- goto finish;
- }
+ if (sql_get_description(stmt, out, column_count) != 0)
+ return -1;
size = mp_sizeof_uint(IPROTO_DATA);
pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
- goto err;
+ return -1;
}
pos = mp_encode_uint(pos, IPROTO_DATA);
if (port_tuple_vtab.dump_msgpack(port, out) < 0)
- goto err;
+ return -1;
} else {
int keys = 1;
assert(((struct port_tuple *)port)->size == 0);
@@ -633,7 +629,7 @@ err:
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
- goto err;
+ return -1;
}
pos = mp_encode_map(pos, keys);
pos = mp_encode_uint(pos, IPROTO_SQL_INFO);
@@ -656,7 +652,7 @@ err:
char *buf = obuf_alloc(out, size);
if (buf == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "buf");
- goto err;
+ return -1;
}
buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
buf = mp_encode_uint(buf, changes);
@@ -671,9 +667,7 @@ err:
}
}
}
-finish:
- port_destroy(port);
- return rc;
+ return 0;
}
/**
@@ -685,7 +679,7 @@ finish:
*
* @param db SQL handle.
* @param stmt Prepared statement.
- * @param[out] port Port to store SQL response.
+ * @param port Port to store SQL response.
* @param region Region to allocate temporary objects.
*
* @retval 0 Success.
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 09e8914..896599d 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1648,9 +1648,11 @@ tx_process_sql(struct cmsg *m)
goto error;
}
if (port_dump_msgpack(&port, out) != 0) {
+ port_destroy(&port);
obuf_rollback_to_svp(out, &header_svp);
goto error;
}
+ port_destroy(&port);
iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
iproto_wpos_create(&msg->wpos, out);
return;
New version:
commit 84a0a8b7c7f923ef3ef9d65300703569f17556fd
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..1658d9b 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -83,6 +83,92 @@ 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.
+ *
+ * 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 +573,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,98 +590,36 @@ 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;
- int rc = 0, column_count = sqlite3_column_count(stmt);
+ struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
+ int 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;
+ return -1;
}
pos = mp_encode_map(pos, keys);
- if (sql_get_description(stmt, out, column_count) != 0) {
-err:
- rc = -1;
- goto finish;
- }
- size = mp_sizeof_uint(IPROTO_DATA) +
- mp_sizeof_array(port_tuple->size);
+ if (sql_get_description(stmt, out, column_count) != 0)
+ return -1;
+ size = mp_sizeof_uint(IPROTO_DATA);
pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
- goto err;
+ return -1;
}
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. */
- goto err;
- }
+ if (port_tuple_vtab.dump_msgpack(port, out) < 0)
+ return -1;
} 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;
@@ -604,7 +629,7 @@ err:
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
- goto err;
+ return -1;
}
pos = mp_encode_map(pos, keys);
pos = mp_encode_uint(pos, IPROTO_SQL_INFO);
@@ -627,7 +652,7 @@ err:
char *buf = obuf_alloc(out, size);
if (buf == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "buf");
- goto err;
+ return -1;
}
buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
buf = mp_encode_uint(buf, changes);
@@ -642,8 +667,65 @@ err:
}
}
}
-finish:
- port_destroy(&response->port);
- sqlite3_finalize(stmt);
- return rc;
+ return 0;
+}
+
+/**
+ * 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 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..896599d 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,12 +1643,16 @@ 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) {
+ port_destroy(&port);
obuf_rollback_to_svp(out, &header_svp);
goto error;
}
+ port_destroy(&port);
iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
iproto_wpos_create(&msg->wpos, out);
return;
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] 15+ messages in thread* [tarantool-patches] [PATCH v8 4/6] lua: create method dump_lua for port_sql
2019-01-19 13:20 [tarantool-patches] [PATCH v8 0/6] sql: remove box.sql.execute imeevma
` (2 preceding siblings ...)
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 3/6] iproto: create port_sql imeevma
@ 2019-01-19 13:20 ` imeevma
2019-01-29 20:42 ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 5/6] lua: parameter binding for new execute() imeevma
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: imeevma @ 2019-01-19 13:20 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Diff between versions:
commit 02605eb00e06bbf105ee391d51f810fc94c4ab36
Author: Mergen Imeev <imeevma@gmail.com>
Date: Thu Jan 17 14:26:33 2019 +0300
Temporary: Review fixes
diff --git a/src/box/execute.c b/src/box/execute.c
index 78aab93..7abf5a6 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -149,7 +149,6 @@ 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.
@@ -750,7 +749,6 @@ port_sql_dump_lua(struct port *port, struct lua_State *L)
lua_setfield(L, -2, "autoincrement_ids");
}
}
- port_destroy(port);
}
/**
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index f18a797..2a9ad3b 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -128,6 +128,7 @@ lbox_execute(struct lua_State *L)
&fiber()->gc) != 0)
return luaT_error(L);
port_dump_lua(&port, L);
+ port_destroy(&port);
return 1;
}
New version:
commit 78e3d54239d51ad52ebf377acb016c66426299ec
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 1658d9b..7abf5a6 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,
@@ -145,6 +146,16 @@ 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.
+ *
+ * @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)
{
@@ -155,7 +166,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,
};
@@ -671,6 +682,76 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
}
/**
+ * 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");
+ }
+ }
+}
+
+/**
* 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..2a9ad3b 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,27 @@ 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);
+ port_destroy(&port);
+ return 1;
+}
+
+static int
lua_sql_debug(struct lua_State *L)
{
struct info_handler info;
@@ -124,6 +146,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] 15+ messages in thread* [tarantool-patches] Re: [PATCH v8 4/6] lua: create method dump_lua for port_sql
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 4/6] lua: create method dump_lua for port_sql imeevma
@ 2019-01-29 20:42 ` Vladislav Shpilevoy
0 siblings, 0 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2019-01-29 20:42 UTC (permalink / raw)
To: imeevma, tarantool-patches
Thanks for the patch! See 2 comments below.
> commit 78e3d54239d51ad52ebf377acb016c66426299ec
> 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
>
1. Please, replace ... with # in all patches, as I asked in the
review of the first commit.
2. I think, that it is time to finally finish this patchset
and remove "new_execute" from this commit, and in the
last commit replace "sql.execute" with "execute".
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] [PATCH v8 5/6] lua: parameter binding for new execute()
2019-01-19 13:20 [tarantool-patches] [PATCH v8 0/6] sql: remove box.sql.execute imeevma
` (3 preceding siblings ...)
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 4/6] lua: create method dump_lua for port_sql imeevma
@ 2019-01-19 13:20 ` imeevma
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 6/6] sql: check new box.sql.execute() imeevma
2019-01-22 19:58 ` [tarantool-patches] Re: [PATCH v8 0/6] sql: remove box.sql.execute Vladislav Shpilevoy
6 siblings, 0 replies; 15+ messages in thread
From: imeevma @ 2019-01-19 13:20 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
This patch defines parameters binding for SQL statements executed
through box.
Part of #3505
Closes #3401
---
src/box/execute.c | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/box/execute.h | 21 +++++++
src/box/lua/sql.c | 14 ++++-
3 files changed, 198 insertions(+), 3 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 7abf5a6..d04822b 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,
@@ -325,6 +326,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 2a9ad3b..a08b558 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] 15+ messages in thread* [tarantool-patches] [PATCH v8 6/6] sql: check new box.sql.execute()
2019-01-19 13:20 [tarantool-patches] [PATCH v8 0/6] sql: remove box.sql.execute imeevma
` (4 preceding siblings ...)
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 5/6] lua: parameter binding for new execute() imeevma
@ 2019-01-19 13:20 ` imeevma
2019-01-22 19:58 ` [tarantool-patches] Re: [PATCH v8 0/6] sql: remove box.sql.execute Vladislav Shpilevoy
6 siblings, 0 replies; 15+ messages in thread
From: imeevma @ 2019-01-19 13:20 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 d04822b..3544c3b 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -750,7 +750,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);
@@ -870,6 +871,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");
@@ -934,7 +937,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) {
@@ -951,7 +954,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;
@@ -960,18 +963,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 896599d..112198d 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 a08b558..8c53d66 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -133,11 +133,15 @@ 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);
port_destroy(&port);
return 1;
+
+sqlerror:
+ lua_pushstring(L, sqlite3_errmsg(sql_get()));
+ return lua_error(L);
}
static int
@@ -153,7 +157,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] 15+ messages in thread* [tarantool-patches] Re: [PATCH v8 0/6] sql: remove box.sql.execute
2019-01-19 13:20 [tarantool-patches] [PATCH v8 0/6] sql: remove box.sql.execute imeevma
` (5 preceding siblings ...)
2019-01-19 13:20 ` [tarantool-patches] [PATCH v8 6/6] sql: check new box.sql.execute() imeevma
@ 2019-01-22 19:58 ` Vladislav Shpilevoy
2019-01-24 7:31 ` Imeev Mergen
6 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2019-01-22 19:58 UTC (permalink / raw)
To: tarantool-patches, imeevma
Hi! Thanks for the fixes!
On 19/01/2019 16:20, imeevma@tarantool.org wrote:
> 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 eighth 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.
It was not necessary to resend the whole patchset just for such
minor comments. Strictly speaking, it was not necessary for some
other older versions.
>
> 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:
If we nonetheless consider this mail thread as a new patchset,
the text below does not describe the difference correctly. It is
an old description.
Besides, in each cover-letter of each version you should list
differences between each pair of sequential versions, as I know.
> - 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
> v7:
> https://www.freelists.org/post/tarantool-patches/PATCH-v7-06-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 | 507 +++++++++++++++++++++++++++++++++++++++++--------
> src/box/execute.h | 63 ++----
> src/box/iproto.cc | 18 +-
> src/box/lua/call.c | 9 +-
> src/box/lua/schema.lua | 23 +++
> src/box/lua/sql.c | 37 +++-
> 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 | 97 +++++-----
> src/lua/utils.h | 8 +-
> 13 files changed, 589 insertions(+), 206 deletions(-)
>
^ permalink raw reply [flat|nested] 15+ messages in thread* [tarantool-patches] Re: [PATCH v8 0/6] sql: remove box.sql.execute
2019-01-22 19:58 ` [tarantool-patches] Re: [PATCH v8 0/6] sql: remove box.sql.execute Vladislav Shpilevoy
@ 2019-01-24 7:31 ` Imeev Mergen
0 siblings, 0 replies; 15+ messages in thread
From: Imeev Mergen @ 2019-01-24 7:31 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
Hi! Thanks for comments. Also, I sent wrong branch to review,
sorry about that. Right branch is:
https://github.com/tarantool/tarantool/tree/imeevma/gh-3505-replace-box_sql_execute-by-box_execute
On 1/22/19 10:58 PM, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
>
> On 19/01/2019 16:20, imeevma@tarantool.org wrote:
>> 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 eighth 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.
>
> It was not necessary to resend the whole patchset just for such
> minor comments. Strictly speaking, it was not necessary for some
> other older versions.
Ok, understood.
>
>>
>> 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:
>
> If we nonetheless consider this mail thread as a new patchset,
> the text below does not describe the difference correctly. It is
> an old description.
>
> Besides, in each cover-letter of each version you should list
> differences between each pair of sequential versions, as I know.
>
Ok, understood.
>> - 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
>>
>> v7:
>> https://www.freelists.org/post/tarantool-patches/PATCH-v7-06-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 | 507
>> +++++++++++++++++++++++++++++++++++++++++--------
>> src/box/execute.h | 63 ++----
>> src/box/iproto.cc | 18 +-
>> src/box/lua/call.c | 9 +-
>> src/box/lua/schema.lua | 23 +++
>> src/box/lua/sql.c | 37 +++-
>> 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 | 97 +++++-----
>> src/lua/utils.h | 8 +-
>> 13 files changed, 589 insertions(+), 206 deletions(-)
>>
^ permalink raw reply [flat|nested] 15+ messages in thread