From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 18 Jun 2019 15:12:56 +0300 From: Vladimir Davydov Subject: Re: [PATCH v3 2/6] box: rework box_lua_{call, eval} to use input port Message-ID: <20190618121256.hexjwmwh4tdjmas3@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Thu, Jun 13, 2019 at 05:08:22PM +0300, Kirill Shcherbatov wrote: > Re-factor box_lua_call and box_lua_eval so that they don't take > call_request. This approach is more scalable: in case of a > functional index, the user expects to see a tuple with field > names so we should be able to pass not only raw msgpack, but > also a tuple to a Lua call so we need an universal way to pass > arguments to _call methods. > > To pass a tuple msgpack introduced a new port_msgpack: the port > class with dump_lua method. > > Needed for #4182, #1260 > --- > src/box/call.c | 46 ++++++---------- > src/box/call.h | 4 -- > src/box/execute.c | 1 + > src/box/func.c | 27 ++++++++-- > src/box/func.h | 11 ++-- > src/box/lua/call.c | 121 +++++++++++++++++++++++++++++++----------- > src/box/lua/call.h | 8 ++- > src/box/lua/execute.c | 7 +-- > src/box/lua/execute.h | 4 +- > src/box/lua/misc.cc | 5 +- > src/box/port.c | 3 +- > src/box/port.h | 15 ++++++ > src/lib/core/port.h | 15 ++++-- > 13 files changed, 184 insertions(+), 83 deletions(-) The patch looks mostly fine to me. See a few comments below. > > diff --git a/src/box/call.c b/src/box/call.c > index 56da53fb3..1ab3993da 100644 > --- a/src/box/call.c > +++ b/src/box/call.c > @@ -105,33 +105,6 @@ access_check_func(const char *name, uint32_t name_len, struct func **funcp) > return 0; > } > > -static int > -box_c_call(struct func *func, struct call_request *request, struct port *port) > -{ > - assert(func != NULL && func->def->language == FUNC_LANGUAGE_C); > - > - /* Create a call context */ > - port_tuple_create(port); > - box_function_ctx_t ctx = { port }; > - > - /* Clear all previous errors */ > - diag_clear(&fiber()->diag); > - assert(!in_txn()); /* transaction is not started */ > - > - /* Call function from the shared library */ > - int rc = func_call(func, &ctx, request->args, request->args_end); > - func = NULL; /* May be deleted by DDL */ > - if (rc != 0) { > - if (diag_last_error(&fiber()->diag) == NULL) { > - /* Stored procedure forget to set diag */ > - diag_set(ClientError, ER_PROC_C, "unknown error"); > - } > - port_destroy(port); > - return -1; > - } > - return 0; > -} > - > int > box_module_reload(const char *name) > { > @@ -199,11 +172,15 @@ box_process_call(struct call_request *request, struct port *port) > } > > int rc; > + struct port in_port; Nit: I don't quite like the names in_port/out_port - what's the point in including the type name into a variable name? I'd call them simply 'args' and 'ret' (or 'retval' or 'out'). > + port_msgpack_create(&in_port, request->args, > + request->args_end - request->args); > if (func && func->def->language == FUNC_LANGUAGE_C) { > - rc = box_c_call(func, request, port); > + rc = func_call(func, &in_port, port); IMO box_c_call should be removed by the next patch. This patch should simply replace msgpack args and call_request with port. I don't want to make you re-split though. Up to you. > } else { > - rc = box_lua_call(request, port); > + rc = box_lua_call(name, name_len, &in_port, port); > } > + port_destroy(&in_port); > /* Restore the original user */ > if (orig_credentials) > fiber_set_user(fiber(), orig_credentials); > @@ -229,16 +206,23 @@ box_process_eval(struct call_request *request, struct port *port) > /* Check permissions */ > if (access_check_universe(PRIV_X) != 0) > return -1; > - if (box_lua_eval(request, port) != 0) { > + struct port in_port; > + port_msgpack_create(&in_port, request->args, > + request->args_end - request->args); > + const char *expr = request->expr; > + uint32_t expr_len = mp_decode_strl(&expr); > + if (box_lua_eval(expr, expr_len, &in_port, port) != 0) { > + port_destroy(&in_port); We know that port_destroy is a no-op for port_msgpack. I think it's okay to omit the call here. > txn_rollback(); > return -1; > } > > if (in_txn()) { > diag_set(ClientError, ER_FUNCTION_TX_ACTIVE); > + port_destroy(&in_port); > txn_rollback(); > return -1; > } > - > + port_destroy(&in_port); > return 0; > } > diff --git a/src/box/call.h b/src/box/call.h > index 1b54551be..45580bc9d 100644 > --- a/src/box/call.h > +++ b/src/box/call.h > @@ -38,10 +38,6 @@ extern "C" { > struct port; > struct call_request; > > -struct box_function_ctx { > - struct port *port; > -}; > - > /** > * Reload loadable module by name. > * > diff --git a/src/box/execute.c b/src/box/execute.c > index e81cc32aa..a91c939fc 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -108,6 +108,7 @@ const struct port_vtab port_sql_vtab = { > /* .dump_msgpack_16 = */ NULL, > /* .dump_lua = */ port_sql_dump_lua, > /* .dump_plain = */ NULL, > + /* .get_msgpack = */ NULL, > /* .destroy = */ port_sql_destroy, > }; > > diff --git a/src/box/func.c b/src/box/func.c > index d1b8879ad..740fad3d7 100644 > --- a/src/box/func.c > +++ b/src/box/func.c > @@ -34,6 +34,9 @@ > #include "lua/utils.h" > #include "error.h" > #include "diag.h" > +#include "fiber.h" > +#include "port.h" > +#include "txn.h" > #include > > /** > @@ -433,21 +436,39 @@ func_load(struct func *func) > } > > int > -func_call(struct func *func, box_function_ctx_t *ctx, const char *args, > - const char *args_end) > +func_call(struct func *func, struct port *in_port, struct port *out_port) > { > + assert(func != NULL && func->def->language == FUNC_LANGUAGE_C); > + /* Transaction is not started. */ > + assert(!in_txn()); I don't think it's a good idea to retain this requirement for any C function call. First, it introduces a dependency on txn. Second, we might want to call a C function from a transaction one day. Let's leave this check in box_process_call. > if (func->func == NULL) { > if (func_load(func) != 0) > return -1; > } > > + uint32_t args_sz; > + const char *args = port_get_msgpack(in_port, &args_sz); > + if (args == NULL) > + return -1; > + > + port_tuple_create(out_port); > + box_function_ctx_t ctx = { out_port }; > + > /* Module can be changed after function reload. */ > struct module *module = func->module; > assert(module != NULL); > ++module->calls; > - int rc = func->func(ctx, args, args_end); > + int rc = func->func(&ctx, args, args + args_sz); > --module->calls; > module_gc(module); > + if (rc != 0) { > + if (diag_last_error(&fiber()->diag) == NULL) { > + /* Stored procedure forget to set diag */ > + diag_set(ClientError, ER_PROC_C, "unknown error"); > + } > + port_destroy(out_port); > + return -1; > + } > return rc; > } > > diff --git a/src/box/func.h b/src/box/func.h > index fa4d738ab..d38a2b283 100644 > --- a/src/box/func.h > +++ b/src/box/func.h > @@ -42,6 +42,12 @@ > extern "C" { > #endif /* defined(__cplusplus) */ > > +struct port; > + > +struct box_function_ctx { > + struct port *port; > +}; > + I'd rather move it to src/box/func_def.h, because we have box_function_ctx_t and box_function_f defined there. > /** > * Dynamic shared module. > */ > @@ -104,11 +110,10 @@ void > func_delete(struct func *func); > > /** > - * Call stored C function using @a args. > + * Call function with arguments represented with given in_port. > */ > int > -func_call(struct func *func, box_function_ctx_t *ctx, const char *args, > - const char *args_end); > +func_call(struct func *func, struct port *in_port, struct port *out_port); > > /** > * Reload dynamically loadable module. > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > index 04020ef6f..e32845095 100644 > --- a/src/box/lua/call.c > +++ b/src/box/lua/call.c I guess we don't need to include box/xrow.h into this file anymore. Please remove. > @@ -266,6 +266,55 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg, > return root.size; > } > > +static const struct port_vtab port_msgpack_vtab; > + > +void > +port_msgpack_create(struct port *base, const char *data, uint32_t data_sz) > +{ > + struct port_msgpack *port_msgpack = (struct port_msgpack *) base; > + memset(port_msgpack, 0, sizeof(*port_msgpack)); > + port_msgpack->vtab = &port_msgpack_vtab; > + port_msgpack->data = data; > + port_msgpack->data_sz = data_sz; > +} > + > +static void > +port_msgpack_dump_lua(struct port *base, struct lua_State *L, bool is_flat) > +{ > + assert(is_flat == true); > + struct port_msgpack *port = (struct port_msgpack *) base; > + assert(port->vtab == &port_msgpack_vtab); > + > + const char *args = port->data; > + uint32_t arg_count = mp_decode_array(&args); > + for (uint32_t i = 0; i < arg_count; i++) > + luamp_decode(L, luaL_msgpack_default, &args); > +} > + > +static const char * > +port_msgpack_get_msgpack(struct port *base, uint32_t *size) > +{ > + struct port_msgpack *port = (struct port_msgpack *) base; > + assert(port->vtab == &port_msgpack_vtab); > + *size = port->data_sz; > + return port->data; > +} > + > +static void > +port_msgpack_destroy(struct port *base) > +{ > + (void) base; > +} > + > +static const struct port_vtab port_msgpack_vtab = { > + .dump_msgpack = NULL, > + .dump_msgpack_16 = NULL, > + .dump_lua = port_msgpack_dump_lua, > + .dump_plain = NULL, > + .get_msgpack = port_msgpack_get_msgpack, > + .destroy = port_msgpack_destroy, > +}; > + port_msgpack is used solely from src/box/call.c so I think we should define vtab there. The 'destroy' method could be set to NULL - as I wrote above we should probably omit calling it altogether. We can't define port_msgpack_dump_lua in src/box/call.c, because it depends on Lua API. Okay, let's define it in src/box/lua/misc.cc then, just like we did in case of port_tuple_dump_lua. > static const struct port_vtab port_lua_vtab; > > void > @@ -282,28 +331,31 @@ port_lua_create(struct port *port, struct lua_State *L) > port_lua->ref = -1; > } > > +struct lua_process_ctx { I'd rather call it execute_lua_ctx. > + const char *name; > + uint32_t name_len; > +}; > + > static int > execute_lua_call(lua_State *L) > { > - struct call_request *request = (struct call_request *) > - lua_topointer(L, 1); > + struct port *in_port = (struct port *) lua_topointer(L, 1); > + struct lua_process_ctx *ctx = > + (struct lua_process_ctx *) lua_topointer(L, 2); Why not include the port containing call/eval arguments into the context as well? > @@ -429,37 +480,47 @@ static const struct port_vtab port_lua_vtab = { > .dump_msgpack_16 = port_lua_dump_16, > .dump_lua = NULL, > .dump_plain = port_lua_dump_plain, > + .get_msgpack = NULL, > .destroy = port_lua_destroy, > }; > > static inline int > -box_process_lua(struct call_request *request, struct port *base, > - lua_CFunction handler) > +box_process_lua(lua_CFunction handler, void *arg, struct port *in_port, s/void *arg/struct execute_lua_ctx *ctx ? > diff --git a/src/box/lua/call.h b/src/box/lua/call.h > index 0542123da..1801d5090 100644 > --- a/src/box/lua/call.h > +++ b/src/box/lua/call.h > @@ -35,6 +35,8 @@ > extern "C" { > #endif /* defined(__cplusplus) */ > > +#include > + Nit: header inclusions should go before extern "C". > struct lua_State; > > void > diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c > index 239aba47b..187bdb73d 100644 > --- a/src/box/lua/execute.c > +++ b/src/box/lua/execute.c > @@ -39,8 +39,9 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L, > } > > void > -port_sql_dump_lua(struct port *port, struct lua_State *L) > +port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat) > { > + assert(is_flat == false); > assert(port->vtab == &port_sql_vtab); > struct sql *db = sql_get(); > struct sql_stmt *stmt = ((struct port_sql *)port)->stmt; > @@ -49,7 +50,7 @@ port_sql_dump_lua(struct port *port, struct lua_State *L) > lua_createtable(L, 0, 2); > lua_sql_get_metadata(stmt, L, column_count); > lua_setfield(L, -2, "metadata"); > - port_tuple_vtab.dump_lua(port, L); > + port_tuple_vtab.dump_lua(port, L, is_flat); Nit: better pass 'false' explicitly IMO. > diff --git a/src/box/port.h b/src/box/port.h > index f18803660..4c0c3cf7a 100644 > --- a/src/box/port.h > +++ b/src/box/port.h > @@ -32,6 +32,7 @@ > */ > #include "trivia/util.h" > #include > +#include > > #if defined(__cplusplus) > extern "C" { > @@ -80,6 +81,20 @@ port_tuple_create(struct port *port); > int > port_tuple_add(struct port *port, struct tuple *tuple); > > +/** Port implementation used for storing raw data. */ > +struct port_msgpack { > + const struct port_vtab *vtab; > + const char *data; > + uint32_t data_sz; > +}; > + > +static_assert(sizeof(struct port_msgpack) <= sizeof(struct port), > + "sizeof(struct port_msgpack) must be <= sizeof(struct port)"); > + > +/** Initialize a port to dump row data. */ s/row/raw > +void > +port_msgpack_create(struct port *port, const char *data, uint32_t data_sz); > + > /** Port for storing the result of a Lua CALL/EVAL. */ > struct port_lua { > const struct port_vtab *vtab; > diff --git a/src/lib/core/port.h b/src/lib/core/port.h > index 8ace40fc5..2f1bc5ed1 100644 > --- a/src/lib/core/port.h > +++ b/src/lib/core/port.h > @@ -30,6 +30,7 @@ > * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > * SUCH DAMAGE. > */ > +#include > #include > > #if defined(__cplusplus) > @@ -63,7 +64,7 @@ struct port_vtab { > */ > int (*dump_msgpack_16)(struct port *port, struct obuf *out); > /** Dump the content of a port to Lua stack. */ > - void (*dump_lua)(struct port *port, struct lua_State *L); > + void (*dump_lua)(struct port *port, struct lua_State *L, bool is_flat); Please document the new is_flat option.