From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 10 Jun 2019 12:14:46 +0300 From: Vladimir Davydov Subject: Re: [PATCH v2 0/9] box: rework functions machinery Message-ID: <20190610091446.55r323oaag2u5l6g@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 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?