[PATCH v3 2/6] box: rework box_lua_{call, eval} to use input port
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Jun 18 15:12:56 MSK 2019
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 <dlfcn.h>
>
> /**
> @@ -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 <stdint.h>
> +
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 <port.h>
> +#include <stdbool.h>
>
> #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 <stdbool.h>
> #include <stdint.h>
>
> #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.
More information about the Tarantool-patches
mailing list