[Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module

[Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module

[Timur Safin]

This patchset is a continuation of patch series which Alexander Turenko has sent 
earlier on. It extends module api with the necessary wrappers we should have 
in api to make external merger possible. Some internal functionality which
was acessible to internal merger, should be wrapped via accessors if merger
is becoming external.

- most of introduced functions are simple public wrappers around of internal 
  functions, e.g. `box_tuple_validate` is a wrapper around 
  `tuple_validate_raw`, or `bax_key_def_dup` is a wrapper for `key_def_dup`.
  They were necessary to not reveal implementation specific details 
  (i.e. `tuple_data` needed in `tuple_validate_raw` should know `box_tuple_t`
  layout);
- wherever we used to use `struct tuple *` we replacing them with public alias
  `box_tuple_t`. Same is for `struct tuple_format` -> `box_tuple_format_t` and 
  other typedefs.

- eventually merger in Tarantool core might get deprecated and all new 
  development for merger will continue in version agnostic external module
  thus we decided that common utility functions (e.g. `luaT_temp_luastate`)
  which used to reside in merger to better be sitting in `src/lua/utils.c`
  instead (in addition to becoming public as described above).

NB! You could observe, that this part of changes for merger in module_api is
much, much easier than those for key_def as sent by Sasha Turenko before.

Issue:
* https://github.com/tarantool/tarantool/issues/5273 
  ('module api: expose everything that is needed for external key_def module')

Branches:
* https://github.com/tarantool/tarantool/tree/tsafin/gh-5273-expand-module-api
  (top 7 commits above of 14 @Totktonada's commits)
* https://github.com/tarantool/tarantool/tree/tsafin/gh-5273-expand-module-api-1.10
  (last 9 commits above of 16 @Totktonada's commits)

== External merger module

Currently external merger is residing here 
https://github.com/tsafin/tarantool-merge
and it uses key_def from https://github.com/Totktonada/key_def repository
via simple luarocks dependency (we had to introduce debugging rock-server to
make this working).

CI is installed and is running on GitHub Actions, thus we know that it's 
sort of working together, but more extensive testing of merger and key_def 
yet to be proceeded. Though it might be done independently of current 
kernel patches.


Timur Safin (7):
  module api: export box_tuple_validate
  module api: export box_key_def_dup
  module api: luaT_newthread
  module api: luaL_register_module & luaL_register_type
  module api: luaT_temp_luastate & luaT_release_temp_luastate
  module api: luaL_checkibuf & luaL_checkconstchar
  module api: luaL_cdata_iscallable

 src/box/key_def_api.c |  6 +++
 src/box/key_def_api.h | 10 +++++
 src/box/lua/merger.c  | 78 -------------------------------------
 src/box/tuple.c       |  6 +++
 src/box/tuple.h       | 11 ++++++
 src/exports.h         | 10 +++++
 src/lua/utils.c       | 51 ++++++++++++++++++++++++-
 src/lua/utils.h       | 89 ++++++++++++++++++++++++++++++++++++-------
 8 files changed, 169 insertions(+), 92 deletions(-)

-- 
2.20.1

[Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Timur Safin]

For external merger we need means to valudate tuple data,
thus exporting `box_tuple_validate` which is wrapper around
`tuple_validate_raw` without revealing access to tuple
internals.

Part of #5273
---
 src/box/tuple.c |  6 ++++++
 src/box/tuple.h | 11 +++++++++++
 src/exports.h   |  1 +
 3 files changed, 18 insertions(+)

diff --git a/src/box/tuple.c b/src/box/tuple.c
index f3965476e..ddf41567c 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -748,6 +748,12 @@ box_tuple_new(box_tuple_format_t *format, const char *data, const char *end)
 	return tuple_bless(ret);
 }
 
+int
+box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple)
+{
+	return tuple_validate_raw(format, tuple_data(tuple));
+}
+
 /* }}} box_tuple_* */
 
 int
diff --git a/src/box/tuple.h b/src/box/tuple.h
index 53ae690cc..ed0501464 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -283,6 +283,17 @@ box_tuple_update(box_tuple_t *tuple, const char *expr, const char *expr_end);
 box_tuple_t *
 box_tuple_upsert(box_tuple_t *tuple, const char *expr, const char *expr_end);
 
+/**
+ * Check tuple data correspondence to the space format.
+ * @param format Format to which the tuple must match.
+ * @param tuple  Tuple to validate.
+ *
+ * @retval  0 The tuple is valid.
+ * @retval -1 The tuple is invalid.
+ */
+int
+box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple);
+
 /** \endcond public */
 
 /**
diff --git a/src/exports.h b/src/exports.h
index 48894ea72..592b388bb 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -77,6 +77,7 @@ EXPORT(box_tuple_to_buf)
 EXPORT(box_tuple_unref)
 EXPORT(box_tuple_update)
 EXPORT(box_tuple_upsert)
+EXPORT(box_tuple_validate)
 EXPORT(box_tuple_validate_key_parts)
 EXPORT(box_txn)
 EXPORT(box_txn_alloc)
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup

[Timur Safin]

Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`

Part of #5273
---
 src/box/key_def_api.c |  6 ++++++
 src/box/key_def_api.h | 10 ++++++++++
 src/exports.h         |  1 +
 3 files changed, 17 insertions(+)

diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 2555b9fdd..98602ff24 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -199,6 +199,12 @@ box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count)
 	return key_def;
 }
 
+box_key_def_t *
+box_key_def_dup(const box_key_def_t *key_def)
+{
+	return key_def_dup(key_def);
+}
+
 void
 box_key_def_delete(box_key_def_t *key_def)
 {
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index 8dd6eb10b..a419b712c 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -173,6 +173,16 @@ box_key_part_def_create(box_key_part_def_t *part);
 API_EXPORT box_key_def_t *
 box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count);
 
+/**
+ * Duplicate key_def.
+ * \param key_def Original key_def.
+ *
+ * \retval not NULL Duplicate of src.
+ * \retval     NULL Memory error.
+ */
+box_key_def_t *
+box_key_def_dup(const box_key_def_t *key_def);
+
 /**
  * Delete key definition
  *
diff --git a/src/exports.h b/src/exports.h
index 592b388bb..202f5bf19 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -31,6 +31,7 @@ EXPORT(box_iterator_free)
 EXPORT(box_iterator_next)
 EXPORT(box_key_def_delete)
 EXPORT(box_key_def_dump_parts)
+EXPORT(box_key_def_dup)
 EXPORT(box_key_def_merge)
 EXPORT(box_key_def_new)
 EXPORT(box_key_def_new_ex)
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Timur Safin]

Exporting `luaT_newthread` as it's necessary for external merger

Part of #5273
---
 src/exports.h   |  1 +
 src/lua/utils.h | 25 +++++++++++++------------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/src/exports.h b/src/exports.h
index 202f5bf19..20070b240 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -412,6 +412,7 @@ EXPORT(luaT_checktuple)
 EXPORT(luaT_cpcall)
 EXPORT(luaT_error)
 EXPORT(luaT_istuple)
+EXPORT(luaT_newthread)
 EXPORT(luaT_pushtuple)
 EXPORT(luaT_state)
 EXPORT(luaT_tolstring)
diff --git a/src/lua/utils.h b/src/lua/utils.h
index e9dd27d08..7639cd64a 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -539,6 +539,19 @@ luaT_tolstring(lua_State *L, int idx, size_t *ssize);
 LUA_API int
 luaL_iscallable(lua_State *L, int idx);
 
+
+/**
+ * @brief Creates a new Lua coroutine in a protected frame. If
+ * <lua_newthread> call underneath succeeds, the created Lua state
+ * is on the top of the guest stack and a pointer to this state is
+ * returned. Otherwise LUA_ERRMEM error is handled and the result
+ * is NULL.
+ * @param L is a Lua state
+ * @sa <lua_newthread>
+ */
+struct lua_State *
+luaT_newthread(struct lua_State *L);
+
 /** \endcond public */
 
 /**
@@ -616,18 +629,6 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
 		luaL_error(L, "number must not be NaN or Inf");
 }
 
-/**
- * @brief Creates a new Lua coroutine in a protected frame. If
- * <lua_newthread> call underneath succeeds, the created Lua state
- * is on the top of the guest stack and a pointer to this state is
- * returned. Otherwise LUA_ERRMEM error is handled and the result
- * is NULL.
- * @param L is a Lua state
- * @sa <lua_newthread>
- */
-struct lua_State *
-luaT_newthread(struct lua_State *L);
-
 /**
  * Check if a value on @a L stack by index @a idx is an ibuf
  * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Timur Safin]

Exporting `luaL_register_module` & `luaL_register_type`
functions as they needed for external merger

Part of #5273
---
 src/exports.h   | 2 ++
 src/lua/utils.h | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/exports.h b/src/exports.h
index 20070b240..1c16e88b2 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -386,6 +386,8 @@ EXPORT(luaL_pushresult)
 EXPORT(luaL_pushuint64)
 EXPORT(luaL_ref)
 EXPORT(luaL_register)
+EXPORT(luaL_register_module)
+EXPORT(luaL_register_type)
 EXPORT(luaL_setcdatagc)
 EXPORT(luaL_setfuncs)
 EXPORT(luaL_setmetatable)
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 7639cd64a..9b1fe7e57 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -78,6 +78,8 @@ luaL_pushuuid(struct lua_State *L);
 
 /** \cond public */
 
+struct luaL_Reg;
+
 /**
  * @brief Checks whether a value on the Lua stack is a cdata.
  *
@@ -442,6 +444,8 @@ luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
 	luaL_convertfield(L, cfg, idx, field);
 }
 
+/** \cond public */
+
 void
 luaL_register_type(struct lua_State *L, const char *type_name,
 		   const struct luaL_Reg *methods);
@@ -451,8 +455,6 @@ void
 luaL_register_module(struct lua_State *L, const char *modname,
 		     const struct luaL_Reg *methods);
 
-/** \cond public */
-
 /**
  * Push uint64_t onto the stack
  *
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Timur Safin]

Moved `luaT_temp_luastate` & `luaT_release_temp_luastate` from
merger to ustil.c and made them public.

They were necessary for external merger support.

Part of #5273
---
 src/box/lua/merger.c | 78 --------------------------------------------
 src/exports.h        |  2 ++
 src/lua/utils.c      | 49 ++++++++++++++++++++++++++++
 src/lua/utils.h      | 35 ++++++++++++++++++++
 4 files changed, 86 insertions(+), 78 deletions(-)

diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
index 583946c4c..f26594049 100644
--- a/src/box/lua/merger.c
+++ b/src/box/lua/merger.c
@@ -149,84 +149,6 @@ luaT_gettuple(struct lua_State *L, int idx, struct tuple_format *format)
 	return tuple;
 }
 
-/**
- * Get a temporary Lua state.
- *
- * Use case: a function does not accept a Lua state as an argument
- * to allow using from C code, but uses a Lua value, which is
- * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
- * to get and process the value.
- *
- * The resulting Lua state has a separate Lua stack, but the same
- * globals and registry as `tarantool_L` (and all Lua states in
- * tarantool at the moment of writing this).
- *
- * This Lua state should be used only from one fiber: otherwise
- * one fiber may change the stack and another one will access a
- * wrong stack slot when it will be scheduled for execution after
- * yield.
- *
- * Return a Lua state on success and set @a coro_ref and @a top.
- * These values should be passed to
- * `luaT_release_temp_luastate()`, when the state is not needed
- * anymore.
- *
- * Return NULL and set a diag at failure.
- */
-static struct lua_State *
-luaT_temp_luastate(int *coro_ref, int *top)
-{
-	if (fiber()->storage.lua.stack != NULL) {
-		/*
-		 * Reuse existing stack. In the releasing function
-		 * we should drop a stack top to its initial
-		 * value to don't exhaust available slots by
-		 * many requests in row.
-		 */
-		struct lua_State *L = fiber()->storage.lua.stack;
-		*coro_ref = LUA_NOREF;
-		*top = lua_gettop(L);
-		return L;
-	}
-
-	/* Popped by luaL_ref(). */
-	struct lua_State *L = luaT_newthread(tarantool_L);
-	if (L == NULL)
-		return NULL;
-	/*
-	 * We should remove the reference to the newly created Lua
-	 * thread from tarantool_L, because of two reasons:
-	 *
-	 * First, if we'll push something to tarantool_L and
-	 * yield, then another fiber will not know that a stack
-	 * top is changed and may operate on a wrong slot.
-	 *
-	 * Second, many requests that push a value to tarantool_L
-	 * and yield may exhaust available slots on the stack. It
-	 * is limited by LUAI_MAXSTACK build time constant (~65K).
-	 *
-	 * We cannot just pop the value, but should keep the
-	 * reference in the registry while it is in use.
-	 * Otherwise it may be garbage collected.
-	 */
-	*coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
-	*top = -1;
-	return L;
-}
-
-/**
- * Release a temporary Lua state.
- *
- * It complements `luaT_temp_luastate()`.
- */
-static void
-luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top)
-{
-	if (top >= 0)
-		lua_settop(L, top);
-	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
-}
-
 /* }}} */
 
 /* {{{ Create, destroy structures from Lua */
diff --git a/src/exports.h b/src/exports.h
index 1c16e88b2..ef4f3b741 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -416,7 +416,9 @@ EXPORT(luaT_error)
 EXPORT(luaT_istuple)
 EXPORT(luaT_newthread)
 EXPORT(luaT_pushtuple)
+EXPORT(luaT_release_temp_luastate)
 EXPORT(luaT_state)
+EXPORT(luaT_temp_luastate)
 EXPORT(luaT_tolstring)
 EXPORT(luaT_tuple_encode)
 EXPORT(luaT_tuple_new)
diff --git a/src/lua/utils.c b/src/lua/utils.c
index bf5452d3e..40078b5df 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1259,6 +1259,55 @@ luaT_newthread(struct lua_State *L)
 	return L1;
 }
 
+struct lua_State *
+luaT_temp_luastate(int *coro_ref, int *top)
+{
+	if (fiber()->storage.lua.stack != NULL) {
+		/*
+		 * Reuse existing stack. In the releasing function
+		 * we should drop a stack top to its initial
+		 * value to don't exhaust available slots by
+		 * many requests in row.
+		 */
+		struct lua_State *L = fiber()->storage.lua.stack;
+		*coro_ref = LUA_NOREF;
+		*top = lua_gettop(L);
+		return L;
+	}
+
+	/* Popped by luaL_ref(). */
+	struct lua_State *L = luaT_newthread(tarantool_L);
+	if (L == NULL)
+		return NULL;
+	/*
+	 * We should remove the reference to the newly created Lua
+	 * thread from tarantool_L, because of two reasons:
+	 *
+	 * First, if we'll push something to tarantool_L and
+	 * yield, then another fiber will not know that a stack
+	 * top is changed and may operate on a wrong slot.
+	 *
+	 * Second, many requests that push a value to tarantool_L
+	 * and yield may exhaust available slots on the stack. It
+	 * is limited by LUAI_MAXSTACK build time constant (~65K).
+	 *
+	 * We cannot just pop the value, but should keep the
+	 * reference in the registry while it is in use.
+	 * Otherwise it may be garbage collected.
+	 */
+	*coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	*top = -1;
+	return L;
+}
+
+void
+luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top)
+{
+	if (top >= 0)
+		lua_settop(L, top);
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
+}
+
 int
 tarantool_lua_utils_init(struct lua_State *L)
 {
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 9b1fe7e57..da0140076 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -554,6 +554,41 @@ luaL_iscallable(lua_State *L, int idx);
 struct lua_State *
 luaT_newthread(struct lua_State *L);
 
+/**
+ * Get a temporary Lua state.
+ *
+ * Use case: a function does not accept a Lua state as an argument
+ * to allow using from C code, but uses a Lua value, which is
+ * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
+ * to get and process the value.
+ *
+ * The resulting Lua state has a separate Lua stack, but the same
+ * globals and registry as `tarantool_L` (and all Lua states in
+ * tarantool at the moment of writing this).
+ *
+ * This Lua state should be used only from one fiber: otherwise
+ * one fiber may change the stack and another one will access a
+ * wrong stack slot when it will be scheduled for execution after
+ * yield.
+ *
+ * Return a Lua state on success and set @a coro_ref and @a top.
+ * These values should be passed to
+ * `luaT_release_temp_luastate()`, when the state is not needed
+ * anymore.
+ *
+ * Return NULL and set a diag at failure.
+ */
+struct lua_State *
+luaT_temp_luastate(int *coro_ref, int *top);
+
+/**
+ * Release a temporary Lua state.
+ *
+ * It complements `luaT_temp_luastate()`.
+ */
+void
+luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top);
+
 /** \endcond public */
 
 /**
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar

[Timur Safin]

Moved `luaL_checkibuf` & `luaL_checkconstchar` to the public
part of module api.

Part of #5273
---
 src/exports.h   |  2 ++
 src/lua/utils.h | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/src/exports.h b/src/exports.h
index ef4f3b741..08c7ac53f 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -348,6 +348,8 @@ EXPORT(luaL_callmeta)
 EXPORT(luaL_cdef)
 EXPORT(luaL_checkany)
 EXPORT(luaL_checkcdata)
+EXPORT(luaL_checkconstchar)
+EXPORT(luaL_checkibuf)
 EXPORT(luaL_checkint64)
 EXPORT(luaL_checkinteger)
 EXPORT(luaL_checklstring)
diff --git a/src/lua/utils.h b/src/lua/utils.h
index da0140076..6b10d2755 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -589,6 +589,23 @@ luaT_temp_luastate(int *coro_ref, int *top);
 void
 luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top);
 
+/**
+ * Check if a value on @a L stack by index @a idx is an ibuf
+ * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
+ * Returns NULL, if can't convert - not an ibuf object.
+ */
+struct ibuf *
+luaL_checkibuf(struct lua_State *L, int idx);
+
+/**
+ * Check if a value on @a L stack by index @a idx is pointer at
+ * char or const char. '(char *)NULL' is also considered a valid
+ * char pointer.
+ */
+int
+luaL_checkconstchar(struct lua_State *L, int idx, const char **res,
+		    uint32_t *cdata_type_p);
+
 /** \endcond public */
 
 /**
@@ -666,6 +683,8 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
 		luaL_error(L, "number must not be NaN or Inf");
 }
 
+/** \cond public */
+
 /**
  * Check if a value on @a L stack by index @a idx is an ibuf
  * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
-- 
2.20.1

[Tarantool-patches] [PATCH 2.X 7/7] module api: luaL_cdata_iscallable

[Timur Safin]

In addition to `luaL_iscallable` we need `luaL_cdata_iscallable`
because code which was calling it directly had to be copied to
merger side

Part of #5273
---
 src/exports.h   | 1 +
 src/lua/utils.c | 2 +-
 src/lua/utils.h | 6 ++++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/exports.h b/src/exports.h
index 08c7ac53f..943ad69fa 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -345,6 +345,7 @@ EXPORT(luaL_addvalue)
 EXPORT(luaL_argerror)
 EXPORT(luaL_buffinit)
 EXPORT(luaL_callmeta)
+EXPORT(luaL_cdata_iscallable)
 EXPORT(luaL_cdef)
 EXPORT(luaL_checkany)
 EXPORT(luaL_checkcdata)
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 40078b5df..bf9f46cb7 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1063,7 +1063,7 @@ luaT_tolstring(lua_State *L, int idx, size_t *len)
 }
 
 /* Based on ffi_meta___call() from luajit/src/lib_ffi.c. */
