Re: [PATCH v2 0/9] box: rework functions machinery

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

On Thu, Jun 06, 2019 at 03:03:56PM +0300, Kirill Shcherbatov wrote:
>  src/box/CMakeLists.txt            |   1 +
>  src/box/alter.cc                  | 118 +++++--
>  src/box/bootstrap.snap            | Bin 4393 -> 4449 bytes
>  src/box/call.c                    | 157 +--------
>  src/box/call.h                    |  14 -
>  src/box/errcode.h                 |   1 +
>  src/box/execute.c                 |   1 +
>  src/box/func.c                    | 564 ++++++++++++++++++++++++++++--
>  src/box/func.h                    |  84 +++--
>  src/box/func_def.c                |   9 +
>  src/box/func_def.h                |  43 ++-
>  src/box/lua/call.c                | 379 ++------------------
>  src/box/lua/init.c                |   2 +
>  src/box/lua/port_lua.c            | 318 +++++++++++++++++
>  src/box/lua/schema.lua            |  38 +-
>  src/box/lua/upgrade.lua           |  25 +-
>  src/box/port.h                    |   2 +-
>  src/box/schema.cc                 |  47 ++-
>  src/box/schema.h                  |  31 +-
>  src/box/schema_def.h              |   4 +
>  src/lib/core/port.h               |  16 +
>  src/lua/utils.c                   | 126 +++++++
>  src/lua/utils.h                   |  19 +

I don't quite like the way you split the code among modules. First, we
try to keep all Lua-related stuff in src/lua or src/box/lua. There are
exceptions (e.g. luaT_module_find located in src/box/func.c), but we try
to avoid them.

I think that the code should be split as follows:

 - src/box/func.[hc] should contain definition of the base func class,
   C func implementation and module manipulation.

 - src/box/lua/call.[ch] should contain implementation of Lua func
   class, both for persistent functions and not, and helpers to run Lua
   code (eval/call). We might want to rename src/box/lua/call.[hc] to
   func.[hc], but I'm not sure it's really necessary at this point.
   Also, I don't think it's worth moving sandboxing to util.[hc] - it's
   only used to run Lua code so I guess it's okay to leave it in
   lua/call along with port_lua and luaT_func_find.

 - src/box/call.[hc] should contain helpers to invoke a call request
   (CALL/EVAL). CALL code should check permissions, look up function by
   name and, if it's found execute, execute it, otherwise call a helper
   function defined in lua/func to run the function by name.

A general node: please avoid patching and moving the code at the same
time, except cases when everything you change is names of the moved
functions: this makes the code nearly impossible to review for
correctness.

Here's how I'd try to organize the patch set:

 1. Simplify func updates on alter (in a little bit different way than
    you did - see my comments to the corresponding patch).
 2. Re-factor box_lua_call and box_lua_eval so that they don't take
    call_request. I think they should take struct port. Rationale: 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.
 3. Introduce base func class to src/box/func.[hc]. Implement func_c in
    src/box/func.c and func_lua in src/box/lua/call.c (func_lua_create
    should be declared in src/box/lua/call.h so that vtab and other
    internals aren't exposed).
 4. Add infrastructure to call functions from Lua. The code should live
    in src/box/lua/call.c. Please write a separate docbot request for
    this one.
 5. Add persistent functions. Splitting it in three patches doesn't
    really facilitate review IMO: I want to see the complete picture,
    including new _func format and sandboxing. BTW I guess is_sandbox
    shouldn't be a part of options at this time - it should be a
    separate column, like is_deterministic.

I'll post a few more comments re the code in reply to emails with
patches.

Thoughts, objections?