Re: [PATCH v3 2/6] box: rework box_lua_{call, eval} to use input port

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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.