-static int
+int
 luaL_cdata_iscallable(lua_State *L, int idx)
 {
 	/* Calculate absolute value in the stack. */
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 6b10d2755..69ff4de86 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -541,6 +541,12 @@ luaT_tolstring(lua_State *L, int idx, size_t *ssize);
 LUA_API int
 luaL_iscallable(lua_State *L, int idx);
 
+/**
+ * Check whether a Lua object is a cdata metatype with a __call field.
+ *
+ */
+LUA_API int
+luaL_cdata_iscallable(lua_State *L, int idx);
 
 /**
  * @brief Creates a new Lua coroutine in a protected frame. If
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 2 comments below.

On 24.09.2020 19:00, Timur Safin wrote:
> For external merger we need means to valudate tuple data,

1. valudate -> validate.

> thus exporting `box_tuple_validate` which is wrapper around
> `tuple_validate_raw` without revealing access to tuple
> internals.
> 
> diff --git a/src/box/tuple.h b/src/box/tuple.h
> index 53ae690cc..ed0501464 100644
> --- a/src/box/tuple.h
> +++ b/src/box/tuple.h
> @@ -283,6 +283,17 @@ box_tuple_update(box_tuple_t *tuple, const char *expr, const char *expr_end);
>  box_tuple_t *
>  box_tuple_upsert(box_tuple_t *tuple, const char *expr, const char *expr_end);
>  
> +/**
> + * Check tuple data correspondence to the space format.
> + * @param format Format to which the tuple must match.
> + * @param tuple  Tuple to validate.
> + *
> + * @retval  0 The tuple is valid.
> + * @retval -1 The tuple is invalid.

I see you are a doxygen-master, nice.

> + */
> +int
> +box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple);

2. OCD mode on. I would propose either make tuple the first
argument, or rename it to box_tuple_format_validate_tuple().
So as to be consistent with our agreement, that if something
is a method of <type>, then the <type> argument goes first,
and the method name is <type>_<action>.

I see we currently have in the public API the functions:

	box_tuple_validate - your new function, a bit
		inconsistent.

	box_tuple_validate_key_parts - this should have been
		box_key_def_validate_tuple from the beginning,
		but we can't do anything about it now.

	box_key_def_validate_key - correct. Key_def goes first,
		and the name is consistent.

So if you will make box_tuple_validate consistent, we will have
more correct signatures (2/3) than incorrect, for validation
methods at least.

Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

On 24.09.2020 19:00, Timur Safin wrote:
> Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`

1. Do you really need this method? It looks like it can be done by

	old_parts = box_key_def_dump_parts(old_key_def);
	new_key_def = box_key_def_new_ex(old_parts);

So the method seems redundant.

> diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> index 8dd6eb10b..a419b712c 100644
> --- a/src/box/key_def_api.h
> +++ b/src/box/key_def_api.h
> @@ -173,6 +173,16 @@ box_key_part_def_create(box_key_part_def_t *part);
>  API_EXPORT box_key_def_t *
>  box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count);
>  
> +/**
> + * Duplicate key_def.
> + * \param key_def Original key_def.
> + *
> + * \retval not NULL Duplicate of src.
> + * \retval     NULL Memory error.

2. OCD mode intensifies.

:(
Lets use @ everywhere in the new code instead of \.

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

On 24.09.2020 19:00, Timur Safin wrote:
> Exporting `luaT_newthread` as it's necessary for external merger
> 
> Part of #5273
> ---
>  src/exports.h   |  1 +
>  src/lua/utils.h | 25 +++++++++++++------------
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/src/exports.h b/src/exports.h
> index 202f5bf19..20070b240 100644
> --- a/src/exports.h
> +++ b/src/exports.h
> @@ -412,6 +412,7 @@ EXPORT(luaT_checktuple)
>  EXPORT(luaT_cpcall)
>  EXPORT(luaT_error)
>  EXPORT(luaT_istuple)
> +EXPORT(luaT_newthread)
>  EXPORT(luaT_pushtuple)
>  EXPORT(luaT_state)
>  EXPORT(luaT_tolstring)
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index e9dd27d08..7639cd64a 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -539,6 +539,19 @@ luaT_tolstring(lua_State *L, int idx, size_t *ssize);
>  LUA_API int
>  luaL_iscallable(lua_State *L, int idx);
>  
> +

1. Extra empty line.

> +/**
> + * @brief Creates a new Lua coroutine in a protected frame. If
> + * <lua_newthread> call underneath succeeds, the created Lua state
> + * is on the top of the guest stack and a pointer to this state is
> + * returned. Otherwise LUA_ERRMEM error is handled and the result
> + * is NULL.
> + * @param L is a Lua state

2.
	@param L Lua state.

Or probably could be even dropped. Such description is useless anyway.

> + * @sa <lua_newthread>

3. It is worth mentioning that the error is set into the diagnostics
area.

> + */
> +struct lua_State *
> +luaT_newthread(struct lua_State *L);
> +

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

On 24.09.2020 19:00, Timur Safin wrote:
> Exporting `luaL_register_module` & `luaL_register_type`
> functions as they needed for external merger
> 
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index 7639cd64a..9b1fe7e57 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -78,6 +78,8 @@ luaL_pushuuid(struct lua_State *L);
>  
>  /** \cond public */
>  
> +struct luaL_Reg;

1. It does not seem to be public. How are users supposed to work with that?
Igor, could you please take a look at this?

If you need it so badly, at least we need a wrapper around luaL_Reg
or we need to expose it and document too. Probably Igor has better ideas.

> +
>  /**
>   * @brief Checks whether a value on the Lua stack is a cdata.
>   *
> @@ -442,6 +444,8 @@ luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
>  	luaL_convertfield(L, cfg, idx, field);
>  }
>  
> +/** \cond public */
> +
>  void
>  luaL_register_type(struct lua_State *L, const char *type_name,
>  		   const struct luaL_Reg *methods);

2. These newly exported methods don't have a single comment. Why?

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Vladislav Shpilevoy]

Thanks for the patch!

I strongly don't like this export. It seems to be too internal.

But I have no better ideas than propose to implement your own
lua_State cache in the merger module. It does not seem to be too
hard, and I don't think it would occupy much memory.

Just a simple list of lua_State objects, created by luaT_newthread
on demand, and put back into the list when unused. What is wrong
with that?

Adding Igor as Lua master to help with this.

See 2 comments below.

> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index 9b1fe7e57..da0140076 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -554,6 +554,41 @@ luaL_iscallable(lua_State *L, int idx);
>  struct lua_State *
>  luaT_newthread(struct lua_State *L);
>  
> +/**
> + * Get a temporary Lua state.
> + *
> + * Use case: a function does not accept a Lua state as an argument
> + * to allow using from C code, but uses a Lua value, which is
> + * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
> + * to get and process the value.
> + *
> + * The resulting Lua state has a separate Lua stack, but the same
> + * globals and registry as `tarantool_L` (and all Lua states in

1. Users don't know what is tarantool_L.

> + * tarantool at the moment of writing this).
> + *
> + * This Lua state should be used only from one fiber: otherwise
> + * one fiber may change the stack and another one will access a
> + * wrong stack slot when it will be scheduled for execution after
> + * yield.
> + *
> + * Return a Lua state on success and set @a coro_ref and @a top.
> + * These values should be passed to
> + * `luaT_release_temp_luastate()`, when the state is not needed
> + * anymore.

2. I would encapsulate the out parameters into some kind of
luaT_temp_state_ctx or something like that. Probably not even
opaque, but extendible.

Also maybe worth grouping these methods by using a common prefix:

	luaT_temp_luastate_get
	luaT_temp_luastate_release

> + *
> + * Return NULL and set a diag at failure.
> + */
> +struct lua_State *
> +luaT_temp_luastate(int *coro_ref, int *top);
> +
> +/**
> + * Release a temporary Lua state.
> + *
> + * It complements `luaT_temp_luastate()`.
> + */
> +void
> +luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top);
> +
>  /** \endcond public */
>  
>  /**
> 

Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index da0140076..6b10d2755 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -589,6 +589,23 @@ luaT_temp_luastate(int *coro_ref, int *top);
>  void
>  luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top);
>  
> +/**
> + * Check if a value on @a L stack by index @a idx is an ibuf
> + * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
> + * Returns NULL, if can't convert - not an ibuf object.
> + */
> +struct ibuf *
> +luaL_checkibuf(struct lua_State *L, int idx);

1. IMO 'check' is almost always worse than 'is'. Because you leave
a user no choice but to use lua_cpcall if he does not want an
exception. With 'is' he would be able to decide whether he wants to
throw. The same for the method below.

Also what exactly is the reason you need the ibuf checking for?
Ibuf is not exposed as a part of tarantool binary. It is a part of
small library. When we want to export parts of small from inside
of the executable, we need to obfuscate the types and wrap the
functions, because user's small library may be different from the
executable's small.

If you use ibuf internally in the merger, what is wrong with
registering it in the merger, and storing a global variable of
uint32_t type, like we do inside tarantool executable? You will even
get the same type index, but won't need to carry small library
types into the public lua API.

> +
> +/**
> + * Check if a value on @a L stack by index @a idx is pointer at
> + * char or const char. '(char *)NULL' is also considered a valid
> + * char pointer.
> + */
> +int
> +luaL_checkconstchar(struct lua_State *L, int idx, const char **res,
> +		    uint32_t *cdata_type_p);

2. What is 'res'? What is 'cdata_type_p'?

> +
>  /** \endcond public */
>  
>  /**

Re: [Tarantool-patches] [PATCH 2.X 7/7] module api: luaL_cdata_iscallable

[Vladislav Shpilevoy]

Thanks for the patch!

See 4 comments below.

On 24.09.2020 19:00, Timur Safin wrote:
> In addition to `luaL_iscallable` we need `luaL_cdata_iscallable`
> because code which was calling it directly had to be copied to
> merger side

1. Please, put '.' in the end of sentences. Here and in other
commits.

2. luaL_iscallable() already checks luaL_cdata_iscallable(). Why
do you need it separated from luaL_iscallable()?

> Part of #5273
> ---
>  src/exports.h   | 1 +
>  src/lua/utils.c | 2 +-
>  src/lua/utils.h | 6 ++++++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index 6b10d2755..69ff4de86 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -541,6 +541,12 @@ luaT_tolstring(lua_State *L, int idx, size_t *ssize);
>  LUA_API int
>  luaL_iscallable(lua_State *L, int idx);
>  
> +/**
> + * Check whether a Lua object is a cdata metatype with a __call field.

3. The function will crash in debug build, if it is not cdata. And will
do UB in release build. This is because it is not supposed to be called
out of luaL_iscallable(). And I don't see why would you need it.

> + *

4. Extra empty line.

> + */
> +LUA_API int
> +luaL_cdata_iscallable(lua_State *L, int idx);

Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup

[Alexander Turenko]

On Tue, Sep 29, 2020 at 12:21:02AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 2 comments below.
> 
> On 24.09.2020 19:00, Timur Safin wrote:
> > Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`
> 
> 1. Do you really need this method? It looks like it can be done by
> 
> 	old_parts = box_key_def_dump_parts(old_key_def);
> 	new_key_def = box_key_def_new_ex(old_parts);
> 
> So the method seems redundant.

It is not strictly necessary, however using of box_key_def_dup() would
be less error-prone (no extra allocations) and the resulting code would
be more readable. My vote is for this method if you have no strict
objections.

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Alexander Turenko]

> > diff --git a/src/lua/utils.h b/src/lua/utils.h
> > index 7639cd64a..9b1fe7e57 100644
> > --- a/src/lua/utils.h
> > +++ b/src/lua/utils.h
> > @@ -78,6 +78,8 @@ luaL_pushuuid(struct lua_State *L);
> >  
> >  /** \cond public */
> >  
> > +struct luaL_Reg;
> 
> 1. It does not seem to be public. How are users supposed to work with that?
> Igor, could you please take a look at this?
> 
> If you need it so badly, at least we need a wrapper around luaL_Reg
> or we need to expose it and document too. Probably Igor has better ideas.

It is exposed from lauxlib.h, no need to duplicate in the module API.

> 
> > +
> >  /**
> >   * @brief Checks whether a value on the Lua stack is a cdata.
> >   *
> > @@ -442,6 +444,8 @@ luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
> >  	luaL_convertfield(L, cfg, idx, field);
> >  }
> >  
> > +/** \cond public */
> > +
> >  void
> >  luaL_register_type(struct lua_State *L, const char *type_name,
> >  		   const struct luaL_Reg *methods);
> 
> 2. These newly exported methods don't have a single comment. Why?

luaL_register_*() are our helpers around luaL_register(), AFAIR. A
module should use the latter.

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Alexander Turenko]

On Tue, Sep 29, 2020 at 12:21:18AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> I strongly don't like this export. It seems to be too internal.

I have no point to start discussion. Can you clarify your feeling?

> 
> But I have no better ideas than propose to implement your own
> lua_State cache in the merger module. It does not seem to be too
> hard, and I don't think it would occupy much memory.
> 
> Just a simple list of lua_State objects, created by luaT_newthread
> on demand, and put back into the list when unused. What is wrong
> with that?

When we able to simplify modules, it worth to do so (when there are no
certain objections). Write once, use many.

> 
> Adding Igor as Lua master to help with this.
> 
> See 2 comments below.
> 
> > diff --git a/src/lua/utils.h b/src/lua/utils.h
> > index 9b1fe7e57..da0140076 100644
> > --- a/src/lua/utils.h
> > +++ b/src/lua/utils.h
> > @@ -554,6 +554,41 @@ luaL_iscallable(lua_State *L, int idx);
> >  struct lua_State *
> >  luaT_newthread(struct lua_State *L);
> >  
> > +/**
> > + * Get a temporary Lua state.
> > + *
> > + * Use case: a function does not accept a Lua state as an argument
> > + * to allow using from C code, but uses a Lua value, which is
> > + * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
> > + * to get and process the value.
> > + *
> > + * The resulting Lua state has a separate Lua stack, but the same
> > + * globals and registry as `tarantool_L` (and all Lua states in
> 
> 1. Users don't know what is tarantool_L.

Note: It is <luaT_state>() in the module API.

Re: [Tarantool-patches] [PATCH 2.X 7/7] module api: luaL_cdata_iscallable

[Alexander Turenko]

> 2. luaL_iscallable() already checks luaL_cdata_iscallable(). Why
> do you need it separated from luaL_iscallable()?

+1 here. The same question from my side.

Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Alexander Turenko]

> Part of #5273

Sorry for nitpicking, but the issue states 'expose everything that is
needed for external key_def module'. The commit technically is not part
of the issue.

Either change the issue, or link commits with
tarantool/vshard-cluster-api#5, or file a separate issue (I would do the
latter).

It is just to don't mix different things into one: #5273 have certain
definition of done. It will be hard to understand what going on in the
future if we'll mix everything together.

(It is the same for all commits.)

Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar

[Alexander Turenko]

> > +/**
> > + * Check if a value on @a L stack by index @a idx is an ibuf
> > + * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
> > + * Returns NULL, if can't convert - not an ibuf object.
> > + */
> > +struct ibuf *
> > +luaL_checkibuf(struct lua_State *L, int idx);
> 
> 1. IMO 'check' is almost always worse than 'is'. Because you leave
> a user no choice but to use lua_cpcall if he does not want an
> exception. With 'is' he would be able to decide whether he wants to
> throw. The same for the method below.

I personally prefer *is* Lua functions. It is good to have *is* variants
in the public API aside of (or even instead of) *check* ones. I don't
insist, however.

> Also what exactly is the reason you need the ibuf checking for?
> Ibuf is not exposed as a part of tarantool binary. It is a part of
> small library. When we want to export parts of small from inside
> of the executable, we need to obfuscate the types and wrap the
> functions, because user's small library may be different from the
> executable's small.

It seems, we really should expose non-opacue <box_ibuf_t>: rpos and wpos
are used in merger (it would be too expensive to wrap them into calls).
The module accepts ibuf created from Lua (tarantool's <struct ibuf>), so
linking with external small is not an option (unless we'll really care
about ABI).

Maybe we can alias <box_ibuf_t> with <struct ibuf> if we'll add static
asserts around the structure size and offset of fields? And expose
box_ibuf_*(), of course.

> If you use ibuf internally in the merger, what is wrong with
> registering it in the merger, and storing a global variable of
> uint32_t type, like we do inside tarantool executable? You will even
> get the same type index, but won't need to carry small library
> types into the public lua API.

*check*, *is* helpers are usable and I would not be against exposing
them: just to don't write the same code in different modules. However,
if there are certain objections (not just 'you can implement it in your
module'), it can be skipped because of simplicity.

I think we should consider exposing things, which are useful for module
writters (and C stored procedure writters): tarantool is the tool and I
hope we all want it to be nice tool.

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Alexander Turenko]

On Thu, Sep 24, 2020 at 08:00:16PM +0300, Timur Safin wrote:
> Exporting `luaT_newthread` as it's necessary for external merger

But you expose accessors for the temporary Lua state in the later patch.
Built-in merger only uses luaT_newthread() in luaT_temp_luastate().

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Igor Munkin]

Vlad,

I glanced the patch and doubt these functions need to be exported by
Tarantool. These functions neither wrap internals nor provide any
performance benefits (e.g. reduce numbers of excess allocations). It
would be nice to have such auxiliary interfaces in so called "modulekit"
to make third party modules development easier, but I see no problem to
borrow this code to merger/key_def as is for now.

On 29.09.20, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 2 comments below.
> 
> On 24.09.2020 19:00, Timur Safin wrote:
> > Exporting `luaL_register_module` & `luaL_register_type`
> > functions as they needed for external merger
> > 
> > diff --git a/src/lua/utils.h b/src/lua/utils.h
> > index 7639cd64a..9b1fe7e57 100644
> > --- a/src/lua/utils.h
> > +++ b/src/lua/utils.h
> > @@ -78,6 +78,8 @@ luaL_pushuuid(struct lua_State *L);
> >  
> >  /** \cond public */
> >  
> > +struct luaL_Reg;
> 
> 1. It does not seem to be public. How are users supposed to work with that?
> Igor, could you please take a look at this?

It's public[1] and is provided by <lauxlib.h> Lua standard header as
Sasha already mentioned it in the thread.

> 

<snipped>

[1]: https://www.lua.org/manual/5.1/manual.html#luaL_Reg

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Igor Munkin]

Vlad,

I guess there should be a question to be asked in scope of the original
series: what performance enhancement can we obtain with such hacky
optimization? Do we have any benchmarks? If the performance impact is
negligible I believe these interfaces should be left internal.

On 29.09.20, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> I strongly don't like this export. It seems to be too internal.
> 
> But I have no better ideas than propose to implement your own
> lua_State cache in the merger module. It does not seem to be too
> hard, and I don't think it would occupy much memory.

Well, such exercise looks like fun but it's better to make it once and
provide user-friendly interfaces by so called "modulekit" to be used in
third party modules. Anyway, using this routine seems a bit complicated
than <lua_newstate> and <luaT_newstate> (also fully implemented via Lua
C standart API + already exported <luaT_call>). Ergo one ought to push
extern developers using the new one besides exporting it.

> 
> Just a simple list of lua_State objects, created by luaT_newthread
> on demand, and put back into the list when unused. What is wrong
> with that?

Again, how much will performance be nerfed if we simply create a new Lua
coroutine by demand?

> 
> Adding Igor as Lua master to help with this.
> 
> See 2 comments below.
> 
> > diff --git a/src/lua/utils.h b/src/lua/utils.h
> > index 9b1fe7e57..da0140076 100644
> > --- a/src/lua/utils.h
> > +++ b/src/lua/utils.h
> > @@ -554,6 +554,41 @@ luaL_iscallable(lua_State *L, int idx);
> >  struct lua_State *
> >  luaT_newthread(struct lua_State *L);
> >  

<snipped>

> 
> 2. I would encapsulate the out parameters into some kind of
> luaT_temp_state_ctx or something like that. Probably not even
> opaque, but extendible.

Totally agree here. Current interface is too fragile and tricky and IMHO
has to be redesigned/reworked prior to be exported for public usage.

> 
> Also maybe worth grouping these methods by using a common prefix:
> 
> 	luaT_temp_luastate_get
> 	luaT_temp_luastate_release
> 
> > + *
> > + * Return NULL and set a diag at failure.
> > + */
> > +struct lua_State *
> > +luaT_temp_luastate(int *coro_ref, int *top);
> > +
> > +/**
> > + * Release a temporary Lua state.
> > + *
> > + * It complements `luaT_temp_luastate()`.
> > + */
> > +void
> > +luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top);
> > +
> >  /** \endcond public */
> >  
> >  /**
> > 

Finally, I see no strict arguments *for* exporting these routines, and
have many doubts regarding its current design and *obligatoriness*. Yes,
they may be useful inside the platform but do they look like ones user
must have? I tend to "no" for now.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Igor Munkin]

Timur,

Thanks for the patch, but I guess this one can be implemented using the
existing public interfaces (i.e. Lua C standart API + already exported
<luaT_call>). It was done to resolve the issue with Tarantool internal
crash[1] and I believe if user need to handle OOM error it can do it by
himself.

[1]: https://github.com/tarantool/tarantool/issues/4556

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Alexander Turenko]

On Tue, Sep 29, 2020 at 06:10:02PM +0300, Igor Munkin wrote:
> Vlad,
> 
> I guess there should be a question to be asked in scope of the original
> series: what performance enhancement can we obtain with such hacky
> optimization? Do we have any benchmarks? If the performance impact is
> negligible I believe these interfaces should be left internal.

Why hacky? I'm a module developer and I ask Tarantool: Hey, I know you
have a cache of Lua states. Give me one? Can I yield while the state is
in use? I can, nice. Can I share it between fibers? No, okay.

That's all. Quite simple.

> 
> On 29.09.20, Vladislav Shpilevoy wrote:
> > Thanks for the patch!
> > 
> > I strongly don't like this export. It seems to be too internal.
> > 
> > But I have no better ideas than propose to implement your own
> > lua_State cache in the merger module. It does not seem to be too
> > hard, and I don't think it would occupy much memory.
> 
> Well, such exercise looks like fun but it's better to make it once and
> provide user-friendly interfaces by so called "modulekit" to be used in
> third party modules. Anyway, using this routine seems a bit complicated
> than <lua_newstate> and <luaT_newstate> (also fully implemented via Lua
> C standart API + already exported <luaT_call>). Ergo one ought to push
> extern developers using the new one besides exporting it.
> 
> > 
> > Just a simple list of lua_State objects, created by luaT_newthread
> > on demand, and put back into the list when unused. What is wrong
> > with that?
> 
> Again, how much will performance be nerfed if we simply create a new Lua
> coroutine by demand?

acpi-cpufreq, userspace governor, fixed frequency. Hyperthreading. No
taskset.

A: 2.6.0-118-g90108875a RelWithDebInfo. 
B: Same, but with the patch:

 | diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
 | index 583946c4c..a4332e3d6 100644
 | --- a/src/box/lua/merger.c
 | +++ b/src/box/lua/merger.c
 | @@ -530,13 +530,14 @@ luaL_merge_source_buffer_fetch_impl(struct merge_source_buffer *source,
 |  static int
 |  luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
 |  {
 | -       int coro_ref = LUA_NOREF;
 | -       int top = -1;
 | -       struct lua_State *L = luaT_temp_luastate(&coro_ref, &top);
 | -       if (L == NULL)
 | +       int top = lua_gettop(tarantool_L);
 | +       struct lua_State *L = luaT_newthread(tarantool_L);
 | +       if (L == NULL) {
 | +               lua_settop(tarantool_L, top);
 |                 return -1;
 | +       }
 |         int rc = luaL_merge_source_buffer_fetch_impl(source, L);
 | -       luaT_release_temp_luastate(L, coro_ref, top);
 | +       lua_settop(tarantool_L, top);
 |         return rc;
 |  }

(The patch is stupid, I know, we should not leave an extra element on
tarantool_L and yield, but call luaL_ref / luaL_unref with Lua registry
instead. I forget about this before measurements, but it should not be
much important here.)

Workload (I run it from the console):

 | tarantool> clock = require('clock') merger = require('merger') buffer = require('buffer') do local start = clock.monotonic() for i = 1, 10^8 do merger.new_source_frombuffer(buffer.ibuf()):select() end print(clock.monotonic() - start) end

The workload is VERY synthetic: it just don't do anything aside
creating a new buffer source and observing it as the empty one.

A (five runs within one console session):

212.67329062428
213.37809054693
213.94517080393
213.76512717083
215.20638198918

B (measured in the same way):

218.82243523281
219.3976499592
220.46797300316
221.37441124767
220.58953443216

The average performance decrease seems to be around 3% for this case.

`perf record -a -g` results:

 | A                                                   | B                                            |
 | --------------------------------------------------- | -------------------------------------------- |
 | - 75.31%     0.00%  tarantool                       | - 76.20%     0.00%  tarantool                |
 |   - 0x41fe53b8                                      |   - 0x410a03b8                               |
 |     - 54.56% lj_BC_FUNCC                            |     - 34.88% lj_BC_FUNCC                     |
 |       - 36.90% lbox_merge_source_new                |       - 10.98% lbox_merge_source_new         |
 |         - 20.76% luaL_pushcdata                     |         + 3.31% luaL_setcdatagc              |
 |           + 20.00% lj_gc_step                       |         + 3.17% luaL_merge_source_buffer_new |
 |         + 5.34% lua_pushcclosure                    |         + 2.04% luaL_iscallable              |
 |         + 4.77% luaL_setcdatagc                     |           0.77% luaL_pushcdata               |
 |         + 3.69% luaL_merge_source_buffer_new        |           0.76% lua_pushcclosure             |
 |         + 1.61% luaL_iscallable                     |       + 8.39% lbox_merge_source_select       |
 |       + 7.65% lbox_merge_source_select              |       - 7.60% lua_gettable_wrapper           |
 |       + 4.58% lbox_merge_source_gc                  |         - 6.00% lua_pushthread               |
 |       + 2.08% lj_cf_ffi_meta___call                 |           + 4.64% lj_state_new               |
 |         0.73% lua_gettop                            |           + 1.16% lua_close                  |
 |         0.61% lj_cf_ffi_clib___index                |         - 1.50% lua_newthread                |
 |     - 11.93% 0x5645f3a0fa76                         |           + 1.20% lj_gc_step                 |
 |       + 11.93% lj_gc_step_jit                       |       + 2.99% lbox_merge_source_gc           |
 |     + 5.84% lj_vm_exit_handler                      |       + 1.83% lj_cf_ffi_meta___call          |
 |     + 1.05% lj_vmeta_call                           |         0.51% lua_xmove                      |
 |       0.53% lj_vmeta_tgetv                          |     - 32.65% 0x55569f7bfa76                  |
 |                                                     |       + 32.61% lj_gc_step_jit                |
 |                                                     |     + 5.43% lj_vm_exit_handler               |
 |                                                     |       0.69% lj_vmeta_call                    |
 |                                                     |       0.57% lj_vmeta_tgetv                   |

Duration of samples collecting was a bit different, I don't know how much it matters.

To be honest, I don't feel myself strong enough to interpret this. But
if we'll sum lj_gc_step and lj_gc_step_jit, then it'll be 31.93% for A
and 33.81% for B. And there is lua_gettable_wrapper (7.60%) in B.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup

[Vladislav Shpilevoy]

On 29.09.2020 07:03, Alexander Turenko wrote:
> On Tue, Sep 29, 2020 at 12:21:02AM +0200, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> See 2 comments below.
>>
>> On 24.09.2020 19:00, Timur Safin wrote:
>>> Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`
>>
>> 1. Do you really need this method? It looks like it can be done by
>>
>> 	old_parts = box_key_def_dump_parts(old_key_def);
>> 	new_key_def = box_key_def_new_ex(old_parts);
>>
>> So the method seems redundant.
> 
> It is not strictly necessary, however using of box_key_def_dup() would
> be less error-prone (no extra allocations) and the resulting code would
> be more readable. My vote is for this method if you have no strict
> objections.

Regarding this one I am not strictly against, but I don't like overloading
module API with too many methods, especially with the trivial ones, which
can be easily implemented via the others in a few lines.

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Vladislav Shpilevoy]

On 29.09.2020 07:09, Alexander Turenko wrote:
>>> diff --git a/src/lua/utils.h b/src/lua/utils.h
>>> index 7639cd64a..9b1fe7e57 100644
>>> --- a/src/lua/utils.h
>>> +++ b/src/lua/utils.h
>>> @@ -78,6 +78,8 @@ luaL_pushuuid(struct lua_State *L);
>>>  
>>>  /** \cond public */
>>>  
>>> +struct luaL_Reg;
>>
>> 1. It does not seem to be public. How are users supposed to work with that?
>> Igor, could you please take a look at this?
>>
>> If you need it so badly, at least we need a wrapper around luaL_Reg
>> or we need to expose it and document too. Probably Igor has better ideas.
> 
> It is exposed from lauxlib.h, no need to duplicate in the module API.

Thanks, didn't know it is public. Ok, then no question here.

>>> +
>>>  /**
>>>   * @brief Checks whether a value on the Lua stack is a cdata.
>>>   *
>>> @@ -442,6 +444,8 @@ luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
>>>  	luaL_convertfield(L, cfg, idx, field);
>>>  }
>>>  
>>> +/** \cond public */
>>> +
>>>  void
>>>  luaL_register_type(struct lua_State *L, const char *type_name,
>>>  		   const struct luaL_Reg *methods);
>>
>> 2. These newly exported methods don't have a single comment. Why?
> 
> luaL_register_*() are our helpers around luaL_register(), AFAIR. A
> module should use the latter.

In that case I don't see why do we need to export them.

luaL_register_module() is a wrapper around luaL_register() to
support multilevel modules like 'box.space', 'net.box', etc.
With '.' in them, AFAIS. And a few error checks.

It is not bad to have a bit excessive but commonly used or perf
critical things in module.h, but there is a certain border of
what is "commonly used" and "perf critical". Module registration
does not seem to me fitting any of these categories (subjectively).

So if it can be done without any new exports, I would stick to
that.

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Vladislav Shpilevoy]

On 29.09.2020 07:17, Alexander Turenko wrote:
> On Tue, Sep 29, 2020 at 12:21:18AM +0200, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> I strongly don't like this export. It seems to be too internal.
> 
> I have no point to start discussion. Can you clarify your feeling?

I don't like that we give access to fiber's Lua state. If a user will
leave anything on it, it can lead to anything as well, UB I guess. But
I didn't check.

>> But I have no better ideas than propose to implement your own
>> lua_State cache in the merger module. It does not seem to be too
>> hard, and I don't think it would occupy much memory.
>>
>> Just a simple list of lua_State objects, created by luaT_newthread
>> on demand, and put back into the list when unused. What is wrong
>> with that?
> 
> When we able to simplify modules, it worth to do so (when there are no
> certain objections). Write once, use many.

What are applications for that except the merger? Potentially.

>> Adding Igor as Lua master to help with this.
>>
>> See 2 comments below.
>>
>>> diff --git a/src/lua/utils.h b/src/lua/utils.h
>>> index 9b1fe7e57..da0140076 100644
>>> --- a/src/lua/utils.h
>>> +++ b/src/lua/utils.h
>>> @@ -554,6 +554,41 @@ luaL_iscallable(lua_State *L, int idx);
>>>  struct lua_State *
>>>  luaT_newthread(struct lua_State *L);
>>>  
>>> +/**
>>> + * Get a temporary Lua state.
>>> + *
>>> + * Use case: a function does not accept a Lua state as an argument
>>> + * to allow using from C code, but uses a Lua value, which is
>>> + * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
>>> + * to get and process the value.
>>> + *
>>> + * The resulting Lua state has a separate Lua stack, but the same
>>> + * globals and registry as `tarantool_L` (and all Lua states in
>>
>> 1. Users don't know what is tarantool_L.
> 
> Note: It is <luaT_state>() in the module API.

Nice, didn't know that either. But still the comment is invalid.

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Vladislav Shpilevoy]

>>> diff --git a/src/lua/utils.h b/src/lua/utils.h
>>> index 7639cd64a..9b1fe7e57 100644
>>> --- a/src/lua/utils.h
>>> +++ b/src/lua/utils.h
>>> @@ -78,6 +78,8 @@ luaL_pushuuid(struct lua_State *L);
>>>  
>>>  /** \cond public */
>>>  
>>> +struct luaL_Reg;
>>
>> 1. It does not seem to be public. How are users supposed to work with that?
>> Igor, could you please take a look at this?
> 
> It's public[1] and is provided by <lauxlib.h> Lua standard header as
> Sasha already mentioned it in the thread.

Thanks, didn't know that.

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Vladislav Shpilevoy]

On 29.09.2020 23:03, Alexander Turenko wrote:
> On Tue, Sep 29, 2020 at 06:10:02PM +0300, Igor Munkin wrote:
>> Vlad,
>>
>> I guess there should be a question to be asked in scope of the original
>> series: what performance enhancement can we obtain with such hacky
>> optimization? Do we have any benchmarks? If the performance impact is
>> negligible I believe these interfaces should be left internal.
> 
> Why hacky? I'm a module developer and I ask Tarantool: Hey, I know you
> have a cache of Lua states. Give me one? Can I yield while the state is
> in use? I can, nice. Can I share it between fibers? No, okay.

It is not really a cache of lua states. Maybe of a single lua state, and
working only in Lua fibers (in C fibers you will create/delete the state
constantly), but not of states.

A proper Lua state cache/pool can be implemented easily without fiber at
all. Just have a pool with these states, where you either take a free one,
or allocate a new when the pool is empty. And when you don't need a state,
put it back into the pool. Then you will have at most the same number of
states in this pool, as the number of fibers, using this pool. Does not
seem to be a complex thing, will fit in 50-70 lines on C I think, top.
Neither seem to be too heavy (I didn't check), since all these states will
have empty stack, cleared before putting them back to the pool.

> That's all. Quite simple.

It is not that simple. Whatever you export now, we will need to support
potentially forever. So exporting not vital things increases support costs
for us for a very long time by saving a few time to you and Timur so you
don't need to change merger code and can move it as is. I would better
one time made merger less depending on Tarantool internals, then do long
support of the new pile of internal methods exposed in a hurry.

Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar

[Vladislav Shpilevoy]

On 29.09.2020 07:53, Alexander Turenko wrote:
>>> +/**
>>> + * Check if a value on @a L stack by index @a idx is an ibuf
>>> + * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
>>> + * Returns NULL, if can't convert - not an ibuf object.
>>> + */
>>> +struct ibuf *
>>> +luaL_checkibuf(struct lua_State *L, int idx);
>>
>> 1. IMO 'check' is almost always worse than 'is'. Because you leave
>> a user no choice but to use lua_cpcall if he does not want an
>> exception. With 'is' he would be able to decide whether he wants to
>> throw. The same for the method below.
> 
> I personally prefer *is* Lua functions. It is good to have *is* variants
> in the public API aside of (or even instead of) *check* ones. I don't
> insist, however.

This is what I said. *is* is better than *check*. More freedom to decide
what to do, when it returns false.

>> Also what exactly is the reason you need the ibuf checking for?
>> Ibuf is not exposed as a part of tarantool binary. It is a part of
>> small library. When we want to export parts of small from inside
>> of the executable, we need to obfuscate the types and wrap the
>> functions, because user's small library may be different from the
>> executable's small.
> 
> It seems, we really should expose non-opacue <box_ibuf_t>: rpos and wpos
> are used in merger (it would be too expensive to wrap them into calls).
> The module accepts ibuf created from Lua (tarantool's <struct ibuf>), so
> linking with external small is not an option (unless we'll really care
> about ABI).

It seems you care about ABI a lot. So why do you stop now? If we don't care
about ABI that much, then why do we worry about box_key_part_def_t?

I am ok with box_ibuf_t, of any kind. Then we could eventually legally expose
tarantool_lua_ibuf, as at least for single alloc+free pairs it is better than
region (a tiny tiny better).

> Maybe we can alias <box_ibuf_t> with <struct ibuf> if we'll add static
> asserts around the structure size and offset of fields? And expose
> box_ibuf_*(), of course.

Also looks fine. However returning to the topic, it seems not related to the
Lua type ids. You don't need ibuf definition to get its type ID in C. All
is done in luaL_ctypeid function taking a string anyway.

	luaL_ctypeid(L, "struct ibuf *");
	luaL_ctypeid(L, "struct ibuf");

So why does Timur need that method exported? luaL_checkibuf is a trival
function, which can be implemented by luaL_iscdata() + getting ibuf type
id using luaL_ctypeid() and comparing result with the cdata type id.

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Alexander Turenko]

> >>> +
> >>>  /**
> >>>   * @brief Checks whether a value on the Lua stack is a cdata.
> >>>   *
> >>> @@ -442,6 +444,8 @@ luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
> >>>  	luaL_convertfield(L, cfg, idx, field);
> >>>  }
> >>>  
> >>> +/** \cond public */
> >>> +
> >>>  void
> >>>  luaL_register_type(struct lua_State *L, const char *type_name,
> >>>  		   const struct luaL_Reg *methods);
> >>
> >> 2. These newly exported methods don't have a single comment. Why?
> > 
> > luaL_register_*() are our helpers around luaL_register(), AFAIR. A
> > module should use the latter.
> 
> In that case I don't see why do we need to export them.
> 
> luaL_register_module() is a wrapper around luaL_register() to
> support multilevel modules like 'box.space', 'net.box', etc.
> With '.' in them, AFAIS. And a few error checks.
> 
> It is not bad to have a bit excessive but commonly used or perf
> critical things in module.h, but there is a certain border of
> what is "commonly used" and "perf critical". Module registration
> does not seem to me fitting any of these categories (subjectively).
> 
> So if it can be done without any new exports, I would stick to
> that.

I meant, an external module should use luaL_register() directly. Sorry
for the confusion.

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Alexander Turenko]

Resend, CCed Timur and Igor back.

> >>> +
> >>>  /**
> >>>   * @brief Checks whether a value on the Lua stack is a cdata.
> >>>   *
> >>> @@ -442,6 +444,8 @@ luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
> >>>  	luaL_convertfield(L, cfg, idx, field);
> >>>  }
> >>>  
> >>> +/** \cond public */
> >>> +
> >>>  void
> >>>  luaL_register_type(struct lua_State *L, const char *type_name,
> >>>  		   const struct luaL_Reg *methods);
> >>
> >> 2. These newly exported methods don't have a single comment. Why?
> > 
> > luaL_register_*() are our helpers around luaL_register(), AFAIR. A
> > module should use the latter.
> 
> In that case I don't see why do we need to export them.
> 
> luaL_register_module() is a wrapper around luaL_register() to
> support multilevel modules like 'box.space', 'net.box', etc.
> With '.' in them, AFAIS. And a few error checks.
> 
> It is not bad to have a bit excessive but commonly used or perf
> critical things in module.h, but there is a certain border of
> what is "commonly used" and "perf critical". Module registration
> does not seem to me fitting any of these categories (subjectively).
> 
> So if it can be done without any new exports, I would stick to
> that.

I meant, an external module should use luaL_register() directly. Sorry
for the confusion.

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Alexander Turenko]

On Wed, Sep 30, 2020 at 01:23:17AM +0200, Vladislav Shpilevoy wrote:
> On 29.09.2020 23:03, Alexander Turenko wrote:
> > On Tue, Sep 29, 2020 at 06:10:02PM +0300, Igor Munkin wrote:
> >> Vlad,
> >>
> >> I guess there should be a question to be asked in scope of the original
> >> series: what performance enhancement can we obtain with such hacky
> >> optimization? Do we have any benchmarks? If the performance impact is
> >> negligible I believe these interfaces should be left internal.
> > 
> > Why hacky? I'm a module developer and I ask Tarantool: Hey, I know you
> > have a cache of Lua states. Give me one? Can I yield while the state is
> > in use? I can, nice. Can I share it between fibers? No, okay.
> 
> It is not really a cache of lua states. Maybe of a single lua state, and
> working only in Lua fibers (in C fibers you will create/delete the state
> constantly), but not of states.

I was about to say that background fibers also can reuse a Lua state
after ec9b544a123f3e0614ab94b83a7ac4b395326f34 ('lua: expose temporary
Lua state for iproto calls'), but it is not so in 1.10. And you're about
C fibers, so, yep, I agree: it is not as simple as the cache pattern.

The near goal of exposing those functions would be offer an
easy way to save some allocations in the external merger module (and
maybe others in future). Since the module is mainly needed for 1.10 and
the functions will not save allocations on 1.10 -- so the practial
reason is negligible.

The performance difference also looks small (I shared benchmarking results
in this mailing thread). Since there are questions about design of those
functions, I would use lua_newthread() (or wrapper around it to handle
OOM -- written in the module). We can return back to this API later.

> 
> A proper Lua state cache/pool can be implemented easily without fiber at
> all. Just have a pool with these states, where you either take a free one,
> or allocate a new when the pool is empty. And when you don't need a state,
> put it back into the pool. Then you will have at most the same number of
> states in this pool, as the number of fibers, using this pool. Does not
> seem to be a complex thing, will fit in 50-70 lines on C I think, top.
> Neither seem to be too heavy (I didn't check), since all these states will
> have empty stack, cleared before putting them back to the pool.
> 
> > That's all. Quite simple.
> 
> It is not that simple. Whatever you export now, we will need to support
> potentially forever. So exporting not vital things increases support costs
> for us for a very long time by saving a few time to you and Timur so you
> don't need to change merger code and can move it as is. I would better
> one time made merger less depending on Tarantool internals, then do long
> support of the new pile of internal methods exposed in a hurry.

I agree, but also I consider this activity as good opportunity to
improve the module API. Of course, when we have good design.

Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar

[Alexander Turenko]

On Wed, Sep 30, 2020 at 01:25:15AM +0200, Vladislav Shpilevoy wrote:
> On 29.09.2020 07:53, Alexander Turenko wrote:
> >>> +/**
> >>> + * Check if a value on @a L stack by index @a idx is an ibuf
> >>> + * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
> >>> + * Returns NULL, if can't convert - not an ibuf object.
> >>> + */
> >>> +struct ibuf *
> >>> +luaL_checkibuf(struct lua_State *L, int idx);
> >>
> >> 1. IMO 'check' is almost always worse than 'is'. Because you leave
> >> a user no choice but to use lua_cpcall if he does not want an
> >> exception. With 'is' he would be able to decide whether he wants to
> >> throw. The same for the method below.
> > 
> > I personally prefer *is* Lua functions. It is good to have *is* variants
> > in the public API aside of (or even instead of) *check* ones. I don't
> > insist, however.
> 
> This is what I said. *is* is better than *check*. More freedom to decide
> what to do, when it returns false.

I just agreed explicitly.

> 
> >> Also what exactly is the reason you need the ibuf checking for?
> >> Ibuf is not exposed as a part of tarantool binary. It is a part of
> >> small library. When we want to export parts of small from inside
> >> of the executable, we need to obfuscate the types and wrap the
> >> functions, because user's small library may be different from the
> >> executable's small.
> > 
> > It seems, we really should expose non-opacue <box_ibuf_t>: rpos and wpos
> > are used in merger (it would be too expensive to wrap them into calls).
> > The module accepts ibuf created from Lua (tarantool's <struct ibuf>), so
> > linking with external small is not an option (unless we'll really care
> > about ABI).
> 
> It seems you care about ABI a lot. So why do you stop now? If we don't care
> about ABI that much, then why do we worry about box_key_part_def_t?

'unless we'll really care about ABI' was regarding the small ABI. Sorry,
now I see that it is not obvious from my wording.

I'll repeat options as a list just in case:

1. Expose <box_ibuf_t> and <box_ibuf_*>() from the module API.
2. Ensure that we can hold small's ibuf API / ABI (from several sides:
   ability to extend it in future, static asserts, testing, no implicit
   dependency on a particular toolchain behaviour) and use it directly.

Both ways are okay for me. The second one is maybe better, but it is
more dangerous considering our current deadlines.

I would highlight that just 'let's use small directly, unlikely it will
be changed' does not work in the long terms. So the second way is not
cheap, IMHO. Many things should be considered before we'll able to state
that, say, ibuf API / ABI is perfectly stable.

> 
> I am ok with box_ibuf_t, of any kind. Then we could eventually legally expose
> tarantool_lua_ibuf, as at least for single alloc+free pairs it is better than
> region (a tiny tiny better).
> 
> > Maybe we can alias <box_ibuf_t> with <struct ibuf> if we'll add static
> > asserts around the structure size and offset of fields? And expose
> > box_ibuf_*(), of course.

I meant non-opaque structure and it seems I was not right. We cannot
alias <box_ibuf_t> to <struct ibuf> now and, when we'll change
<struct ibuf>, provide another <box_ibuf_t>, which will have the same
layout as the old <struct ibuf>. The reason is that 'real' <struct ibuf>
is already available via Lua ('buffer' module and via net.box). So if
we'll going this way (expose non-opaque <box_ibuf_t>), we should typedef
it to another structure right now and return
cdata<struct this_new_structure> from Lua. It seems, it would be good to
avoid this extra complexity of code and APIs.

So let's consider opaque <box_ibuf_t> approach. It'll add indirect calls
on hot paths (for each tuple reading from a buffer or writing to a
buffer), but I don't see any good way to overcome this. Ever increasing
of tuple's reference counter will be indirect call for an external
module.

So lot calls to do memcpy() and advance a pointer...

> 
> Also looks fine. However returning to the topic, it seems not related to the
> Lua type ids. You don't need ibuf definition to get its type ID in C. All
> is done in luaL_ctypeid function taking a string anyway.
> 
> 	luaL_ctypeid(L, "struct ibuf *");
> 	luaL_ctypeid(L, "struct ibuf");
> 
> So why does Timur need that method exported? luaL_checkibuf is a trival
> function, which can be implemented by luaL_iscdata() + getting ibuf type
> id using luaL_ctypeid() and comparing result with the cdata type id.

Timur does not need this method, he can implement it on the module side.

But since our built-in module 'buffer' provides cdata<struct ibuf> and
cdata<struct ibuf *>, it would be good to have Lua/C accessors in the
module API. It is simple, so there are no much questions for design, and
it looks worthful to expose it while we're here.

I would say the same for cdata<struct key_def &>, but since there is
another similar cdata type offered by the external key_def module, it
has no practical reason: we should handle both anyway.

Maybe uuid and decimal too, but I'm not sure. It is hard to estimate how
many times those functions will be used in modules (that are written by
us or by users). I would think about this like so: since we provide
ability to write modules using Lua/C and have cdata<struct foo> objects
in Lua API, it would be good to have luaT_isfoo() and luaT_pushfoo()
functions in the module API.

Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup

[Alexander Turenko]

On Wed, Sep 30, 2020 at 01:19:19AM +0200, Vladislav Shpilevoy wrote:
> On 29.09.2020 07:03, Alexander Turenko wrote:
> > On Tue, Sep 29, 2020 at 12:21:02AM +0200, Vladislav Shpilevoy wrote:
> >> Thanks for the patch!
> >>
> >> See 2 comments below.
> >>
> >> On 24.09.2020 19:00, Timur Safin wrote:
> >>> Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`
> >>
> >> 1. Do you really need this method? It looks like it can be done by
> >>
> >> 	old_parts = box_key_def_dump_parts(old_key_def);
> >> 	new_key_def = box_key_def_new_ex(old_parts);
> >>
> >> So the method seems redundant.
> > 
> > It is not strictly necessary, however using of box_key_def_dup() would
> > be less error-prone (no extra allocations) and the resulting code would
> > be more readable. My vote is for this method if you have no strict
> > objections.
> 
> Regarding this one I am not strictly against, but I don't like overloading
> module API with too many methods, especially with the trivial ones, which
> can be easily implemented via the others in a few lines.

I'll not strictly against if Timur will decide to going this way, but
personally I would not be very comfortable with copying over
serialization-deserialization if I would be author of the external
module.

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Alexander Turenko]

On Wed, Sep 30, 2020 at 01:21:13AM +0200, Vladislav Shpilevoy wrote:
> On 29.09.2020 07:17, Alexander Turenko wrote:
> > On Tue, Sep 29, 2020 at 12:21:18AM +0200, Vladislav Shpilevoy wrote:
> >> Thanks for the patch!
> >>
> >> I strongly don't like this export. It seems to be too internal.
> > 
> > I have no point to start discussion. Can you clarify your feeling?
> 
> I don't like that we give access to fiber's Lua state. If a user will
> leave anything on it, it can lead to anything as well, UB I guess. But
> I didn't check.
> 
> >> But I have no better ideas than propose to implement your own
> >> lua_State cache in the merger module. It does not seem to be too
> >> hard, and I don't think it would occupy much memory.
> >>
> >> Just a simple list of lua_State objects, created by luaT_newthread
> >> on demand, and put back into the list when unused. What is wrong
> >> with that?
> > 
> > When we able to simplify modules, it worth to do so (when there are no
> > certain objections). Write once, use many.
> 
> What are applications for that except the merger? Potentially.

When we want to call a Lua function (some callback), which can
potentially yield (so we can't use luaT_state()), but we have no direct
access to some current Lua state. It may be because we're in box C
function or because of API design: for merger it is because API of
source's virtual functions are Lua agnostic.

So... maybe other projects with pure C part and Lua/C part, where C code
may call Lua/C code in some way. Say, when there is general pure C code and
particular implementation of some API, which works with Lua objects.
Something designed as quite extensible thing.

Anyway, I already agreed that it is not as simple as cache and we should
return back to it later if there will be a need.

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Igor Munkin]

Sasha,

Thanks a lot for the benchmarks and clarifications!

On 30.09.20, Alexander Turenko wrote:
> On Wed, Sep 30, 2020 at 01:23:17AM +0200, Vladislav Shpilevoy wrote:

<snipped>

> 
> The performance difference also looks small (I shared benchmarking results
> in this mailing thread). Since there are questions about design of those
> functions, I would use lua_newthread() (or wrapper around it to handle
> OOM -- written in the module). We can return back to this API later.
> 

Perfect! I would gladly join the discussion and design for the vital
public API interfaces.

<snipped>

> > 
> > It is not that simple. Whatever you export now, we will need to support
> > potentially forever. So exporting not vital things increases support costs
> > for us for a very long time by saving a few time to you and Timur so you
> > don't need to change merger code and can move it as is. I would better
> > one time made merger less depending on Tarantool internals, then do long
> > support of the new pile of internal methods exposed in a hurry.
> 
> I agree, but also I consider this activity as good opportunity to
> improve the module API. Of course, when we have good design.

I agree with Vlad here. I totally understand your motivation but I
strongly doubt we should mix these issues (i.e. exporting the methods
necessary for merger and public API enhancements).

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module

[Timur Safin]

Before getting into all gory details and start to actively participate in discussion 
I want to highlight some basic principles which we used while preparing these simple 
patches (some are obvious, but some not that much):

- I try to offload functionality to the code existing inside of Tarantool as much as 
possible. If there are some structures or functions - refer to them. And this principle 
better be applied not only to Tarantool core specific functionality, but also to all 3rd 
party functions it uses. E.g. small, msgpuck, or even heap.

- And current principle to compile against them is simple - we use these 3rd party headers 
for compilation of functions in the module, but ignore libraries for a moment. We delay 
binding till run-time where system loader would substitute symbol from Tarantool executable 
which eventually called us as external module. If we could use msgpuck or small in this 
manner then we almost entirely avoiding most ABI incompatibility problems. 

- The question is: where to get those 3rd party headers? We may use several approaches 
  in modules:

1. via cmake ExternalProject_Add to github repo;
2. via git submodule to github repo;
3. via systems package manager dependency and binary dependency (e.g. apt or rpm).

So happened, that in merger we used several of them, e.g. submodule for small https://github.com/tsafin/tarantool-merge/tree/master/src
tarantool-dev system package for msgpuck. And simply copy-pasted heap.h from salad.
[IMVHO, salad worth to be treated similarly as small. For better reuse we may export
It to external repository]

3 different approaches for quite similar cases of code reuse. :) Eventually we may 
develop the consistent approach here - how to reuse 3rd party Tarantool library 
in ABI stable way, which may be originating of different versions of Tarantool? 
(e.g. small in 2 its incarnations from 2.x and 1.10)

Would it be always enough to use older version of headers, and run-time linking with 
parent executable? At the moment I see no reason to not do so. As far as I can see 
small is quite similar from ABI prospective between 1.10 and 2.x at the moment, 
we could start away as is, but certainly should add binary compatibility check somewhere 
into CI (to small repo, to Tarantool repo, and/or to merger repo) for future 
compatibility guarantees. I do assume that small repository was created just for such
usage scenarios like here (module reusing some useful functionality), and it's 
correct way to use it as submodule or ExternalProject_ in cmake. We just need to
make sure it will not broken later.

Same for msgpuck - it's already created for better code reuse - we just need to 
Make sure it's possible to be used in future compatible way. (Via extra CI here 
and there).

Having said all that, now we could return to patches discussions - see my 
other emails...

Regards,
Timur


: -----Original Message-----
: From: Timur Safin <tsafin@tarantool.org>
: Sent: Thursday, September 24, 2020 8:00 PM
: To: v.shpilevoy@tarantool.org; alexander.turenko@tarantool.org
: Cc: Timur Safin <tsafin@tarantool.org>; tarantool-
: patches@dev.tarantool.org
: Subject: [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua
: module
: 
: This patchset is a continuation of patch series which Alexander Turenko
: has sent
: earlier on. It extends module api with the necessary wrappers we should
: have
: in api to make external merger possible. Some internal functionality which
: was acessible to internal merger, should be wrapped via accessors if
: merger
: is becoming external.
: 
: - most of introduced functions are simple public wrappers around of
: internal
:   functions, e.g. `box_tuple_validate` is a wrapper around
:   `tuple_validate_raw`, or `bax_key_def_dup` is a wrapper for
: `key_def_dup`.
:   They were necessary to not reveal implementation specific details
:   (i.e. `tuple_data` needed in `tuple_validate_raw` should know
: `box_tuple_t`
:   layout);
: - wherever we used to use `struct tuple *` we replacing them with public
: alias
:   `box_tuple_t`. Same is for `struct tuple_format` -> `box_tuple_format_t`
: and
:   other typedefs.
: 
: - eventually merger in Tarantool core might get deprecated and all new
:   development for merger will continue in version agnostic external module
:   thus we decided that common utility functions (e.g.
: `luaT_temp_luastate`)
:   which used to reside in merger to better be sitting in `src/lua/utils.c`
:   instead (in addition to becoming public as described above).
: 
: NB! You could observe, that this part of changes for merger in module_api
: is
: much, much easier than those for key_def as sent by Sasha Turenko before.
: 
: Issue:
: * https://github.com/tarantool/tarantool/issues/5273
:   ('module api: expose everything that is needed for external key_def
: module')
: 
: Branches:
: * https://github.com/tarantool/tarantool/tree/tsafin/gh-5273-expand-
: module-api
:   (top 7 commits above of 14 @Totktonada's commits)
: * https://github.com/tarantool/tarantool/tree/tsafin/gh-5273-expand-
: module-api-1.10
:   (last 9 commits above of 16 @Totktonada's commits)
: 
: == External merger module
: 
: Currently external merger is residing here
: https://github.com/tsafin/tarantool-merge
: and it uses key_def from https://github.com/Totktonada/key_def repository
: via simple luarocks dependency (we had to introduce debugging rock-server
: to
: make this working).
: 
: CI is installed and is running on GitHub Actions, thus we know that it's
: sort of working together, but more extensive testing of merger and key_def
: yet to be proceeded. Though it might be done independently of current
: kernel patches.
: 
: 
: Timur Safin (7):
:   module api: export box_tuple_validate
:   module api: export box_key_def_dup
:   module api: luaT_newthread
:   module api: luaL_register_module & luaL_register_type
:   module api: luaT_temp_luastate & luaT_release_temp_luastate
:   module api: luaL_checkibuf & luaL_checkconstchar
:   module api: luaL_cdata_iscallable
: 
:  src/box/key_def_api.c |  6 +++
:  src/box/key_def_api.h | 10 +++++
:  src/box/lua/merger.c  | 78 -------------------------------------
:  src/box/tuple.c       |  6 +++
:  src/box/tuple.h       | 11 ++++++
:  src/exports.h         | 10 +++++
:  src/lua/utils.c       | 51 ++++++++++++++++++++++++-
:  src/lua/utils.h       | 89 ++++++++++++++++++++++++++++++++++++-------
:  8 files changed, 169 insertions(+), 92 deletions(-)
: 
: --
: 2.20.1

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Timur Safin]

: From: Igor Munkin <imun@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 5/7] module api:
: luaT_temp_luastate & luaT_release_temp_luastate
: 
: Vlad,
: 
: I guess there should be a question to be asked in scope of the original
: series: what performance enhancement can we obtain with such hacky
: optimization? Do we have any benchmarks? If the performance impact is
: negligible I believe these interfaces should be left internal.

That's quite good question - till some moment in the past we have used 
Internal module implementation with fiber stack optimization 
disabled, but then, when we have realized we could not avoid 
module api extension, I've moved it back to utils

Please see the older version with some code disabled due to 
inaccessible some of fiber() internal data - 
https://github.com/tsafin/tarantool-merge/commit/25c62cc01c487b2a2a6ea140340e50a0d2cb3e2a#diff-2f3bd595507f3ac1f19396c3d20c6cc0L192-L233

We could move it back to module, but I need some advice how to not lost
some speed due to not reusing existing stack?

: 
: On 29.09.20, Vladislav Shpilevoy wrote:
: > Thanks for the patch!
: >
: > I strongly don't like this export. It seems to be too internal.
: >
: > But I have no better ideas than propose to implement your own
: > lua_State cache in the merger module. It does not seem to be too
: > hard, and I don't think it would occupy much memory.
: 
: Well, such exercise looks like fun but it's better to make it once and
: provide user-friendly interfaces by so called "modulekit" to be used in
: third party modules. Anyway, using this routine seems a bit complicated
: than <lua_newstate> and <luaT_newstate> (also fully implemented via Lua
: C standart API + already exported <luaT_call>). Ergo one ought to push
: extern developers using the new one besides exporting it.
: 
: >
: > Just a simple list of lua_State objects, created by luaT_newthread
: > on demand, and put back into the list when unused. What is wrong
: > with that?
: 
: Again, how much will performance be nerfed if we simply create a new Lua
: coroutine by demand?

Looks like Alexander Turenko has already answered this question -
But I'll return back to you soon with discussion on fiber stack reuse 
details...


: 
: >
: > Adding Igor as Lua master to help with this.
: >
: > See 2 comments below.
: >
: > > diff --git a/src/lua/utils.h b/src/lua/utils.h
: > > index 9b1fe7e57..da0140076 100644
: > > --- a/src/lua/utils.h
: > > +++ b/src/lua/utils.h
: > > @@ -554,6 +554,41 @@ luaL_iscallable(lua_State *L, int idx);
: > >  struct lua_State *
: > >  luaT_newthread(struct lua_State *L);
: > >
: 
: <snipped>
: 
: >
: > 2. I would encapsulate the out parameters into some kind of
: > luaT_temp_state_ctx or something like that. Probably not even
: > opaque, but extendible.
: 
: Totally agree here. Current interface is too fragile and tricky and IMHO
: has to be redesigned/reworked prior to be exported for public usage.
: 
: >
: > Also maybe worth grouping these methods by using a common prefix:
: >
: > 	luaT_temp_luastate_get
: > 	luaT_temp_luastate_release
: >
: > > + *
: > > + * Return NULL and set a diag at failure.
: > > + */
: > > +struct lua_State *
: > > +luaT_temp_luastate(int *coro_ref, int *top);
: > > +
: > > +/**
: > > + * Release a temporary Lua state.
: > > + *
: > > + * It complements `luaT_temp_luastate()`.
: > > + */
: > > +void
: > > +luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int
: top);
: > > +
: > >  /** \endcond public */
: > >
: > >  /**
: > >
: 
: Finally, I see no strict arguments *for* exporting these routines, and
: have many doubts regarding its current design and *obligatoriness*. Yes,
: they may be useful inside the platform but do they look like ones user
: must have? I tend to "no" for now.

Yup, time to return back and polish prior local to module design.

: 
: --
: Best regards,
: IM

Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export
: box_tuple_validate
: 
: Hi! Thanks for the patch!
: 
: See 2 comments below.
: 
: On 24.09.2020 19:00, Timur Safin wrote:
: > For external merger we need means to valudate tuple data,
: 
: 1. valudate -> validate.

Haha, thanks!

: 
: > thus exporting `box_tuple_validate` which is wrapper around
: > `tuple_validate_raw` without revealing access to tuple
: > internals.
: >

Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export
: box_key_def_dup
: 
: On Wed, Sep 30, 2020 at 01:19:19AM +0200, Vladislav Shpilevoy wrote:
: > On 29.09.2020 07:03, Alexander Turenko wrote:
: > > On Tue, Sep 29, 2020 at 12:21:02AM +0200, Vladislav Shpilevoy wrote:
: > >> Thanks for the patch!
: > >>
: > >> See 2 comments below.
: > >>
: > >> On 24.09.2020 19:00, Timur Safin wrote:
: > >>> Exporting `box_key_def_dup` as accessor to the internal
: `key_def_dup`
: > >>
: > >> 1. Do you really need this method? It looks like it can be done by
: > >>
: > >> 	old_parts = box_key_def_dump_parts(old_key_def);
: > >> 	new_key_def = box_key_def_new_ex(old_parts);
: > >>
: > >> So the method seems redundant.
: > >
: > > It is not strictly necessary, however using of box_key_def_dup() would
: > > be less error-prone (no extra allocations) and the resulting code
: would
: > > be more readable. My vote is for this method if you have no strict
: > > objections.
: >
: > Regarding this one I am not strictly against, but I don't like
: overloading
: > module API with too many methods, especially with the trivial ones,
: which
: > can be easily implemented via the others in a few lines.
: 
: I'll not strictly against if Timur will decide to going this way, but
: personally I would not be very comfortable with copying over
: serialization-deserialization if I would be author of the external
: module.

Thanks Sasha! Honestly I didn't expect **this** function to be discussed 
at all, because it was simple convenience wrapper around function 
existing in both 1.10 and 2.*. So I'd rather prefer to keep it as part
of module api.

Timur

Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 2/7] module api: export
: box_key_def_dup
: 
: Thanks for the patch!
: 
: See 2 comments below.
: 
: On 24.09.2020 19:00, Timur Safin wrote:
: > Exporting `box_key_def_dup` as accessor to the internal `key_def_dup`
: 
: 1. Do you really need this method? It looks like it can be done by
: 
: 	old_parts = box_key_def_dump_parts(old_key_def);
: 	new_key_def = box_key_def_new_ex(old_parts);
: 
: So the method seems redundant.

Meh! [I do understand that here we are on a weak grounds of pure taste 
preferences, which usually should be out of discussion during patchset 
review. But if you were asking me...]

Introducing simple accessor method to already existing internal 
function for me, much more preferred way that introducing newer,
extended variance of another, lower level function, and combining 
it with 2nd lower level method. 

Module api, ideally, should be high-level for external module 
developer, thus (holistically speaking) 1 higher-level wrapper 
is better than 2 lower-level wrappers. But, certainly, this is
per personal tastes, thus we could hardly get to the final,
single opinion. 

: 
: > diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
: > index 8dd6eb10b..a419b712c 100644
: > --- a/src/box/key_def_api.h
: > +++ b/src/box/key_def_api.h
: > @@ -173,6 +173,16 @@ box_key_part_def_create(box_key_part_def_t *part);
: >  API_EXPORT box_key_def_t *
: >  box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count);
: >
: > +/**
: > + * Duplicate key_def.
: > + * \param key_def Original key_def.
: > + *
: > + * \retval not NULL Duplicate of src.
: > + * \retval     NULL Memory error.
: 
: 2. OCD mode intensifies.

Take care! (hehe, will fix)

: 
: :(
: Lets use @ everywhere in the new code instead of \.

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [PATCH 2.X 3/7] module api: luaT_newthread
: 
: On Thu, Sep 24, 2020 at 08:00:16PM +0300, Timur Safin wrote:
: > Exporting `luaT_newthread` as it's necessary for external merger
: 
: But you expose accessors for the temporary Lua state in the later patch.
: Built-in merger only uses luaT_newthread() in luaT_temp_luastate().

Good point! Indeed - it would be unnecessary iff we keep
luaT_temp_luastate part of module api...

But looks like we are not (and would copy corresponding code back to 
merger' utils code), so we should consider this patch once again as 
necessary.

Timur

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 3/7] module api:
: luaT_newthread
: 
: Thanks for the patch!
: 
: See 3 comments below.
: 
: On 24.09.2020 19:00, Timur Safin wrote:
: > Exporting `luaT_newthread` as it's necessary for external merger
: >
: > Part of #5273
: > ---
: >  src/exports.h   |  1 +
: >  src/lua/utils.h | 25 +++++++++++++------------
: >  2 files changed, 14 insertions(+), 12 deletions(-)
: >
: > diff --git a/src/exports.h b/src/exports.h
: > index 202f5bf19..20070b240 100644
: > --- a/src/exports.h
: > +++ b/src/exports.h
: > @@ -412,6 +412,7 @@ EXPORT(luaT_checktuple)
: >  EXPORT(luaT_cpcall)
: >  EXPORT(luaT_error)
: >  EXPORT(luaT_istuple)
: > +EXPORT(luaT_newthread)
: >  EXPORT(luaT_pushtuple)
: >  EXPORT(luaT_state)
: >  EXPORT(luaT_tolstring)
: > diff --git a/src/lua/utils.h b/src/lua/utils.h
: > index e9dd27d08..7639cd64a 100644
: > --- a/src/lua/utils.h
: > +++ b/src/lua/utils.h
: > @@ -539,6 +539,19 @@ luaT_tolstring(lua_State *L, int idx, size_t
: *ssize);
: >  LUA_API int
: >  luaL_iscallable(lua_State *L, int idx);
: >
: > +
: 
: 1. Extra empty line.

Yup.
: 
: > +/**
: > + * @brief Creates a new Lua coroutine in a protected frame. If
: > + * <lua_newthread> call underneath succeeds, the created Lua state
: > + * is on the top of the guest stack and a pointer to this state is
: > + * returned. Otherwise LUA_ERRMEM error is handled and the result
: > + * is NULL.
: > + * @param L is a Lua state
: 
: 2.
: 	@param L Lua state.
: 
: Or probably could be even dropped. Such description is useless anyway.

Do you consider that L is obvious for external author 
that it means Lua state? I'd rather disagree.

: 
: > + * @sa <lua_newthread>
: 
: 3. It is worth mentioning that the error is set into the diagnostics
: area.

Indeed, thanks!

Timur

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Alexander Turenko]

On Fri, Oct 02, 2020 at 03:26:47PM +0300, Timur Safin wrote:
> : From: Alexander Turenko <alexander.turenko@tarantool.org>
> : Subject: Re: [PATCH 2.X 3/7] module api: luaT_newthread
> : 
> : On Thu, Sep 24, 2020 at 08:00:16PM +0300, Timur Safin wrote:
> : > Exporting `luaT_newthread` as it's necessary for external merger
> : 
> : But you expose accessors for the temporary Lua state in the later patch.
> : Built-in merger only uses luaT_newthread() in luaT_temp_luastate().
> 
> Good point! Indeed - it would be unnecessary iff we keep
> luaT_temp_luastate part of module api...
> 
> But looks like we are not (and would copy corresponding code back to 
> merger' utils code), so we should consider this patch once again as 
> necessary.

It seems, me and Vlad agreed on using lua_newthread() inside the module.
Don't know whether it worth to wrap it with Lua OOM error catching now.
The code at whole should consider possibility of such error, but now it
does not.

Since there is no agreement on this topic, I would say that we should
postpone it until we'll elaborate certain pathway how a module developer
should overcome possible OOM errors. You know, you can get it from any
function that perform allocation of a new Lua object. So protection of
just one allocation function does not change the situation at whole.

Maybe we should elaborate certain recommendations how to determine code
sections that should be protected and how to actually protect them.
Current merger code unlikely handle Lua OOM gracefully.

I would also add here, that a mechanish of Lua OOM error injection is
necessary, because any contract that is not tested is a kind of lie.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Timur Safin]

: From: Igor Munkin <imun@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 3/7] module api:
: luaT_newthread
: 
: Timur,
: 
: Thanks for the patch, but I guess this one can be implemented using the
: existing public interfaces (i.e. Lua C standart API + already exported
: <luaT_call>). It was done to resolve the issue with Tarantool internal
: crash[1] and I believe if user need to handle OOM error it can do it by
: himself.
: 
: [1]: https://github.com/tarantool/tarantool/issues/4556
: 
: --
: Best regards,
: IM

I do not quite get your point here - here we do want to use that newly
introduced wrapper luaT_newthread in the external module. Do you suggest
we reimplement it similarly in the external module? We tried and incr_top()
was the problematic if we live outside of Tarantool core.
Or do you mean that we do not want to deal with incr_top(L) from outside?


Timur

Re: [Tarantool-patches] [PATCH 2.X 7/7] module api: luaL_cdata_iscallable

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 7/7] module api:
: luaL_cdata_iscallable
: 
: Thanks for the patch!
: 
: See 4 comments below.
: 
: On 24.09.2020 19:00, Timur Safin wrote:
: > In addition to `luaL_iscallable` we need `luaL_cdata_iscallable`
: > because code which was calling it directly had to be copied to
: > merger side
: 
: 1. Please, put '.' in the end of sentences. Here and in other
: commits.
: 
: 2. luaL_iscallable() already checks luaL_cdata_iscallable(). Why
: do you need it separated from luaL_iscallable()?

Good question - indeed. I needed luaL_cdata_iscallable only while 
luaL_iscallable was part of compat layer inside of merger module. Now
having luaL_iscallable as part of module api both in 1.10 and 2.* 
the luaL_cdata_iscallable could be local, as it used to be before.

Let me check whether I miss some details I needed in both versions we 
attack?

: 
: > Part of #5273
: > ---
: >  src/exports.h   | 1 +
: >  src/lua/utils.c | 2 +-
: >  src/lua/utils.h | 6 ++++++
: >  3 files changed, 8 insertions(+), 1 deletion(-)
: >
: > diff --git a/src/lua/utils.h b/src/lua/utils.h
: > index 6b10d2755..69ff4de86 100644
: > --- a/src/lua/utils.h
: > +++ b/src/lua/utils.h
: > @@ -541,6 +541,12 @@ luaT_tolstring(lua_State *L, int idx, size_t
: *ssize);
: >  LUA_API int
: >  luaL_iscallable(lua_State *L, int idx);
: >
: > +/**
: > + * Check whether a Lua object is a cdata metatype with a __call field.
: 
: 3. The function will crash in debug build, if it is not cdata. And will
: do UB in release build. This is because it is not supposed to be called
: out of luaL_iscallable(). And I don't see why would you need it.

I hear, hear. Don't need to repeat it twice. Don't need to repeat it twice.

: 
: > + *
: 
: 4. Extra empty line.
: 
: > + */
: > +LUA_API int
: > +luaL_cdata_iscallable(lua_State *L, int idx);

Regards,
Timur

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Timur Safin]

: From: Igor Munkin <imun@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 4/7] module api:
: luaL_register_module & luaL_register_type
: 
: Vlad,
: 
: I glanced the patch and doubt these functions need to be exported by
: Tarantool. These functions neither wrap internals nor provide any
: performance benefits (e.g. reduce numbers of excess allocations). It
: would be nice to have such auxiliary interfaces in so called "modulekit"
: to make third party modules development easier, but I see no problem to
: borrow this code to merger/key_def as is for now.

Ok, let me put it another way:
- we are agreed that this is necessary useful functionality which might 
  be part of some static library, as part of modulekit?
- we are not ok to put those symbols to Tarantool, and use Tarantool 
  executable(s) for linking against those symbols, because it's looking
  too much intimate.

- I'd hardly imagine situation that for such modulekit code reuse we 
  would prefer to introduce yet another repository, compile it as static
  library and then add to the linking dependencies of an external module?
  (or do we?)
- as for me it's still looking part of module api (some headers, and some 
  Helpers in library), and I'd prefer to simply export them from executable...

But.. if we prefer "modulekit" then yes, I'd copy-paste this code to 
my module and to the GitHub template for Tarantool c modules (yet to be 
created). Looks redundant, but will save a couple of external symbols.

Regards,
Timur

: 
: On 29.09.20, Vladislav Shpilevoy wrote:
: > Thanks for the patch!
: >
: > See 2 comments below.
: >
: > On 24.09.2020 19:00, Timur Safin wrote:
: > > Exporting `luaL_register_module` & `luaL_register_type`
: > > functions as they needed for external merger
: > >
: > > diff --git a/src/lua/utils.h b/src/lua/utils.h
: > > index 7639cd64a..9b1fe7e57 100644
: > > --- a/src/lua/utils.h
: > > +++ b/src/lua/utils.h
: > > @@ -78,6 +78,8 @@ luaL_pushuuid(struct lua_State *L);
: > >
: > >  /** \cond public */
: > >
: > > +struct luaL_Reg;
: >
: > 1. It does not seem to be public. How are users supposed to work with
: that?
: > Igor, could you please take a look at this?
: 
: It's public[1] and is provided by <lauxlib.h> Lua standard header as
: Sasha already mentioned it in the thread.
: 
: >
: 
: <snipped>
: 
: [1]: https://www.lua.org/manual/5.1/manual.html#luaL_Reg
: 
: --
: Best regards,
: IM

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 4/7] module api:
: luaL_register_module & luaL_register_type
: 
: > > diff --git a/src/lua/utils.h b/src/lua/utils.h
: > > index 7639cd64a..9b1fe7e57 100644
: > > --- a/src/lua/utils.h
: > > +++ b/src/lua/utils.h
: > > @@ -78,6 +78,8 @@ luaL_pushuuid(struct lua_State *L);
: > >
: > >  /** \cond public */
: > >
: > > +struct luaL_Reg;
: >
: > 1. It does not seem to be public. How are users supposed to work with
: that?
: > Igor, could you please take a look at this?
: >
: > If you need it so badly, at least we need a wrapper around luaL_Reg
: > or we need to expose it and document too. Probably Igor has better
: ideas.
: 
: It is exposed from lauxlib.h, no need to duplicate in the module API.

Didn't know that - good point, thanks!

: 
: >
: > > +
: > >  /**
: > >   * @brief Checks whether a value on the Lua stack is a cdata.
: > >   *
: > > @@ -442,6 +444,8 @@ luaL_checkfield(struct lua_State *L, struct
: luaL_serializer *cfg, int idx,
: > >  	luaL_convertfield(L, cfg, idx, field);
: > >  }
: > >
: > > +/** \cond public */
: > > +
: > >  void
: > >  luaL_register_type(struct lua_State *L, const char *type_name,
: > >  		   const struct luaL_Reg *methods);
: >
: > 2. These newly exported methods don't have a single comment. Why?
: 
: luaL_register_*() are our helpers around luaL_register(), AFAIR. A
: module should use the latter.

Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 6/7] module api:
: luaL_checkibuf & luaL_checkconstchar
: 
: On Wed, Sep 30, 2020 at 01:25:15AM +0200, Vladislav Shpilevoy wrote:
: > On 29.09.2020 07:53, Alexander Turenko wrote:
: > >>> +/**
: > >>> + * Check if a value on @a L stack by index @a idx is an ibuf
: > >>> + * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
: > >>> + * Returns NULL, if can't convert - not an ibuf object.
: > >>> + */
: > >>> +struct ibuf *
: > >>> +luaL_checkibuf(struct lua_State *L, int idx);
: > >>
: > >> 1. IMO 'check' is almost always worse than 'is'. Because you leave
: > >> a user no choice but to use lua_cpcall if he does not want an
: > >> exception. With 'is' he would be able to decide whether he wants to
: > >> throw. The same for the method below.
: > >
: > > I personally prefer *is* Lua functions. It is good to have *is*
: variants
: > > in the public API aside of (or even instead of) *check* ones. I don't
: > > insist, however.
: >
: > This is what I said. *is* is better than *check*. More freedom to decide
: > what to do, when it returns false.
: 
: I just agreed explicitly.

Ok, I see the common consensus about naming here, but teher are bigger questions about small reuse...

: 
: >
: > >> Also what exactly is the reason you need the ibuf checking for?
: > >> Ibuf is not exposed as a part of tarantool binary. It is a part of
: > >> small library. When we want to export parts of small from inside
: > >> of the executable, we need to obfuscate the types and wrap the
: > >> functions, because user's small library may be different from the
: > >> executable's small.
: > >
: > > It seems, we really should expose non-opacue <box_ibuf_t>: rpos and
: wpos
: > > are used in merger (it would be too expensive to wrap them into
: calls).
: > > The module accepts ibuf created from Lua (tarantool's <struct ibuf>),
: so
: > > linking with external small is not an option (unless we'll really care
: > > about ABI).
: >
: > It seems you care about ABI a lot. So why do you stop now? If we don't
: care
: > about ABI that much, then why do we worry about box_key_part_def_t?
: 
: 'unless we'll really care about ABI' was regarding the small ABI. Sorry,
: now I see that it is not obvious from my wording.
: 
: I'll repeat options as a list just in case:
: 
: 1. Expose <box_ibuf_t> and <box_ibuf_*>() from the module API.
: 2. Ensure that we can hold small's ibuf API / ABI (from several sides:
:    ability to extend it in future, static asserts, testing, no implicit
:    dependency on a particular toolchain behaviour) and use it directly.
: 
: Both ways are okay for me. The second one is maybe better, but it is
: more dangerous considering our current deadlines.
: 
: I would highlight that just 'let's use small directly, unlikely it will
: be changed' does not work in the long terms. So the second way is not
: cheap, IMHO. Many things should be considered before we'll able to state
: that, say, ibuf API / ABI is perfectly stable.

This is big question - how to deal with 3rd party libraries here in modules
and across all repositories. Especially if we already have our library extracted
to the separate repository, like in case of small. I've put my thoughts about
this topic elsewhere (see my reply to the cover email), and this worth to 
discuss with Alexander Lyapunov also...

: 
: >
: > I am ok with box_ibuf_t, of any kind. Then we could eventually legally
: expose
: > tarantool_lua_ibuf, as at least for single alloc+free pairs it is better
: than
: > region (a tiny tiny better).
: >
: > > Maybe we can alias <box_ibuf_t> with <struct ibuf> if we'll add static
: > > asserts around the structure size and offset of fields? And expose
: > > box_ibuf_*(), of course.
: 
: I meant non-opaque structure and it seems I was not right. We cannot
: alias <box_ibuf_t> to <struct ibuf> now and, when we'll change
: <struct ibuf>, provide another <box_ibuf_t>, which will have the same
: layout as the old <struct ibuf>. The reason is that 'real' <struct ibuf>
: is already available via Lua ('buffer' module and via net.box). So if
: we'll going this way (expose non-opaque <box_ibuf_t>), we should typedef
: it to another structure right now and return
: cdata<struct this_new_structure> from Lua. It seems, it would be good to
: avoid this extra complexity of code and APIs.
: 
: So let's consider opaque <box_ibuf_t> approach. It'll add indirect calls
: on hot paths (for each tuple reading from a buffer or writing to a
: buffer), but I don't see any good way to overcome this. Ever increasing
: of tuple's reference counter will be indirect call for an external
: module.
: 
: So lot calls to do memcpy() and advance a pointer...
: 
: >
: > Also looks fine. However returning to the topic, it seems not related to
: the
: > Lua type ids. You don't need ibuf definition to get its type ID in C.
: All
: > is done in luaL_ctypeid function taking a string anyway.
: >
: > 	luaL_ctypeid(L, "struct ibuf *");
: > 	luaL_ctypeid(L, "struct ibuf");
: >
: > So why does Timur need that method exported? luaL_checkibuf is a trival
: > function, which can be implemented by luaL_iscdata() + getting ibuf type
: > id using luaL_ctypeid() and comparing result with the cdata type id.
: 
: Timur does not need this method, he can implement it on the module side.
: 
: But since our built-in module 'buffer' provides cdata<struct ibuf> and
: cdata<struct ibuf *>, it would be good to have Lua/C accessors in the
: module API. It is simple, so there are no much questions for design, and
: it looks worthful to expose it while we're here.
: 
: I would say the same for cdata<struct key_def &>, but since there is
: another similar cdata type offered by the external key_def module, it
: has no practical reason: we should handle both anyway.
: 
: Maybe uuid and decimal too, but I'm not sure. It is hard to estimate how
: many times those functions will be used in modules (that are written by
: us or by users). I would think about this like so: since we provide
: ability to write modules using Lua/C and have cdata<struct foo> objects
: in Lua API, it would be good to have luaT_isfoo() and luaT_pushfoo()
: functions in the module API.

Exactly so!

Timur

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Vladislav Shpilevoy]

> : > +/**
> : > + * @brief Creates a new Lua coroutine in a protected frame. If
> : > + * <lua_newthread> call underneath succeeds, the created Lua state
> : > + * is on the top of the guest stack and a pointer to this state is
> : > + * returned. Otherwise LUA_ERRMEM error is handled and the result
> : > + * is NULL.
> : > + * @param L is a Lua state
> : 
> : 2.
> : 	@param L Lua state.
> : 
> : Or probably could be even dropped. Such description is useless anyway.
> 
> Do you consider that L is obvious for external author 
> that it means Lua state? I'd rather disagree.

Its type is literally 'lua_State'. Why don't you describe that an
integer parameter is an 'int' then?

Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar

[Vladislav Shpilevoy]

On 02.10.2020 18:14, Timur Safin wrote:
> : From: Alexander Turenko <alexander.turenko@tarantool.org>
> : Subject: Re: [Tarantool-patches] [PATCH 2.X 6/7] module api:
> : luaL_checkibuf & luaL_checkconstchar
> : 
> : On Wed, Sep 30, 2020 at 01:25:15AM +0200, Vladislav Shpilevoy wrote:
> : > On 29.09.2020 07:53, Alexander Turenko wrote:
> : > >>> +/**
> : > >>> + * Check if a value on @a L stack by index @a idx is an ibuf
> : > >>> + * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
> : > >>> + * Returns NULL, if can't convert - not an ibuf object.
> : > >>> + */
> : > >>> +struct ibuf *
> : > >>> +luaL_checkibuf(struct lua_State *L, int idx);
> : > >>
> : > >> 1. IMO 'check' is almost always worse than 'is'. Because you leave
> : > >> a user no choice but to use lua_cpcall if he does not want an
> : > >> exception. With 'is' he would be able to decide whether he wants to
> : > >> throw. The same for the method below.
> : > >
> : > > I personally prefer *is* Lua functions. It is good to have *is*
> : variants
> : > > in the public API aside of (or even instead of) *check* ones. I don't
> : > > insist, however.
> : >
> : > This is what I said. *is* is better than *check*. More freedom to decide
> : > what to do, when it returns false.
> : 
> : I just agreed explicitly.
> 
> Ok, I see the common consensus about naming here, but teher are bigger questions about small reuse...

It is not about naming. It is about behaviour. 'is' never throws nor fails.
'check' throws/fails/sets diag.

> : 
> : >
> : > I am ok with box_ibuf_t, of any kind. Then we could eventually legally
> : expose
> : > tarantool_lua_ibuf, as at least for single alloc+free pairs it is better
> : than
> : > region (a tiny tiny better).
> : >
> : > > Maybe we can alias <box_ibuf_t> with <struct ibuf> if we'll add static
> : > > asserts around the structure size and offset of fields? And expose
> : > > box_ibuf_*(), of course.
> : 
> : I meant non-opaque structure and it seems I was not right. We cannot
> : alias <box_ibuf_t> to <struct ibuf> now and, when we'll change
> : <struct ibuf>, provide another <box_ibuf_t>, which will have the same
> : layout as the old <struct ibuf>. The reason is that 'real' <struct ibuf>
> : is already available via Lua ('buffer' module and via net.box). So if
> : we'll going this way (expose non-opaque <box_ibuf_t>), we should typedef
> : it to another structure right now and return
> : cdata<struct this_new_structure> from Lua. It seems, it would be good to
> : avoid this extra complexity of code and APIs.
> : 
> : So let's consider opaque <box_ibuf_t> approach. It'll add indirect calls
> : on hot paths (for each tuple reading from a buffer or writing to a
> : buffer), but I don't see any good way to overcome this. Ever increasing
> : of tuple's reference counter will be indirect call for an external
> : module.
> : 
> : So lot calls to do memcpy() and advance a pointer...
> : 
> : >
> : > Also looks fine. However returning to the topic, it seems not related to
> : the
> : > Lua type ids. You don't need ibuf definition to get its type ID in C.
> : All
> : > is done in luaL_ctypeid function taking a string anyway.
> : >
> : > 	luaL_ctypeid(L, "struct ibuf *");
> : > 	luaL_ctypeid(L, "struct ibuf");
> : >
> : > So why does Timur need that method exported? luaL_checkibuf is a trival
> : > function, which can be implemented by luaL_iscdata() + getting ibuf type
> : > id using luaL_ctypeid() and comparing result with the cdata type id.
> : 
> : Timur does not need this method, he can implement it on the module side.
> : 
> : But since our built-in module 'buffer' provides cdata<struct ibuf> and
> : cdata<struct ibuf *>, it would be good to have Lua/C accessors in the
> : module API. It is simple, so there are no much questions for design, and
> : it looks worthful to expose it while we're here.
> : 
> : I would say the same for cdata<struct key_def &>, but since there is
> : another similar cdata type offered by the external key_def module, it
> : has no practical reason: we should handle both anyway.
> : 
> : Maybe uuid and decimal too, but I'm not sure. It is hard to estimate how
> : many times those functions will be used in modules (that are written by
> : us or by users). I would think about this like so: since we provide
> : ability to write modules using Lua/C and have cdata<struct foo> objects
> : in Lua API, it would be good to have luaT_isfoo() and luaT_pushfoo()
> : functions in the module API.
> 
> Exactly so!

Maybe. I don't have strong opinion here, so whatever.

Re: [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module

[Vladislav Shpilevoy]

On 02.10.2020 14:23, Timur Safin wrote:
> Before getting into all gory details and start to actively participate in discussion 
> I want to highlight some basic principles which we used while preparing these simple 
> patches (some are obvious, but some not that much):
>
> - I try to offload functionality to the code existing inside of Tarantool as much as 
> possible. If there are some structures or functions - refer to them. And this principle 
> better be applied not only to Tarantool core specific functionality, but also to all 3rd 
> party functions it uses. E.g. small, msgpuck, or even heap.
>
> - And current principle to compile against them is simple - we use these 3rd party headers 
> for compilation of functions in the module, but ignore libraries for a moment. We delay 
> binding till run-time where system loader would substitute symbol from Tarantool executable 
> which eventually called us as external module. If we could use msgpuck or small in this 
> manner then we almost entirely avoiding most ABI incompatibility problems. 

The headers don't contain just symbols. They contain inlined function
definitions and non-opaque structures. Even if your module does not link
with these libraries, still some of their functions and their structs layout
penetrate into your module's binary file. And if tarantool is built with a
bit different headers with slightly changed structs layout, it will lead
to UB.

That is the main problem with ABI now. It is not about symbols. If some
symbols are missing, your module will fail to load.

> - The question is: where to get those 3rd party headers? We may use several approaches 
>   in modules:
> 
> 1. via cmake ExternalProject_Add to github repo;
> 2. via git submodule to github repo;
> 3. via systems package manager dependency and binary dependency (e.g. apt or rpm).>
> So happened, that in merger we used several of them, e.g. submodule for small https://github.com/tsafin/tarantool-merge/tree/master/src
> tarantool-dev system package for msgpuck. And simply copy-pasted heap.h from salad.
> [IMVHO, salad worth to be treated similarly as small. For better reuse we may export
> It to external repository]
> 
> 3 different approaches for quite similar cases of code reuse. :) Eventually we may 
> develop the consistent approach here - how to reuse 3rd party Tarantool library 
> in ABI stable way, which may be originating of different versions of Tarantool? 
> (e.g. small in 2 its incarnations from 2.x and 1.10)
> 
> Would it be always enough to use older version of headers, and run-time linking with 
> parent executable? At the moment I see no reason to not do so. As far as I can see 
> small is quite similar from ABI prospective between 1.10 and 2.x at the moment, 
> we could start away as is, but certainly should add binary compatibility check somewhere 
> into CI (to small repo, to Tarantool repo, and/or to merger repo) for future 
> compatibility guarantees. I do assume that small repository was created just for such
> usage scenarios like here (module reusing some useful functionality), and it's 
> correct way to use it as submodule or ExternalProject_ in cmake. We just need to
> make sure it will not broken later.
> 
> Same for msgpuck - it's already created for better code reuse - we just need to 
> Make sure it's possible to be used in future compatible way. (Via extra CI here 
> and there).
> 
> Having said all that, now we could return to patches discussions - see my 
> other emails...

If you are proposing to introduce some kind of public version of
small API, doing all the compatibility stuff, then it sounds good.
(msgpuck is already public.)

Now we are trying to solve that inside of tarantool's API, but maybe
it would be easier and more portable the other way.

Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

[Alexander Turenko]

>  | diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
>  | index 583946c4c..a4332e3d6 100644
>  | --- a/src/box/lua/merger.c
>  | +++ b/src/box/lua/merger.c
>  | @@ -530,13 +530,14 @@ luaL_merge_source_buffer_fetch_impl(struct merge_source_buffer *source,
>  |  static int
>  |  luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
>  |  {
>  | -       int coro_ref = LUA_NOREF;
>  | -       int top = -1;
>  | -       struct lua_State *L = luaT_temp_luastate(&coro_ref, &top);
>  | -       if (L == NULL)
>  | +       int top = lua_gettop(tarantool_L);
>  | +       struct lua_State *L = luaT_newthread(tarantool_L);
>  | +       if (L == NULL) {
>  | +               lua_settop(tarantool_L, top);
>  |                 return -1;
>  | +       }
>  |         int rc = luaL_merge_source_buffer_fetch_impl(source, L);
>  | -       luaT_release_temp_luastate(L, coro_ref, top);
>  | +       lua_settop(tarantool_L, top);
>  |         return rc;
>  |  }
> 
> (The patch is stupid, I know, we should not leave an extra element on
> tarantool_L and yield, but call luaL_ref / luaL_unref with Lua registry
> instead. I forget about this before measurements, but it should not be
> much important here.)

I was bother only about most performant case with the buffer source, but
there are table and tuple sources, which acquires a state per next()
call. Their performance will likely have the same order of degradation,
but per next(), not per chunk fetch. My fault.

OTOH, the merge source API (virtual functions of <struct merge_source>)
now assumes that a merge source may be written in pure C and, what is
more important, may be called from C without any Lua state. We can break
this property of the API in the module and just pass a Lua state to
those functions. It is okay for the Lua/C module: it is always called
from Lua. (The property was nice, though.)

Anyway, it may be resolved within a module either with caching of Lua
states or with contamination of pure C API with <struct lua_State>. And
it may be postponed, because has small effect on the most performant
merge source. So I vote to skip exposing of those APIs from tarantool
for now.

Re: [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module

[Alexander Turenko]

On Fri, Oct 02, 2020 at 03:23:17PM +0300, Timur Safin wrote:
> Before getting into all gory details and start to actively participate in discussion 
> I want to highlight some basic principles which we used while preparing these simple 
> patches (some are obvious, but some not that much):
> 
> - I try to offload functionality to the code existing inside of Tarantool as much as 
> possible. If there are some structures or functions - refer to them. And this principle 
> better be applied not only to Tarantool core specific functionality, but also to all 3rd 
> party functions it uses. E.g. small, msgpuck, or even heap.
> 
> - And current principle to compile against them is simple - we use these 3rd party headers 
> for compilation of functions in the module, but ignore libraries for a moment. We delay 
> binding till run-time where system loader would substitute symbol from Tarantool executable 
> which eventually called us as external module. If we could use msgpuck or small in this 
> manner then we almost entirely avoiding most ABI incompatibility problems. 
> 
> - The question is: where to get those 3rd party headers? We may use several approaches 
>   in modules:
> 
> 1. via cmake ExternalProject_Add to github repo;
> 2. via git submodule to github repo;
> 3. via systems package manager dependency and binary dependency (e.g. apt or rpm).
> 
> So happened, that in merger we used several of them, e.g. submodule for small https://github.com/tsafin/tarantool-merge/tree/master/src
> tarantool-dev system package for msgpuck. And simply copy-pasted heap.h from salad.
> [IMVHO, salad worth to be treated similarly as small. For better reuse we may export
> It to external repository]
> 
> 3 different approaches for quite similar cases of code reuse. :) Eventually we may 
> develop the consistent approach here - how to reuse 3rd party Tarantool library 
> in ABI stable way, which may be originating of different versions of Tarantool? 
> (e.g. small in 2 its incarnations from 2.x and 1.10)
> 
> Would it be always enough to use older version of headers, and run-time linking with 
> parent executable? At the moment I see no reason to not do so. As far as I can see 
> small is quite similar from ABI prospective between 1.10 and 2.x at the moment, 
> we could start away as is, but certainly should add binary compatibility check somewhere 
> into CI (to small repo, to Tarantool repo, and/or to merger repo) for future 
> compatibility guarantees. I do assume that small repository was created just for such
> usage scenarios like here (module reusing some useful functionality), and it's 
> correct way to use it as submodule or ExternalProject_ in cmake. We just need to
> make sure it will not broken later.
> 
> Same for msgpuck - it's already created for better code reuse - we just need to 
> Make sure it's possible to be used in future compatible way. (Via extra CI here 
> and there).
> 
> Having said all that, now we could return to patches discussions - see my 
> other emails...
> 
> Regards,
> Timur

You cannot lean on API / ABI compatibility now and implement it later.
You should hold layout of non-opaque structures, think how they will be
extended in a future (introduce padding, maybe), probably introduce some
level of indirectness (public + internal structures, calls instead of
inline functions) and estimate performance impact. You should try to
defend your approach and maybe reimplement it two-three times.

Almost the whole task was about porting the internal modules to the
stable API (the rest is trivial). Either expose something from
tarantool, or provide stable API from small and msgpuck. I said this a
lot of times during the last quarter. I sent you draft of [1] August, 3.
Two months later, a week before planned feature freeze, you say
'eventually we'll do it' and continue attempts to push the team to agree
on the fragile approach.

You say 'I assume it will work' ignoring past breakage cases [1]. You
say 'we can just add binary layout checks later', however you're surely
saw [2] and know that extra work is necessary before freezing ABI.

Optimistic ABI compatibility? I just don't understand that all.

[1]: https://lists.tarantool.org/pipermail/tarantool-discussions/2020-September/000095.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-September/019592.html

Re: [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type

[Alexander Turenko]

On Fri, Oct 02, 2020 at 07:14:19PM +0300, Timur Safin wrote:
> : From: Igor Munkin <imun@tarantool.org>
> : Subject: Re: [Tarantool-patches] [PATCH 2.X 4/7] module api:
> : luaL_register_module & luaL_register_type
> : 
> : Vlad,
> : 
> : I glanced the patch and doubt these functions need to be exported by
> : Tarantool. These functions neither wrap internals nor provide any
> : performance benefits (e.g. reduce numbers of excess allocations). It
> : would be nice to have such auxiliary interfaces in so called "modulekit"
> : to make third party modules development easier, but I see no problem to
> : borrow this code to merger/key_def as is for now.
> 
> Ok, let me put it another way:
> - we are agreed that this is necessary useful functionality which might 
>   be part of some static library, as part of modulekit?

No.

As a module developer, why do you need to have functions in the
metatable itself, not just in __index?

Why do you need to hide a metatable with __metatable?

Why do you need to register a module in some unobvious way? Just
register it as foo.bar (require('foo.bar') will work) and do `return
{bar = require('foo.bar')}` in foo.lua / foo.c (require('foo').bar will
work). If you want to forbid require('foo.bar'), just do
`package.loaded['foo.bar'] = nil` inside foo.lua / foo.c. (And it is
necessary when you have something relatively complex: two level of
nesting.)

I don't want to discuss it on the level 'I guess it is useful'. You
should understand use cases to advocate it.

Re: [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread

[Igor Munkin]

Timur,

On 02.10.20, Timur Safin wrote:
> : From: Igor Munkin <imun@tarantool.org>
> : Subject: Re: [Tarantool-patches] [PATCH 2.X 3/7] module api:
> : luaT_newthread
> : 
> : Timur,
> : 
> : Thanks for the patch, but I guess this one can be implemented using the
> : existing public interfaces (i.e. Lua C standart API + already exported
> : <luaT_call>). It was done to resolve the issue with Tarantool internal
> : crash[1] and I believe if user need to handle OOM error it can do it by
> : himself.
> : 
> : [1]: https://github.com/tarantool/tarantool/issues/4556
> : 
> : --
> : Best regards,
> : IM
> 
> I do not quite get your point here - here we do want to use that newly
> introduced wrapper luaT_newthread in the external module. Do you suggest
> we reimplement it similarly in the external module? We tried and incr_top()
> was the problematic if we live outside of Tarantool core.
> Or do you mean that we do not want to deal with incr_top(L) from outside?

You literally was around to this function, so you can look again: it has
no LuaJIT internals at all. Once more: this helper can be implemented
via public Lua C standard API (provided by lua.h/lauxlib.h) and
<luaT_call> (provided by module.h). Just borrow the implementation from
lua/utils.c, this is about 40loc with verbose comments -- that's all.

> 
> 
> Timur
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Alexander Turenko]

On Tue, Sep 29, 2020 at 08:25:50AM +0300, Alexander Turenko wrote:
> > Part of #5273
> 
> Sorry for nitpicking, but the issue states 'expose everything that is
> needed for external key_def module'. The commit technically is not part
> of the issue.
> 
> Either change the issue, or link commits with
> tarantool/vshard-cluster-api#5, or file a separate issue (I would do the
> latter).
> 
> It is just to don't mix different things into one: #5273 have certain
> definition of done. It will be hard to understand what going on in the
> future if we'll mix everything together.
> 
> (It is the same for all commits.)

Since there was no reaction, I filed
https://github.com/tarantool/tarantool/issues/5384

Please, attach your commits to this issue.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 6/7] module api:
: luaL_checkibuf & luaL_checkconstchar
: 
: On Wed, Sep 30, 2020 at 01:25:15AM +0200, Vladislav Shpilevoy wrote:
: > On 29.09.2020 07:53, Alexander Turenko wrote:
: > >>> +/**
: > >>> + * Check if a value on @a L stack by index @a idx is an ibuf
: > >>> + * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
: > >>> + * Returns NULL, if can't convert - not an ibuf object.
: > >>> + */
: > >>> +struct ibuf *
: > >>> +luaL_checkibuf(struct lua_State *L, int idx);
: > >>
: > >> 1. IMO 'check' is almost always worse than 'is'. Because you leave
: > >> a user no choice but to use lua_cpcall if he does not want an
: > >> exception. With 'is' he would be able to decide whether he wants to
: > >> throw. The same for the method below.
: > >
: > > I personally prefer *is* Lua functions. It is good to have *is*
: variants
: > > in the public API aside of (or even instead of) *check* ones. I don't
: > > insist, however.
: >
: > This is what I said. *is* is better than *check*. More freedom to decide
: > what to do, when it returns false.
: 
: I just agreed explicitly.
: 
: >
: > >> Also what exactly is the reason you need the ibuf checking for?
: > >> Ibuf is not exposed as a part of tarantool binary. It is a part of
: > >> small library. When we want to export parts of small from inside
: > >> of the executable, we need to obfuscate the types and wrap the
: > >> functions, because user's small library may be different from the
: > >> executable's small.
: > >
: > > It seems, we really should expose non-opacue <box_ibuf_t>: rpos and
: wpos
: > > are used in merger (it would be too expensive to wrap them into
: calls).
: > > The module accepts ibuf created from Lua (tarantool's <struct ibuf>),
: so
: > > linking with external small is not an option (unless we'll really care
: > > about ABI).
: >
: > It seems you care about ABI a lot. So why do you stop now? If we don't
: care
: > about ABI that much, then why do we worry about box_key_part_def_t?
: 
: 'unless we'll really care about ABI' was regarding the small ABI. Sorry,
: now I see that it is not obvious from my wording.
: 
: I'll repeat options as a list just in case:
: 
: 1. Expose <box_ibuf_t> and <box_ibuf_*>() from the module API.
: 2. Ensure that we can hold small's ibuf API / ABI (from several sides:
:    ability to extend it in future, static asserts, testing, no implicit
:    dependency on a particular toolchain behaviour) and use it directly.
: 
: Both ways are okay for me. The second one is maybe better, but it is
: more dangerous considering our current deadlines.
: 
: I would highlight that just 'let's use small directly, unlikely it will
: be changed' does not work in the long terms. So the second way is not
: cheap, IMHO. Many things should be considered before we'll able to state
: that, say, ibuf API / ABI is perfectly stable.

I've discussed these things matter with Alexander Lyapunov, and despite 
his plans to introduce here "quite soon" (well in 4-6 month time frame)
newer C++ connector facilities, which would allow to manipulate with
raw buffers (like char*) in a more efficient manner, but he confirms
the fact that he plans to supports binary compatibility in ibuf for 
indefinite time, and it's safe to use it externally today. And using small 
as git submodule is intended scenario.


SashaL, do I put it correctly?

Timur

Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Alexander Turenko]

> > +int
> > +box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple);
> 
> 2. OCD mode on. I would propose either make tuple the first
> argument, or rename it to box_tuple_format_validate_tuple().
> So as to be consistent with our agreement, that if something
> is a method of <type>, then the <type> argument goes first,
> and the method name is <type>_<action>.
> 
> I see we currently have in the public API the functions:
> 
> 	box_tuple_validate - your new function, a bit
> 		inconsistent.
> 
> 	box_tuple_validate_key_parts - this should have been
> 		box_key_def_validate_tuple from the beginning,
> 		but we can't do anything about it now.

We can. It is part of my patchset.

> 
> 	box_key_def_validate_key - correct. Key_def goes first,
> 		and the name is consistent.
> 
> So if you will make box_tuple_validate consistent, we will have
> more correct signatures (2/3) than incorrect, for validation
> methods at least.

So, if we'll apply all your suggestions, the key_def module API will
contain the following functions:

 | Function                     | Consumer        | Name variants (for history)     |
 | ---------------------------- | --------------- | ------------------------------- |
 | box_key_def_new()            | already present |                                 |
 | box_key_part_def_create()    | key_def module  |                                 |
 | box_key_def_new_v2()         | key_def module  | box_key_def_new_ex()            |
 | box_key_def_dump_parts()     | key_def module  |                                 |
 | box_key_def_merge()          | key_def module  |                                 |
 | box_key_def_dup()            | merger module   |                                 |
 | box_key_def_delete()         | already present |                                 |
 | box_key_def_validate_tuple() | key_def module  | box_tuple_validate_key_parts()  |
 | box_tuple_compare()          | already present |                                 |
 | box_tuple_compare_with_key() | already present |                                 |
 | box_key_def_extract_key()    | key_def module  | box_tuple_extract_key_{ex,v2}() |
 | box_key_def_validate_key()   | key_def module  |                                 |

All functions around key_defs and tuples are prefixed with 'box_key_def_',
except box_tuple_compare*(), which are already present.

If we'll follow current internal naming:

 | Function                       | Name variants (may fit better) |
 | ------------------------------ | ------------------------------ |
 | box_key_def_new()              |                                |
 | box_key_part_def_create()      |                                |
 | box_key_def_new_v2()           |                                |
 | box_key_def_dump_parts()       |                                |
 | box_key_def_merge()            |                                |
 | box_key_def_dup()              |                                |
 | box_key_def_delete()           |                                |
 | box_tuple_validate_key_parts() | box_tuple_validate_key()       |
 | box_tuple_compare()            |                                |
 | box_tuple_compare_with_key()   |                                |
 | box_tuple_extract_key_v2()     |                                |
 | box_key_def_validate_key()     | box_validate_key()             |

Here functions that operate on key_def itself are prefixed with
'box_key_def_', but functions that operate on tuples using a key
definition are named 'box_tuple_<action>()' (generally, see below).

The exception is box_key_def_validate_key(), but we can rename it to
box_validate_key(). And also drop '_parts' from
box_tuple_validate_key_parts() (because it meaningless):

 | Function                       |
 | ------------------------------ |
 | box_key_def_new()              |
 | box_key_part_def_create()      |
 | box_key_def_new_v2()           |
 | box_key_def_dump_parts()       |
 | box_key_def_merge()            |
 | box_key_def_dup()              |
 | box_key_def_delete()           |
 | box_tuple_validate_key()       |
 | box_tuple_compare()            |
 | box_tuple_compare_with_key()   |
 | box_tuple_extract_key_v2()     |
 | box_validate_key()             |

Isn't that nice?

Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Vladislav Shpilevoy]

On 09.10.2020 03:11, Alexander Turenko wrote:
>>> +int
>>> +box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple);
>>
>> 2. OCD mode on. I would propose either make tuple the first
>> argument, or rename it to box_tuple_format_validate_tuple().
>> So as to be consistent with our agreement, that if something
>> is a method of <type>, then the <type> argument goes first,
>> and the method name is <type>_<action>.
>>
>> I see we currently have in the public API the functions:
>>
>> 	box_tuple_validate - your new function, a bit
>> 		inconsistent.
>>
>> 	box_tuple_validate_key_parts - this should have been
>> 		box_key_def_validate_tuple from the beginning,
>> 		but we can't do anything about it now.
> 
> We can. It is part of my patchset.

Then lets do it.

>> 	box_key_def_validate_key - correct. Key_def goes first,
>> 		and the name is consistent.
>>
>> So if you will make box_tuple_validate consistent, we will have
>> more correct signatures (2/3) than incorrect, for validation
>> methods at least.
> 
> So, if we'll apply all your suggestions, the key_def module API will
> contain the following functions:
> 
>  | Function                     | Consumer        | Name variants (for history)     |
>  | ---------------------------- | --------------- | ------------------------------- |
>  | box_key_def_new()            | already present |                                 |
>  | box_key_part_def_create()    | key_def module  |                                 |
>  | box_key_def_new_v2()         | key_def module  | box_key_def_new_ex()            |
>  | box_key_def_dump_parts()     | key_def module  |                                 |
>  | box_key_def_merge()          | key_def module  |                                 |
>  | box_key_def_dup()            | merger module   |                                 |
>  | box_key_def_delete()         | already present |                                 |
>  | box_key_def_validate_tuple() | key_def module  | box_tuple_validate_key_parts()  |
>  | box_tuple_compare()          | already present |                                 |
>  | box_tuple_compare_with_key() | already present |                                 |
>  | box_key_def_extract_key()    | key_def module  | box_tuple_extract_key_{ex,v2}() |
>  | box_key_def_validate_key()   | key_def module  |                                 |> 
> 
> All functions around key_defs and tuples are prefixed with 'box_key_def_',
> except box_tuple_compare*(), which are already present.
> 
> If we'll follow current internal naming:
> 
>  | Function                       | Name variants (may fit better) |
>  | ------------------------------ | ------------------------------ |
>  | box_key_def_new()              |                                |
>  | box_key_part_def_create()      |                                |
>  | box_key_def_new_v2()           |                                |
>  | box_key_def_dump_parts()       |                                |
>  | box_key_def_merge()            |                                |
>  | box_key_def_dup()              |                                |
>  | box_key_def_delete()           |                                |
>  | box_tuple_validate_key_parts() | box_tuple_validate_key()       |
>  | box_tuple_compare()            |                                |
>  | box_tuple_compare_with_key()   |                                |
>  | box_tuple_extract_key_v2()     |                                |
>  | box_key_def_validate_key()     | box_validate_key()             |
> 
> Here functions that operate on key_def itself are prefixed with
> 'box_key_def_', but functions that operate on tuples using a key
> definition are named 'box_tuple_<action>()' (generally, see below).

Tuple validation methods operate on key_def in the same extent as
on the tuples.

> The exception is box_key_def_validate_key(), but we can rename it to
> box_validate_key(). And also drop '_parts' from
> box_tuple_validate_key_parts() (because it meaningless):
> 
> 
>  | Function                       |
>  | ------------------------------ |
>  | box_key_def_new()              |
>  | box_key_part_def_create()      |
>  | box_key_def_new_v2()           |
>  | box_key_def_dump_parts()       |
>  | box_key_def_merge()            |
>  | box_key_def_dup()              |
>  | box_key_def_delete()           |
>  | box_tuple_validate_key()       |
>  | box_tuple_compare()            |
>  | box_tuple_compare_with_key()   |
>  | box_tuple_extract_key_v2()     |
>  | box_validate_key()             |
> 
> Isn't that nice?

It is fine. As long as all methods belong to a type and have its
name as a prefix. I don't mind if tuple validation and key extraction
methods will belong to box_tuple except box_key_def.

What looks inconsistent is box_validate_key(). It seems it does not
belong to anything.

If we rename it to box_key_def_validate_key(), we need to rename
box_tuple_validate_key() to box_key_def_validate_tuple() to be
consistent in who validates whom.

If we rename it to box_key_validate(), then it is inconsistent about
not having a 'key' type. And will become wrong if we will ever introduce
a key type.

Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Alexander Turenko]

On Fri, Oct 09, 2020 at 10:11:05PM +0200, Vladislav Shpilevoy wrote:
> On 09.10.2020 03:11, Alexander Turenko wrote:
> >>> +int
> >>> +box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple);
> >>
> >> 2. OCD mode on. I would propose either make tuple the first
> >> argument, or rename it to box_tuple_format_validate_tuple().
> >> So as to be consistent with our agreement, that if something
> >> is a method of <type>, then the <type> argument goes first,
> >> and the method name is <type>_<action>.
> >>
> >> I see we currently have in the public API the functions:
> >>
> >> 	box_tuple_validate - your new function, a bit
> >> 		inconsistent.
> >>
> >> 	box_tuple_validate_key_parts - this should have been
> >> 		box_key_def_validate_tuple from the beginning,
> >> 		but we can't do anything about it now.
> > 
> > We can. It is part of my patchset.
> 
> Then lets do it.

Aye!

> 
> >> 	box_key_def_validate_key - correct. Key_def goes first,
> >> 		and the name is consistent.
> >>
> >> So if you will make box_tuple_validate consistent, we will have
> >> more correct signatures (2/3) than incorrect, for validation
> >> methods at least.
> > 
> > So, if we'll apply all your suggestions, the key_def module API will
> > contain the following functions:
> > 
> >  | Function                     | Consumer        | Name variants (for history)     |
> >  | ---------------------------- | --------------- | ------------------------------- |
> >  | box_key_def_new()            | already present |                                 |
> >  | box_key_part_def_create()    | key_def module  |                                 |
> >  | box_key_def_new_v2()         | key_def module  | box_key_def_new_ex()            |
> >  | box_key_def_dump_parts()     | key_def module  |                                 |
> >  | box_key_def_merge()          | key_def module  |                                 |
> >  | box_key_def_dup()            | merger module   |                                 |
> >  | box_key_def_delete()         | already present |                                 |
> >  | box_key_def_validate_tuple() | key_def module  | box_tuple_validate_key_parts()  |
> >  | box_tuple_compare()          | already present |                                 |
> >  | box_tuple_compare_with_key() | already present |                                 |
> >  | box_key_def_extract_key()    | key_def module  | box_tuple_extract_key_{ex,v2}() |
> >  | box_key_def_validate_key()   | key_def module  |                                 |> 
> > 
> > All functions around key_defs and tuples are prefixed with 'box_key_def_',
> > except box_tuple_compare*(), which are already present.
> > 
> > If we'll follow current internal naming:
> > 
> >  | Function                       | Name variants (may fit better) |
> >  | ------------------------------ | ------------------------------ |
> >  | box_key_def_new()              |                                |
> >  | box_key_part_def_create()      |                                |
> >  | box_key_def_new_v2()           |                                |
> >  | box_key_def_dump_parts()       |                                |
> >  | box_key_def_merge()            |                                |
> >  | box_key_def_dup()              |                                |
> >  | box_key_def_delete()           |                                |
> >  | box_tuple_validate_key_parts() | box_tuple_validate_key()       |
> >  | box_tuple_compare()            |                                |
> >  | box_tuple_compare_with_key()   |                                |
> >  | box_tuple_extract_key_v2()     |                                |
> >  | box_key_def_validate_key()     | box_validate_key()             |
> > 
> > Here functions that operate on key_def itself are prefixed with
> > 'box_key_def_', but functions that operate on tuples using a key
> > definition are named 'box_tuple_<action>()' (generally, see below).
> 
> Tuple validation methods operate on key_def in the same extent as
> on the tuples.

Yea, I just tried to find a more precise pattern in the internal naming
that may be useful for us here.

> 
> > The exception is box_key_def_validate_key(), but we can rename it to
> > box_validate_key(). And also drop '_parts' from
> > box_tuple_validate_key_parts() (because it meaningless):
> > 
> > 
> >  | Function                       |
> >  | ------------------------------ |
> >  | box_key_def_new()              |
> >  | box_key_part_def_create()      |
> >  | box_key_def_new_v2()           |
> >  | box_key_def_dump_parts()       |
> >  | box_key_def_merge()            |
> >  | box_key_def_dup()              |
> >  | box_key_def_delete()           |
> >  | box_tuple_validate_key()       |
> >  | box_tuple_compare()            |
> >  | box_tuple_compare_with_key()   |
> >  | box_tuple_extract_key_v2()     |
> >  | box_validate_key()             |
> > 
> > Isn't that nice?
> 
> It is fine. As long as all methods belong to a type and have its
> name as a prefix. I don't mind if tuple validation and key extraction
> methods will belong to box_tuple except box_key_def.
> 
> What looks inconsistent is box_validate_key(). It seems it does not
> belong to anything.
> 
> If we rename it to box_key_def_validate_key(), we need to rename
> box_tuple_validate_key() to box_key_def_validate_tuple() to be
> consistent in who validates whom.
> 
> If we rename it to box_key_validate(), then it is inconsistent about
> not having a 'key' type. And will become wrong if we will ever introduce
> a key type.

Looks meagingful for me. Since it anyway breaks the attempt to use
'box_tuple_<action>()' naming for keydefish actions on tuples, I would
also choose box_key_def_extract_key() instead of
box_tuple_extract_key_v2().

The result becomes the same as in the first table above :)

 | Function                       |
 | ------------------------------ |
 | box_key_def_new()              |
 | box_key_part_def_create()      |
 | box_key_def_new_v2()           |
 | box_key_def_dump_parts()       |
 | box_key_def_merge()            |
 | box_key_def_dup()              |
 | box_key_def_delete()           |
 | box_key_def_validate_tuple()   |
 | box_tuple_compare()            |
 | box_tuple_compare_with_key()   |
 | box_key_def_extract_key()      |
 | box_key_def_validate_key()     |

All names are prefixed, most with the same prefix (except
box_tuple_compare*()). Okay for me. I'll update my patchset to follow
this agreement.