[Tarantool-patches] [PATCH v2 00/15] RFC: module api: extend for external key_def Lua module

[Tarantool-patches] [PATCH v2 00/15] RFC: module api: extend for external key_def Lua module

[Alexander Turenko]

See cover letter for the v1 patchset ([1]) for the background
information.

The patchset still in progress, shared to collect feedback earlier.
Sorry for that, the deadline is very soon...

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-September/019583.html

Changelog from v1 to v2
-----------------------

- 1.10 patchset: polished description of 'lua: adjust contract of
  luaT_tuple_new()' commit.
- Restructured patches around luaT_tuple_encode() and luaT_tuple_new() in order
  to minimize diffs considering that I decided to fix #5382 ('luaT_tuple_new()
  raises a Lua error that leads to various problems') before exposing those
  functions.
- Fixed #5382: luaT_tuple_new() don't raise an error anymore (same for the
  luaT_tuple_encode()).
- Encode to the box region in luaT_tuple_encode().
- Added the preliminary commit 'lua: factor out tuple encoding from
  luaT_tuple_new' to simplify the changes above a bit.
- Changed prefix from 'refactoring' to 'module api' or 'module api/lua' in
  commits regarding API_EXPORT macro.
- Moved key_def_api.[ch] functions back to key_def.[ch] and group them instead.
- Removed the dangerous advice about box region ('data allocated on this region
  are freed eventually').
- Clarified when box_region_alloc() and box_region_aligned_alloc() should be
  used.
- Removed box_region_alloc_object() and box_region_alloc_array() macros from
  the module API.
- Fixed typos, ordering in src/exports.h, Doxygen comments style and so on.
- Removed MULTIKEY_NONE from the API.
- Dropped 'allow_nullable' parameter from key validation function.
- Added copying of collation name from <struct coll_id> in
  box_key_def_dump_parts().
- Renamed box_key_def_new_ex() to box_key_def_new_v2().
- Fixed OOM and zero parts count handling in box_key_def_new_v2().
- Dropped BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT, cut _MASK from
  BOX_KEY_PART_DEF_IS_NULLABLE_MASK.
- Renamed box_tuple_extract_key_ex() to box_key_def_extract_key().
- Renamed box_tuple_validate_key_parts() to box_key_def_validate_tuple().
- Added a note for box_region_align_alloc() that alignment must be a power of
  2.
- Added a basic box region test.
- Added a luaL_iscdata() test.
- Added a basic luaT_tuple_new() test.
- Clarified box_key_part_def_create() guarantees.
- Added basic tests for box_key_part_def_create() and box_key_def_new_v2().
- Added test for box_key_def_dump_parts().
- Added basic test for test_key_def_validate_tuple().

Plan for v3
-----------

- Write basic test cases.
  - box_key_def_merge()
  - box_key_def_extract_key()
  - box_key_def_validate_key()
- Try to simplify commits that introduces luaT_tuple_encode().
- Add @ChangeLog.
- Polish something like order of functions and so on if time will
  permit.

Looks like I have the evening and the night for that. Challenge
accepted.

Issues
------

https://github.com/tarantool/tarantool/issues/5313 ('module api: module
API requires C11')
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/Totktonada/gh-5273-expand-module-api
(top 15 commits)
https://github.com/tarantool/tarantool/tree/Totktonada/gh-5273-expand-module-api-1.10
(outdated now, will update a bit later)

Alexander Turenko (15):
  module api: get rid of typedef redefinitions
  module api: expose box region
  module api/lua: add luaL_iscdata() function
  lua: factor out tuple encoding from luaT_tuple_new
  lua: don't raise a Lua error from luaT_tuple_new()
  WIP: module api/lua: add luaT_tuple_encode()
  module api/lua: expose luaT_tuple_new()
  module api/lua: add API_EXPORT to tuple functions
  module api: add API_EXPORT to key_def functions
  module api: add box_key_def_new_v2()
  module api: add box_key_def_dump_parts()
  module api: expose box_key_def_validate_tuple()
  WIP: module api: expose box_key_def_merge()
  WIP: module api: expose box_key_def_extract_key()
  WIP: module api: add box_key_def_validate_key()

 src/CMakeLists.txt               |   2 +-
 src/box/index.h                  |   5 +-
 src/box/key_def.c                | 251 +++++++++++
 src/box/key_def.h                | 226 +++++++++-
 src/box/lua/tuple.c              | 204 +++++++--
 src/box/lua/tuple.h              |  46 +-
 src/exports.h                    |  14 +
 src/lib/core/fiber.c             |  24 ++
 src/lib/core/fiber.h             |  76 ++++
 src/lua/utils.c                  |   6 +
 src/lua/utils.h                  |  20 +
 test/app-tap/CMakeLists.txt      |   6 +
 test/app-tap/module_api.c        | 713 +++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |  58 ++-
 test/unit/luaT_tuple_new.c       |  42 +-
 test/unit/luaT_tuple_new.result  |   6 +-
 16 files changed, 1639 insertions(+), 60 deletions(-)

-- 
2.25.0

[Tarantool-patches] [PATCH v2 01/15] module api: get rid of typedef redefinitions

[Alexander Turenko]

Technically C99 forbids it. Clang complains about typedef redefinitions
when C99 is used (it is default prior to clang 3.6):

 | error: redefinition of typedef 'box_tuple_t' is a C11 feature
 |        [-Werror,-Wtypedef-redefinition]
 | error: redefinition of typedef 'box_key_def_t' is a C11 feature
 |        [-Werror,-Wtypedef-redefinition]

The generated module.h file should define a type using typedef once.
This patch moves extra definitions out of public parts of header files.
Reordered api headers to place usages of such types after definitions.

Set C99 for the module API test in order to catch problems of this kind
in a future. Fixed 'unused value' warnings, which appears after the
change (it is strange that -Wall was not passed here before).

Fixes #5313
---
 src/CMakeLists.txt          | 2 +-
 src/box/index.h             | 5 +++--
 src/box/key_def.h           | 3 ++-
 test/app-tap/CMakeLists.txt | 6 ++++++
 test/app-tap/module_api.c   | 2 ++
 5 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 68d69eded..699536652 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -144,10 +144,10 @@ set(api_headers
     ${CMAKE_SOURCE_DIR}/src/lua/error.h
     ${CMAKE_SOURCE_DIR}/src/lua/string.h
     ${CMAKE_SOURCE_DIR}/src/box/txn.h
+    ${CMAKE_SOURCE_DIR}/src/box/tuple.h
     ${CMAKE_SOURCE_DIR}/src/box/key_def.h
     ${CMAKE_SOURCE_DIR}/src/box/lua/key_def.h
     ${CMAKE_SOURCE_DIR}/src/box/field_def.h
-    ${CMAKE_SOURCE_DIR}/src/box/tuple.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple_format.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple_extract_key.h
     ${CMAKE_SOURCE_DIR}/src/box/schema_def.h
diff --git a/src/box/index.h b/src/box/index.h
index 86148023f..6225a8674 100644
--- a/src/box/index.h
+++ b/src/box/index.h
@@ -46,10 +46,11 @@ struct index_def;
 struct key_def;
 struct info_handler;
 
-/** \cond public */
-
 typedef struct tuple box_tuple_t;
 typedef struct key_def box_key_def_t;
+
+/** \cond public */
+
 typedef struct iterator box_iterator_t;
 
 /**
diff --git a/src/box/key_def.h b/src/box/key_def.h
index f4d9e76f2..2826273cc 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -287,10 +287,11 @@ key_def_copy(struct key_def *dest, const struct key_def *src);
 void
 key_def_delete(struct key_def *def);
 
+typedef struct tuple box_tuple_t;
+
 /** \cond public */
 
 typedef struct key_def box_key_def_t;
-typedef struct tuple box_tuple_t;
 
 /**
  * Create key definition with key fields with passed typed on passed positions.
diff --git a/test/app-tap/CMakeLists.txt b/test/app-tap/CMakeLists.txt
index bf7d28136..a41640796 100644
--- a/test/app-tap/CMakeLists.txt
+++ b/test/app-tap/CMakeLists.txt
@@ -1,2 +1,8 @@
 build_module(module_api module_api.c)
 build_module(libyield libyield.c)
+
+# gh-5313: verify that module.h actually conforms to the C99
+# standard.
+set(CMAKE_C_FLAGS "-Wall -Wextra -std=c99")
+set(CMAKE_C_FLAGS_DEBUG "-O0 -Werror")
+set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O2")
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index b81a98056..a79fbed0d 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -198,6 +198,7 @@ test_toint64(lua_State *L)
 
 int fiber_test_func(va_list va)
 {
+	(void) va;
 	do {
 		fiber_set_cancellable(true);
 		fiber_sleep(0.01);
@@ -380,6 +381,7 @@ test_call(lua_State *L)
 static int
 cpcall_handler(lua_State *L)
 {
+	(void) L;
 	return 0;
 }
 
-- 
2.25.0

[Tarantool-patches] [PATCH v2 02/15] module api: expose box region

[Alexander Turenko]

It is the better alternative to linking the small library directly to a
module. Why not just use the small library in a module?

Functions from an executable are preferred over ones that are shipped in
a dynamic library (on Linux, Mac OS differs), while a module may use the
small library directly. It may cause a problem when some functions from
the library are inlined, but some are not, and those different versions
of the library offer structures with different layouts. Small library
symbols may be exported by the tarantool executable after the change of
default symbols visibility (see [1]). See more details and examples in
[2].

So it is better to expose so called box region and get rid of all those
compatibility problems.

[1]: 2.5.0-42-g03790ac55 ('cmake: remove dynamic-list linker option')
[2]: https://lists.tarantool.org/pipermail/tarantool-discussions/2020-September/000095.html

Part of #5273
---
 src/exports.h                    |  4 ++
 src/lib/core/fiber.c             | 24 +++++++++
 src/lib/core/fiber.h             | 76 +++++++++++++++++++++++++++++
 test/app-tap/module_api.c        | 84 ++++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |  2 +-
 5 files changed, 189 insertions(+), 1 deletion(-)

diff --git a/src/exports.h b/src/exports.h
index 6d8303180..7861bb529 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -218,6 +218,10 @@ EXPORT(fiber_is_cancelled)
 EXPORT(fiber_join)
 EXPORT(fiber_new)
 EXPORT(fiber_new_ex)
+EXPORT(box_region_aligned_alloc)
+EXPORT(box_region_alloc)
+EXPORT(box_region_truncate)
+EXPORT(box_region_used)
 EXPORT(fiber_reschedule)
 EXPORT(fiber_self)
 EXPORT(fiber_set_cancellable)
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 223c841df..33ec47c66 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -1368,6 +1368,30 @@ fiber_top_disable(void)
 }
 #endif /* ENABLE_FIBER_TOP */
 
+size_t
+box_region_used(void)
+{
+	return region_used(&fiber()->gc);
+}
+
+void *
+box_region_alloc(size_t size)
+{
+	return region_alloc(&fiber()->gc, size);
+}
+
+void *
+box_region_aligned_alloc(size_t size, size_t alignment)
+{
+	return region_aligned_alloc(&fiber()->gc, size, alignment);
+}
+
+void
+box_region_truncate(size_t size)
+{
+	return region_truncate(&fiber()->gc, size);
+}
+
 void
 cord_create(struct cord *cord, const char *name)
 {
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index 16ee9f414..11ca278e5 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -386,6 +386,82 @@ struct slab_cache;
 API_EXPORT struct slab_cache *
 cord_slab_cache(void);
 
+/**
+ * box region allocator
+ *
+ * It is the region allocator from the small library. It is useful
+ * for allocating tons of small objects and free them at once.
+ *
+ * Typical usage is illustrated in the sketch below.
+ *
+ *  | size_t region_svp = box_region_used();
+ *  | while (<...>) {
+ *  |     char *buf = box_region_alloc(<...>);
+ *  |     <...>
+ *  | }
+ *  | box_region_truncate(region_svp);
+ *
+ * There are module API functions that return a result on
+ * this region. In this case a module is responsible to free the
+ * result:
+ *
+ *  | size_t region_svp = box_region_used();
+ *  | char *buf = box_<...>(<...>);
+ *  | <...>
+ *  | box_region_truncate(region_svp);
+ *
+ * This API provides better compatibility guarantees over using
+ * the small library directly in a module. A binary layout of
+ * internal structures may be changed in a future, but
+ * <box_region_*>() functions will remain API and ABI compatible.
+ *
+ * Data allocated on the region are guaranteed to be valid until
+ * a fiber yield or a call of a function from the certain set:
+ *
+ * - Related to transactions.
+ * - Ones that may cause box initialization (box.cfg()).
+ * - Ones that may involve SQL execution.
+ *
+ * FIXME: Provide more strict list of functions, which may
+ * invalidate the data: ones that may lead to calling of
+ * fiber_gc().
+ *
+ * It is safe to call simple box APIs around tuples, key_defs and
+ * so on -- they don't invalidate the allocated data.
+ *
+ * Don't assume this region as fiber local. This is an
+ * implementation detail and may be changed in a future.
+ */
+
+/** How much memory is used by the box region. */
+API_EXPORT size_t
+box_region_used(void);
+
+/**
+ * Allocate size bytes from the box region.
+ *
+ * Don't use this function to allocate a memory block for a value
+ * or array of values of a type with alignment requirements. A
+ * violation of alignment requrements leads to undefined
+ * behaviour.
+ */
+API_EXPORT void *
+box_region_alloc(size_t size);
+
+/**
+ * Allocate size bytes from the box region with given alignment.
+ *
+ * Alignment must be a power of 2.
+ */
+API_EXPORT void *
+box_region_aligned_alloc(size_t size, size_t alignment);
+
+/**
+ * Truncate the box region to the given size.
+ */
+void
+box_region_truncate(size_t size);
+
 /** \endcond public */
 
 /**
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index a79fbed0d..b5949cde4 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -15,6 +15,10 @@
 #define STR2(x) #x
 #define STR(x) STR2(x)
 
+#ifndef lengthof
+#define lengthof(array) (sizeof (array) / sizeof ((array)[0]))
+#endif
+
 /* Test for constants */
 static const char *consts[] = {
 	PACKAGE_VERSION,
@@ -451,6 +455,85 @@ test_iscallable(lua_State *L)
 	return 1;
 }
 
+/* {{{ test_box_region */
+
+/**
+ * Verify basic usage of box region.
+ */
+static int
+test_box_region(struct lua_State *L)
+{
+	size_t region_svp_0 = box_region_used();
+
+	/* Verify allocation and box_region_used(). */
+	size_t size_arr[] = {1, 7, 19, 10 * 1024 * 1024, 1, 18, 1024};
+	size_t region_svp_arr[lengthof(size_arr)];
+	char *ptr_arr[lengthof(size_arr)];
+	for (size_t i = 0; i < lengthof(size_arr); ++i) {
+		size_t size = size_arr[i];
+		size_t region_svp = box_region_used();
+		char *ptr = box_region_alloc(size);
+
+		/* Verify box_region_used() after allocation. */
+		assert(box_region_used() - region_svp == size);
+
+		/* Verify that data is accessible. */
+		for (char *p = ptr; p < ptr + size; ++p)
+			*p = 'x';
+
+		/*
+		 * Save data pointer and savepoint to verify
+		 * truncation later.
+		 */
+		ptr_arr[i] = ptr;
+		region_svp_arr[i] = region_svp;
+	}
+
+	/* Verify truncation. */
+	for (ssize_t i = lengthof(region_svp_arr) - 1; i >= 0; --i) {
+		box_region_truncate(region_svp_arr[i]);
+		assert(box_region_used() == region_svp_arr[i]);
+
+		/*
+		 * Verify that all data before this savepoint
+		 * still accessible.
+		 */
+		for (ssize_t j = 0; j < i; ++j) {
+			size_t size = size_arr[j];
+			char *ptr = ptr_arr[j];
+			for (char *p = ptr; p < ptr + size; ++p) {
+				assert(*p == 'x' || *p == 'y');
+				*p = 'y';
+			}
+		}
+	}
+	assert(box_region_used() == region_svp_0);
+
+	/* Verify aligned allocation. */
+	size_t a_size_arr[] = {1, 3, 5, 7, 11, 13, 17, 19};
+	size_t alignment_arr[] = {1, 2, 4, 8, 16, 32, 64};
+	for (size_t s = 0; s < lengthof(a_size_arr); ++s) {
+		for (size_t a = 0; a < lengthof(alignment_arr); ++a) {
+			size_t size = a_size_arr[s];
+			size_t alignment = alignment_arr[a];
+			char *ptr = box_region_aligned_alloc(size, alignment);
+			assert((uintptr_t) ptr % alignment == 0);
+
+			/* Data is accessible. */
+			for (char *p = ptr; p < ptr + size; ++p)
+				*p = 'x';
+		}
+	}
+
+	/* Clean up. */
+	box_region_truncate(region_svp_0);
+
+	lua_pushboolean(L, true);
+	return 1;
+}
+
+/* }}} test_box_region */
+
 LUA_API int
 luaopen_module_api(lua_State *L)
 {
@@ -479,6 +562,7 @@ luaopen_module_api(lua_State *L)
 		{"test_state", test_state},
 		{"test_tostring", test_tostring},
 		{"iscallable", test_iscallable},
+		{"test_box_region", test_box_region},
 		{NULL, NULL}
 	};
 	luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index a6658cc61..08e8add35 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -117,7 +117,7 @@ local function test_iscallable(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(24)
+    test:plan(25)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
-- 
2.25.0

[Tarantool-patches] [PATCH v2 03/15] module api/lua: add luaL_iscdata() function

[Alexander Turenko]

It is useful to provide a module specific error when cdata expected, but
a value of another type is passed.

Alternative would be using of lua_type() to check against LUA_TCDATA,
but this constant is not exposed for modules. See more in the
luaL_iscdata() API comment.

Part of #5273
---
 src/exports.h                    |  1 +
 src/lua/utils.c                  |  6 ++++
 src/lua/utils.h                  | 20 +++++++++++
 test/app-tap/module_api.c        | 22 ++++++++++++
 test/app-tap/module_api.test.lua | 58 +++++++++++++++++++++++++++++++-
 5 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/src/exports.h b/src/exports.h
index 7861bb529..01c1aa83e 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -356,6 +356,7 @@ EXPORT(luaL_findtable)
 EXPORT(luaL_getmetafield)
 EXPORT(luaL_gsub)
 EXPORT(luaL_iscallable)
+EXPORT(luaL_iscdata)
 EXPORT(luaL_loadbuffer)
 EXPORT(luaL_loadbufferx)
 EXPORT(luaL_loadfile)
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 399bec6c6..3e1c491f8 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -113,6 +113,12 @@ luaL_pushuuid(struct lua_State *L)
 	return luaL_pushcdata(L, CTID_UUID);
 }
 
+int
+luaL_iscdata(struct lua_State *L, int idx)
+{
+	return lua_type(L, idx) == LUA_TCDATA;
+}
+
 void *
 luaL_checkcdata(struct lua_State *L, int idx, uint32_t *ctypeid)
 {
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 7e02a05f2..e80e2b1a2 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -78,6 +78,26 @@ luaL_pushuuid(struct lua_State *L);
 
 /** \cond public */
 
+/**
+ * Checks whether a value on the Lua stack is a cdata.
+ *
+ * Unlike <luaL_checkcdata>() this function does not raise an
+ * error. It is useful to raise a domain specific error.
+ *
+ * Lua API and module API don't expose LUA_TCDATA constant.
+ * We have no guarantee that this constant will remain the same in
+ * future LuaJIT versions. So this function should be used in
+ * modules instead of `lua_type(L, idx) == LUA_TCDATA`.
+ *
+ * @param L    Lua state.
+ * @param idx  Acceptable index on the Lua stack.
+ *
+ * @retval 1   If the value at the given index is a cdata.
+ * @retval 0   Otherwise.
+ */
+LUA_API int
+luaL_iscdata(struct lua_State *L, int idx);
+
 /**
  * @brief Push cdata of given \a ctypeid onto the stack.
  * CTypeID must be used from FFI at least once. Allocated memory returned
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index b5949cde4..928c3a368 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -455,6 +455,27 @@ test_iscallable(lua_State *L)
 	return 1;
 }
 
+static int
+test_iscdata(struct lua_State *L)
+{
+	assert(lua_gettop(L) == 2);
+
+	int exp = lua_toboolean(L, 2);
+
+	/* Basic test. */
+	int res = luaL_iscdata(L, 1);
+	int ok = res == exp;
+	assert(lua_gettop(L) == 2);
+
+	/* Use negative index. */
+	res = luaL_iscdata(L, -2);
+	ok = ok && res == exp;
+	assert(lua_gettop(L) == 2);
+
+	lua_pushboolean(L, res == exp);
+	return 1;
+}
+
 /* {{{ test_box_region */
 
 /**
@@ -562,6 +583,7 @@ luaopen_module_api(lua_State *L)
 		{"test_state", test_state},
 		{"test_tostring", test_tostring},
 		{"iscallable", test_iscallable},
+		{"iscdata", test_iscdata},
 		{"test_box_region", test_box_region},
 		{NULL, NULL}
 	};
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 08e8add35..582c6d85a 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -116,8 +116,63 @@ local function test_iscallable(test, module)
     end
 end
 
+local function test_iscdata(test, module)
+    local ffi = require('ffi')
+    ffi.cdef([[
+        struct foo { int bar; };
+    ]])
+
+    local cases = {
+        {
+            obj = nil,
+            exp = false,
+            description = 'nil',
+        },
+        {
+            obj = 1,
+            exp = false,
+            description = 'number',
+        },
+        {
+            obj = 'hello',
+            exp = false,
+            description = 'string',
+        },
+        {
+            obj = {},
+            exp = false,
+            description = 'table',
+        },
+        {
+            obj = function() end,
+            exp = false,
+            description = 'function',
+        },
+        {
+            obj = ffi.new('struct foo'),
+            exp = true,
+            description = 'cdata',
+        },
+        {
+            obj = ffi.new('struct foo &'),
+            exp = true,
+            description = 'cdata reference',
+        },
+        {
+            obj = 1LL,
+            exp = true,
+            description = 'cdata number',
+        },
+    }
+
+    test:plan(#cases)
+    for _, case in ipairs(cases) do
+        test:ok(module.iscdata(case.obj, case.exp), case.description)
+    end
+end
+
 local test = require('tap').test("module_api", function(test)
-    test:plan(25)
+    test:plan(26)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
@@ -143,6 +198,7 @@ local test = require('tap').test("module_api", function(test)
 
     test:test("pushcdata", test_pushcdata, module)
     test:test("iscallable", test_iscallable, module)
+    test:test("iscdata", test_iscdata, module)
 
     space:drop()
 end)
-- 
2.25.0

[Tarantool-patches] [PATCH v2 04/15] lua: factor out tuple encoding from luaT_tuple_new

[Alexander Turenko]

It simplifies further changes around encoding a tuple: next commits will
get rid of throwing serialization errors and will expose a function to
encode a table (or tuple) from the Lua stack on the box region to the
module API.

Part of #5273
Part of #5382
---
 src/box/lua/tuple.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 03b4b8a2e..ed97c85e4 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -98,8 +98,9 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-struct tuple *
-luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
+static char *
+luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
+			      size_t *tuple_len_ptr)
 {
 	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
 		diag_set(IllegalParams, "A tuple or a table expected, got %s",
@@ -127,8 +128,20 @@ luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
 		luamp_encode_tuple(L, &tuple_serializer, &stream, idx);
 	}
 	mpstream_flush(&stream);
-	struct tuple *tuple = box_tuple_new(format, buf->buf,
-					    buf->buf + ibuf_used(buf));
+	if (tuple_len_ptr != NULL)
+		*tuple_len_ptr = ibuf_used(buf);
+	return buf->buf;
+}
+
+struct tuple *
+luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
+{
+	size_t tuple_len;
+	char *tuple_data = luaT_tuple_encode_on_lua_ibuf(L, idx, &tuple_len);
+	if (tuple_data == NULL)
+		return NULL;
+	struct tuple *tuple = box_tuple_new(format, tuple_data,
+					    tuple_data + tuple_len);
 	if (tuple == NULL)
 		return NULL;
 	ibuf_reinit(tarantool_lua_ibuf);
-- 
2.25.0

[Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Alexander Turenko]

This change fixes incorrect behaviour at tuple serialization error in
several places: <space_object>:frommap(), <key_def_object>:compare(),
<merge_source>:select(). See more in #5382.

Disallow creating a tuple from objects on the Lua stack (idx == 0) in
luaT_tuple_new() for simplicity. There are no such usages in tarantool.
The function is not exposed yet to the module API. This is only
necessary in box.tuple.new(), which anyway raises Lua errors by its
contract.

The better way to implement it would be rewritting of serialization from
Lua to msgpack without raising Lua errors, but it is labirous work. Some
day, I hope, I'll return here.

Part of #5273
Fixes #5382
---
 src/box/lua/tuple.c             | 118 ++++++++++++++++++++++++--------
 src/box/lua/tuple.h             |   7 +-
 test/unit/luaT_tuple_new.c      |  42 +++++++++---
 test/unit/luaT_tuple_new.result |   6 +-
 4 files changed, 127 insertions(+), 46 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index ed97c85e4..18cfef979 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */
 
 uint32_t CTID_STRUCT_TUPLE_REF;
 
+int luaT_tuple_encode_table_ref = LUA_NOREF;
+
 box_tuple_t *
 luaT_checktuple(struct lua_State *L, int idx)
 {
@@ -98,39 +100,79 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
+/**
+ * Encode a Lua values on a Lua stack as an MsgPack array.
+ *
+ * Raise a Lua error when encoding fails.
+ */
+static int
+luaT_tuple_encode_values(struct lua_State *L)
+{
+	struct ibuf *buf = tarantool_lua_ibuf;
+	ibuf_reset(buf);
+	struct mpstream stream;
+	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
+		      L);
+	int argc = lua_gettop(L);
+	mpstream_encode_array(&stream, argc);
+	for (int k = 1; k <= argc; ++k) {
+		luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
+	}
+	mpstream_flush(&stream);
+	return 0;
+}
+
+/**
+ * Encode a Lua table or a tuple as MsgPack.
+ *
+ * Raise a Lua error when encoding fails.
+ *
+ * It is a kind of critical section to be run under luaT_call().
+ */
+static int
+luaT_tuple_encode_table(struct lua_State *L)
+{
+	struct ibuf *buf = tarantool_lua_ibuf;
+	ibuf_reset(buf);
+	struct mpstream stream;
+	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
+		      L);
+	luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
+	mpstream_flush(&stream);
+	return 0;
+}
+
 static char *
 luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
 			      size_t *tuple_len_ptr)
 {
-	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
+	assert(idx != 0);
+	if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
 		diag_set(IllegalParams, "A tuple or a table expected, got %s",
 			 lua_typename(L, lua_type(L, idx)));
 		return NULL;
 	}
 
-	struct ibuf *buf = tarantool_lua_ibuf;
-	ibuf_reset(buf);
-	struct mpstream stream;
-	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
-		      luamp_error, L);
-	if (idx == 0) {
-		/*
-		 * Create the tuple from lua stack
-		 * objects.
-		 */
-		int argc = lua_gettop(L);
-		mpstream_encode_array(&stream, argc);
-		for (int k = 1; k <= argc; ++k) {
-			luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
-		}
-	} else {
-		/* Create the tuple from a Lua table. */
-		luamp_encode_tuple(L, &tuple_serializer, &stream, idx);
-	}
-	mpstream_flush(&stream);
+	int top = lua_gettop(L);
+
+	/* Calculate absolute value in the stack. */
+	if (idx < 0)
+		idx = top + idx + 1;
+
+	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
+	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
+	assert(lua_isfunction(L, -1));
+
+	lua_pushvalue(L, idx);
+
+	int rc = luaT_call(L, 1, 0);
+	lua_settop(L, top);
+	if (rc != 0)
+		return NULL;
+
 	if (tuple_len_ptr != NULL)
-		*tuple_len_ptr = ibuf_used(buf);
-	return buf->buf;
+		*tuple_len_ptr = ibuf_used(tarantool_lua_ibuf);
+	return tarantool_lua_ibuf->buf;
 }
 
 struct tuple *
@@ -156,15 +198,30 @@ lbox_tuple_new(lua_State *L)
 		lua_newtable(L); /* create an empty tuple */
 		++argc;
 	}
+
+	box_tuple_format_t *fmt = box_tuple_format_default();
+
 	/*
 	 * Use backward-compatible parameters format:
-	 * box.tuple.new(1, 2, 3) (idx == 0), or the new one:
-	 * box.tuple.new({1, 2, 3}) (idx == 1).
+	 * box.tuple.new(1, 2, 3).
 	 */
-	int idx = argc == 1 && (lua_istable(L, 1) ||
-		luaT_istuple(L, 1));
-	box_tuple_format_t *fmt = box_tuple_format_default();
-	struct tuple *tuple = luaT_tuple_new(L, idx, fmt);
+	if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
+		struct ibuf *buf = tarantool_lua_ibuf;
+		luaT_tuple_encode_values(L); /* may raise */
+		struct tuple *tuple = box_tuple_new(fmt, buf->buf,
+						    buf->buf + ibuf_used(buf));
+		ibuf_reinit(buf);
+		if (tuple == NULL)
+			return luaT_error(L);
+		luaT_pushtuple(L, tuple);
+		return 1;
+	}
+
+	/*
+	 * Use the new parameters format:
+	 * box.tuple.new({1, 2, 3}).
+	 */
+	struct tuple *tuple = luaT_tuple_new(L, 1, fmt);
 	if (tuple == NULL)
 		return luaT_error(L);
 	/* box_tuple_new() doesn't leak on exception, see public API doc */
@@ -593,4 +650,7 @@ box_lua_tuple_init(struct lua_State *L)
 	(void) rc;
 	CTID_STRUCT_TUPLE_REF = luaL_ctypeid(L, "struct tuple &");
 	assert(CTID_STRUCT_TUPLE_REF != 0);
+
+	lua_pushcfunction(L, luaT_tuple_encode_table);
+	luaT_tuple_encode_table_ref = luaL_ref(L, LUA_REGISTRYINDEX);
 }
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index fd280f565..6787d1afe 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -82,11 +82,8 @@ luaT_istuple(struct lua_State *L, int idx);
 /** \endcond public */
 
 /**
- * Create a new tuple with specific format from a Lua table, a
- * tuple, or objects on the lua stack.
- *
- * Set idx to zero to create the new tuple from objects on the lua
- * stack.
+ * Create a new tuple with specific format from a Lua table or a
+ * tuple.
  *
  * In case of an error set a diag and return NULL.
  */
diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c
index 8f25c8e07..09a81cabe 100644
--- a/test/unit/luaT_tuple_new.c
+++ b/test/unit/luaT_tuple_new.c
@@ -51,20 +51,20 @@ check_tuple(struct tuple *tuple, box_tuple_format_t *format,
 
 void
 check_error(struct lua_State *L, struct tuple *tuple, int retvals,
+	    const struct type_info *error_type, const char *exp_err,
 	    const char *case_name)
 {
-	const char *exp_err = "A tuple or a table expected, got number";
 	is(tuple, NULL, "%s: tuple == NULL", case_name);
 	is(retvals, 0, "%s: check retvals count", case_name);
 	struct error *e = diag_last_error(diag_get());
-	is(e->type, &type_IllegalParams, "%s: check error type", case_name);
+	is(e->type, error_type, "%s: check error type", case_name);
 	ok(!strcmp(e->errmsg, exp_err), "%s: check error message", case_name);
 }
 
 int
 test_basic(struct lua_State *L)
 {
-	plan(19);
+	plan(23);
 	header();
 
 	int top;
@@ -106,14 +106,12 @@ test_basic(struct lua_State *L)
 	assert(lua_gettop(L) == 0);
 
 	/*
-	 * Case: elements on the stack (idx == 0) as an input and
-	 * a non-default format.
+	 * Case: a non-default format (a Lua table on idx == -1).
 	 */
 
 	/* Prepare the Lua stack. */
-	lua_pushinteger(L, 1);
-	lua_pushinteger(L, 2);
-	lua_pushinteger(L, 3);
+	luaL_loadstring(L, "return {1, 2, 3}");
+	lua_call(L, 0, 1);
 
 	/* Create a new format. */
 	struct key_part_def part;
@@ -130,12 +128,12 @@ test_basic(struct lua_State *L)
 
 	/* Create and check a tuple. */
 	top = lua_gettop(L);
-	tuple = luaT_tuple_new(L, 0, another_format);
+	tuple = luaT_tuple_new(L, -1, another_format);
 	check_tuple(tuple, another_format, lua_gettop(L) - top, "objects");
 
 	/* Clean up. */
 	tuple_format_delete(another_format);
-	lua_pop(L, 3);
+	lua_pop(L, 1);
 	assert(lua_gettop(L) == 0);
 
 	/*
@@ -148,7 +146,28 @@ test_basic(struct lua_State *L)
 	/* Try to create and check for the error. */
 	top = lua_gettop(L);
 	tuple = luaT_tuple_new(L, -1, default_format);
-	check_error(L, tuple, lua_gettop(L) - top, "unexpected type");
+	check_error(L, tuple, lua_gettop(L) - top, &type_IllegalParams,
+		    "A tuple or a table expected, got number",
+		    "unexpected type");
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: unserializable item within a Lua table.
+	 *
+	 * The function should not raise a Lua error.
+	 */
+	luaL_loadstring(L, "return {function() end}");
+	lua_call(L, 0, 1);
+
+	/* Try to create and check for the error. */
+	top = lua_gettop(L);
+	tuple = luaT_tuple_new(L, -1, default_format);
+	check_error(L, tuple, lua_gettop(L) - top, &type_LuajitError,
+		    "unsupported Lua type 'function'",
+		    "unserializable element");
 
 	/* Clean up. */
 	lua_pop(L, 1);
@@ -170,6 +189,7 @@ main()
 	luaL_openlibs(L);
 
 	box_init();
+	tarantool_lua_error_init(L);
 	luaopen_msgpack(L);
 	box_lua_tuple_init(L);
 	lua_pop(L, 1);
diff --git a/test/unit/luaT_tuple_new.result b/test/unit/luaT_tuple_new.result
index 110aa68c2..8f3407130 100644
--- a/test/unit/luaT_tuple_new.result
+++ b/test/unit/luaT_tuple_new.result
@@ -1,4 +1,4 @@
-1..19
+1..23
 	*** test_basic ***
 ok 1 - table: tuple != NULL
 ok 2 - table: check tuple format id
@@ -19,4 +19,8 @@ ok 16 - unexpected type: tuple == NULL
 ok 17 - unexpected type: check retvals count
 ok 18 - unexpected type: check error type
 ok 19 - unexpected type: check error message
+ok 20 - unserializable element: tuple == NULL
+ok 21 - unserializable element: check retvals count
+ok 22 - unserializable element: check error type
+ok 23 - unserializable element: check error message
 	*** test_basic: done ***
-- 
2.25.0

[Tarantool-patches] [PATCH v2 06/15] WIP: module api/lua: add luaT_tuple_encode()

[Alexander Turenko]

It is the same as luaT_tuple_new(), but returns raw MsgPack data (not
<box_tuple_t>) allocated on the box region.

The reason to expose this function is to provide ability to use
box_tuple_compare_with_key() function from an external module for a key
passed as a Lua table. The compare function has the following signature:

 | API_EXPORT int
 | box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
 |                            box_key_def_t *key_def);

The second parameter is a key encoded as an MsgPack array, not a tuple
structure. So luaT_tuple_new() is not applicable here (it is not
worthful to create a tuple structure if we need just MsgPack data).

Some complexity was introduced to support encoding on the Lua shared
buffer and the box region both. The motivation is the following:

- luaT_tuple_encode() is exposed with encoding to the box region,
  because it is more usual to the module API. In particular a user of
  the API able to control when the tuple data should be released.
- Encoding to the Lua shared buffer is kept internally, because there is
  no strong reason to change it to the box region for box.tuple.new().

Part of #5273

XXX: Try to get rid of luaT_mpstream_init_*() functions: call
mpstream_init() outside of the protected section with changed error
handler.
---
 src/box/lua/tuple.c              |  99 ++++++++++++++++++++++----
 src/box/lua/tuple.h              |  17 +++++
 src/exports.h                    |   1 +
 test/app-tap/module_api.c        | 115 +++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |   2 +-
 5 files changed, 220 insertions(+), 14 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 18cfef979..8e2255a2b 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -100,10 +100,31 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
+/* {{{ Encode a Lua table as an MsgPack array */
+
+/*
+ * A lot of functions are there, however the task per se looks
+ * simple. Reasons are the following.
+ *
+ * 1. box.tuple.new() supports two parameters conventions.
+ *    <luaT_tuple_encode_values>() implements the old API.
+ * 2. Serializer from Lua to MsgPack may raise a Lua error,
+ *    so it should be run under pcall. The dangerous code is
+ *    encapsulated into <luaT_tuple_encode_table>().
+ * 3. In particular <mpstream_init>() may raise an error in case
+ *    of OOM, so it also is run under pcall.
+ * 4. box.tuple.new() and <luaT_tuple_new>() use shared Lua ibuf
+ *    under the hood (because there is no strong reason to change
+ *    it), while <luaT_tuple_encode>() uses the box region
+ *    (because it is usual for the module API).
+ */
+
 /**
  * Encode a Lua values on a Lua stack as an MsgPack array.
  *
  * Raise a Lua error when encoding fails.
+ *
+ * Helper for <lbox_tuple_new>().
  */
 static int
 luaT_tuple_encode_values(struct lua_State *L)
@@ -122,6 +143,22 @@ luaT_tuple_encode_values(struct lua_State *L)
 	return 0;
 }
 
+typedef void luaT_mpstream_init_f(struct mpstream *stream, struct lua_State *L);
+
+static void
+luaT_mpstream_init_lua_ibuf(struct mpstream *stream, struct lua_State *L)
+{
+	mpstream_init(stream, tarantool_lua_ibuf, ibuf_reserve_cb,
+		      ibuf_alloc_cb, luamp_error, L);
+}
+
+static void
+luaT_mpstream_init_box_region(struct mpstream *stream, struct lua_State *L)
+{
+	mpstream_init(stream, &fiber()->gc, region_reserve_cb, region_alloc_cb,
+		      luamp_error, L);
+}
+
 /**
  * Encode a Lua table or a tuple as MsgPack.
  *
@@ -132,25 +169,26 @@ luaT_tuple_encode_values(struct lua_State *L)
 static int
 luaT_tuple_encode_table(struct lua_State *L)
 {
-	struct ibuf *buf = tarantool_lua_ibuf;
-	ibuf_reset(buf);
 	struct mpstream stream;
-	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
-		      L);
-	luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
+	luaT_mpstream_init_f *luaT_mpstream_init_f = lua_topointer(L, 1);
+	luaT_mpstream_init_f(&stream, L);
+	luamp_encode_tuple(L, &tuple_serializer, &stream, 2);
 	mpstream_flush(&stream);
 	return 0;
 }
 
-static char *
-luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
-			      size_t *tuple_len_ptr)
+/**
+ * Encode a Lua table / tuple to given mpstream.
+ */
+static int
+luaT_tuple_encode_helper(struct lua_State *L, int idx,
+			 luaT_mpstream_init_f *luaT_mpstream_init_f)
 {
 	assert(idx != 0);
 	if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
 		diag_set(IllegalParams, "A tuple or a table expected, got %s",
 			 lua_typename(L, lua_type(L, idx)));
-		return NULL;
+		return -1;
 	}
 
 	int top = lua_gettop(L);
@@ -163,18 +201,53 @@ luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
 	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
 	assert(lua_isfunction(L, -1));
 
+	lua_pushlightuserdata(L, luaT_mpstream_init_f);
 	lua_pushvalue(L, idx);
 
-	int rc = luaT_call(L, 1, 0);
+	int rc = luaT_call(L, 2, 0);
 	lua_settop(L, top);
-	if (rc != 0)
+	return rc == 0 ? 0 : -1;
+}
+
+/**
+ * Encode a Lua table / tuple to Lua shared ibuf.
+ */
+static char *
+luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
+			      size_t *tuple_len_ptr)
+{
+	struct ibuf *buf = tarantool_lua_ibuf;
+	ibuf_reset(buf);
+	if (luaT_tuple_encode_helper(L, idx, luaT_mpstream_init_lua_ibuf) != 0)
 		return NULL;
+	if (tuple_len_ptr != NULL)
+		*tuple_len_ptr = ibuf_used(buf);
+	return buf->buf;
+}
 
+/**
+ * Encode a Lua table / tuple to box region.
+ */
+char *
+luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr)
+{
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	if (luaT_tuple_encode_helper(L, idx, luaT_mpstream_init_box_region) != 0)
+		return NULL;
+	size_t tuple_len = region_used(region) - region_svp;
 	if (tuple_len_ptr != NULL)
-		*tuple_len_ptr = ibuf_used(tarantool_lua_ibuf);
-	return tarantool_lua_ibuf->buf;
+		*tuple_len_ptr = tuple_len;
+	char *tuple_data = region_join(region, tuple_len);
+	if (tuple_data == NULL) {
+		diag_set(OutOfMemory, tuple_len, "region", "tuple data");
+		return NULL;
+	}
+	return tuple_data;
 }
 
+/* }}} Encode a Lua table as an MsgPack array */
+
 struct tuple *
 luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
 {
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index 6787d1afe..aadcf7f59 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -31,6 +31,7 @@
  * SUCH DAMAGE.
  */
 #include <stddef.h>
+#include "trivia/util.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -79,6 +80,22 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
 box_tuple_t *
 luaT_istuple(struct lua_State *L, int idx);
 
+/**
+ * Encode a table or a tuple on the Lua stack as an MsgPack array.
+ *
+ * @param L              Lua state.
+ * @param idx            Acceptable index on the Lua stack.
+ * @param tuple_len_ptr  Where to store tuple data size in bytes
+ *                       (or NULL).
+ *
+ * The storage for data is allocated on the box region. A caller
+ * should call <box_region_truncate>() to release the data.
+ *
+ * In case of an error set a diag and return NULL.
+ */
+API_EXPORT char *
+luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr);
+
 /** \endcond public */
 
 /**
diff --git a/src/exports.h b/src/exports.h
index 01c1aa83e..7bdba5693 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -406,6 +406,7 @@ EXPORT(luaT_istuple)
 EXPORT(luaT_pushtuple)
 EXPORT(luaT_state)
 EXPORT(luaT_tolstring)
+EXPORT(luaT_tuple_encode)
 EXPORT(mp_char2escape)
 EXPORT(mp_decode_double)
 EXPORT(mp_decode_extl)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 928c3a368..9b182144a 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -555,6 +555,120 @@ test_box_region(struct lua_State *L)
 
 /* }}} test_box_region */
 
+/* {{{ test_tuple_encode */
+
+static void
+check_tuple_data(char *tuple_data, size_t tuple_size, int retvals)
+{
+	assert(tuple_size == 4);
+	assert(tuple_data != NULL);
+	assert(!strncmp(tuple_data, "\x93\x01\x02\x03", 4));
+	assert(retvals == 0);
+}
+
+static void
+check_encode_error(char *tuple_data, int retvals, const char *exp_err_type,
+		   const char *exp_err_msg)
+{
+	assert(tuple_data == NULL);
+	box_error_t *e = box_error_last();
+	assert(!strcmp(box_error_type(e), exp_err_type));
+	assert(!strcmp(box_error_message(e), exp_err_msg));
+	assert(retvals == 0);
+}
+
+/**
+ * Encode a Lua table or a tuple into a tuple.
+ *
+ * Similar to <luaT_tuple_new>() unit test.
+ */
+static int
+test_tuple_encode(struct lua_State *L)
+{
+	int top;
+	char *tuple_data;
+	size_t tuple_size;
+
+	size_t region_svp = box_region_used();
+
+	/*
+	 * Case: a Lua table on idx == -2 as an input.
+	 */
+
+	/* Prepare the Lua stack. */
+	luaL_loadstring(L, "return {1, 2, 3}");
+	lua_call(L, 0, 1);
+	lua_pushnil(L);
+
+	/* Create and check a tuple. */
+	top = lua_gettop(L);
+	tuple_data = luaT_tuple_encode(L, -2, &tuple_size);
+	check_tuple_data(tuple_data, tuple_size, lua_gettop(L) - top);
+
+	/* Clean up. */
+	lua_pop(L, 2);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: a tuple on idx == -1 as an input.
+	 */
+
+	/* Prepare the Lua stack. */
+	luaL_loadstring(L, "return box.tuple.new({1, 2, 3})");
+	lua_call(L, 0, 1);
+
+	/* Create and check a tuple. */
+	top = lua_gettop(L);
+	tuple_data = luaT_tuple_encode(L, -1, &tuple_size);
+	check_tuple_data(tuple_data, tuple_size, lua_gettop(L) - top);
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: a Lua object of an unexpected type.
+	 */
+
+	/* Prepare the Lua stack. */
+	lua_pushinteger(L, 42);
+
+	/* Try to encode and check for the error. */
+	top = lua_gettop(L);
+	tuple_data = luaT_tuple_encode(L, -1, &tuple_size);
+	check_encode_error(tuple_data, lua_gettop(L) - top, "IllegalParams",
+			   "A tuple or a table expected, got number");
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: unserializable item within a Lua table.
+	 *
+	 * The function should not raise a Lua error.
+	 */
+	luaL_loadstring(L, "return {function() end}");
+	lua_call(L, 0, 1);
+
+	/* Try to encode and check for the error. */
+	top = lua_gettop(L);
+	tuple_data = luaT_tuple_encode(L, -1, &tuple_size);
+	check_encode_error(tuple_data, lua_gettop(L) - top, "LuajitError",
+			   "unsupported Lua type 'function'");
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	box_region_truncate(region_svp);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
+/* }}} test_tuple_encode */
+
 LUA_API int
 luaopen_module_api(lua_State *L)
 {
@@ -585,6 +699,7 @@ luaopen_module_api(lua_State *L)
 		{"iscallable", test_iscallable},
 		{"iscdata", test_iscdata},
 		{"test_box_region", test_box_region},
+		{"test_tuple_encode", test_tuple_encode},
 		{NULL, NULL}
 	};
 	luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 582c6d85a..262e0751c 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -172,7 +172,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(26)
+    test:plan(27)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
-- 
2.25.0

[Tarantool-patches] [PATCH v2 07/15] module api/lua: expose luaT_tuple_new()

[Alexander Turenko]

It is convenient wrapper around box_tuple_new() to create a tuple from a
Lua table (or from another tuple).

Part of #5273
---
 src/box/lua/tuple.c              |  6 ++---
 src/box/lua/tuple.h              | 20 +++++++++++++---
 src/exports.h                    |  1 +
 test/app-tap/module_api.c        | 40 ++++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |  2 +-
 5 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 8e2255a2b..9e38d895d 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -248,15 +248,15 @@ luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr)
 
 /* }}} Encode a Lua table as an MsgPack array */
 
-struct tuple *
+box_tuple_t *
 luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
 {
 	size_t tuple_len;
 	char *tuple_data = luaT_tuple_encode_on_lua_ibuf(L, idx, &tuple_len);
 	if (tuple_data == NULL)
 		return NULL;
-	struct tuple *tuple = box_tuple_new(format, tuple_data,
-					    tuple_data + tuple_len);
+	box_tuple_t *tuple = box_tuple_new(format, tuple_data,
+					   tuple_data + tuple_len);
 	if (tuple == NULL)
 		return NULL;
 	ibuf_reinit(tarantool_lua_ibuf);
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index aadcf7f59..0c7e8a16f 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -92,21 +92,35 @@ luaT_istuple(struct lua_State *L, int idx);
  * should call <box_region_truncate>() to release the data.
  *
  * In case of an error set a diag and return NULL.
+ *
+ * @sa luaT_tuple_new()
  */
 API_EXPORT char *
 luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr);
 
-/** \endcond public */
-
 /**
  * Create a new tuple with specific format from a Lua table or a
  * tuple.
  *
+ * The new tuple is referenced in the same way as one created by
+ * <box_tuple_new>(). There are two possible usage scenarious:
+ *
+ * 1. A short living tuple may not be referenced explicitly and
+ *    will be collected automatically at the next module API call
+ *    that yields or returns a tuple.
+ * 2. A long living tuple must be referenced using
+ *    <box_tuple_ref>() and unreferenced than with
+ *    <box_tuple_unref>().
+ *
+ * @sa box_tuple_ref()
+ *
  * In case of an error set a diag and return NULL.
  */
-struct tuple *
+API_EXPORT box_tuple_t *
 luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format);
 
+/** \endcond public */
+
 static inline int
 luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
 {
diff --git a/src/exports.h b/src/exports.h
index 7bdba5693..299bb9fc1 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -407,6 +407,7 @@ EXPORT(luaT_pushtuple)
 EXPORT(luaT_state)
 EXPORT(luaT_tolstring)
 EXPORT(luaT_tuple_encode)
+EXPORT(luaT_tuple_new)
 EXPORT(mp_char2escape)
 EXPORT(mp_decode_double)
 EXPORT(mp_decode_extl)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 9b182144a..184fdf8f8 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -669,6 +669,45 @@ test_tuple_encode(struct lua_State *L)
 
 /* }}} test_tuple_encode */
 
+/* {{{ test_tuple_new */
+
+/**
+ * Create a tuple from a Lua table or another tuple.
+ *
+ * Just basic test. More cases in the luaT_tuple_new.c unit test.
+ */
+static int
+test_tuple_new(struct lua_State *L)
+{
+	box_tuple_format_t *default_format = box_tuple_format_default();
+
+	/* Prepare the Lua stack. */
+	luaL_loadstring(L, "return {1, 2, 3}");
+	lua_call(L, 0, 1);
+
+	/* Create a tuple. */
+	int top = lua_gettop(L);
+	box_tuple_t *tuple = luaT_tuple_new(L, -1, default_format);
+
+	/* Verify size, data and Lua stack top. */
+	size_t region_svp = box_region_used();
+	size_t tuple_size = box_tuple_bsize(tuple);
+	char *tuple_data = box_region_alloc(tuple_size);
+	ssize_t rc = box_tuple_to_buf(tuple, tuple_data, tuple_size);
+	assert(rc == (ssize_t) tuple_size);
+	check_tuple_data(tuple_data, tuple_size, lua_gettop(L) - top);
+
+	/* Clean up. */
+	box_region_truncate(region_svp);
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
+/* }}} test_tuple_new */
+
 LUA_API int
 luaopen_module_api(lua_State *L)
 {
@@ -700,6 +739,7 @@ luaopen_module_api(lua_State *L)
 		{"iscdata", test_iscdata},
 		{"test_box_region", test_box_region},
 		{"test_tuple_encode", test_tuple_encode},
+		{"test_tuple_new", test_tuple_new},
 		{NULL, NULL}
 	};
 	luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 262e0751c..0e7ce37db 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -172,7 +172,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(27)
+    test:plan(28)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
-- 
2.25.0

[Tarantool-patches] [PATCH v2 08/15] module api/lua: add API_EXPORT to tuple functions

[Alexander Turenko]

The reason is unification of declarations. It is the rule of thumb to
use API_EXPORT with module API functions.

Part of #5273
---
 src/box/lua/tuple.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index 0c7e8a16f..fdaf0f8fd 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -57,7 +57,7 @@ typedef struct tuple_format box_tuple_format_t;
  * @retval non-NULL argument is tuple
  * @throws error if the argument is not a tuple.
  */
-box_tuple_t *
+API_EXPORT box_tuple_t *
 luaT_checktuple(struct lua_State *L, int idx);
 
 /**
@@ -66,7 +66,7 @@ luaT_checktuple(struct lua_State *L, int idx);
  * @sa luaT_istuple
  * @throws on OOM
  */
-void
+API_EXPORT void
 luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
 
 /**
@@ -77,7 +77,7 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
  * @retval non-NULL argument is tuple
  * @retval NULL argument is not tuple
  */
-box_tuple_t *
+API_EXPORT box_tuple_t *
 luaT_istuple(struct lua_State *L, int idx);
 
 /**
-- 
2.25.0

[Tarantool-patches] [PATCH v2 09/15] module api: add API_EXPORT to key_def functions

[Alexander Turenko]

It is the rule of thumb to use API_EXPORT with module API functions.

To be honest, I don't see strict reason to use this macro except for
unification with other module API functions.

It adds 'extern', which is default for functions.

It adds __attribute__((visibility("default"))), but symbols to be
exported are listed in extra/exports or src/export.h (depending of
tarantool version, see [1]).

It adds __attribute__((nothrow)), which maybe allows a compiler to
produce more optimized code and also catch an obvious problem and emit a
warning. I don't know.

So the reason for me is unification.

Part of #5273

[1]: 2.5.0-42-g03790ac55 ('cmake: remove dynamic-list linker option')
[2]: 1.6.8-71-g55605c5c9 ('Add __attribute__((nothrow)) to API_EXPORT macro')
---
 src/box/key_def.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/box/key_def.h b/src/box/key_def.h
index 2826273cc..625bb6fea 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -302,7 +302,7 @@ typedef struct key_def box_key_def_t;
  * \param part_count the number of key fields
  * \returns a new key definition object
  */
-box_key_def_t *
+API_EXPORT box_key_def_t *
 box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
 
 /**
@@ -310,7 +310,7 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
  *
  * \param key_def key definition to delete
  */
-void
+API_EXPORT void
 box_key_def_delete(box_key_def_t *key_def);
 
 /**
@@ -322,7 +322,7 @@ box_key_def_delete(box_key_def_t *key_def);
  * @retval <0 if key_fields(tuple_a) < key_fields(tuple_b)
  * @retval >0 if key_fields(tuple_a) > key_fields(tuple_b)
  */
-int
+API_EXPORT int
 box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
 		  box_key_def_t *key_def);
 
@@ -337,7 +337,7 @@ box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
  * @retval >0 if key_fields(tuple) > parts(key)
  */
 
-int
+API_EXPORT int
 box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
 			   box_key_def_t *key_def);
 
-- 
2.25.0

[Tarantool-patches] [PATCH v2 10/15] module api: add box_key_def_new_v2()

[Alexander Turenko]

Unlike box_key_def_new() it allows to set nullability, collation and
JSON path.

Note: JSON paths are not supported in the backported version of the
patch for 1.10.

Provided public non-opaque key part definition structure to create a key
definition. The next commit will also use this structure to dump a key
definition.

There are several technical points around the box_key_part_def_t
structure. They are mainly about providing stable ABI.

- Two uint32_t fields are placed first for better aligning of next
  fields (pointers, which are usually 64 bit wide).

- A padding is used to guarantee that the structure size will remain the
  same across tarantool versions. It allows to allocate an array of such
  structures.

- The padding array is not a field of the structure itself, but added as
  a union variant (see the code). It allows to get rid of manual
  calculation of cumulative fields size, which is hard to do in a
  platform independent way.

- A minimal size of the structure is guaranteed by the union with
  padding, but a static assert is required to guarantee that the size
  will not overrun the predefined value.

- PACKED is added as an extra remedy to make the structure layout
  predictable (on given target architecture).

- A bit flag is used for is_nullable. bool is considered as too
  expensive (it requires 8 bits). bitfields (int:1 and so on) do no
  guarantee certain data layout (it is compiler implementation detail),
  while a module is compiled outside of tarantool build and may use
  different toolchain. A bit flag is the only choice.

- A collation is identified using a string. Different IDs may be used on
  different tarantool instances for collations. The only 'real'
  identifier is a collation name, so using it as identifier in the API
  should be more convenient and less error-prone.

- A field type is also identified using a string instead of a number. We
  have <enum field_type> in the module API, but it cannot be used here,
  because IDs are changed across tarantool versions. Aside of this, size
  of a enum is compiler defined. Anyway, we can expose field types as
  numbers and implement number-to-name and name-to-number mapping
  functions, but IMHO it would just add extra complexity.

The dependency on the fiber compilation unit is added for key_def: it is
purely to obtain the box region in the module API functions. The key_def
compilation unit does not use anything fiber related in fact.
---
 src/box/key_def.c                | 141 ++++++++++++++++++
 src/box/key_def.h                | 144 ++++++++++++++++++-
 src/exports.h                    |   2 +
 test/app-tap/module_api.c        | 240 +++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |   2 +-
 5 files changed, 527 insertions(+), 2 deletions(-)

diff --git a/src/box/key_def.c b/src/box/key_def.c
index a03537227..e2bcfe321 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -39,6 +39,7 @@
 #include "coll_id_cache.h"
 #include "small/region.h"
 #include "coll/coll.h"
+#include "fiber.h"
 
 const char *sort_order_strs[] = { "asc", "desc", "undef" };
 
@@ -341,6 +342,76 @@ key_def_dump_parts(const struct key_def *def, struct key_part_def *parts,
 	return 0;
 }
 
+ /* {{{ Module API helpers */
+
+static int
+key_def_set_internal_part(struct key_part_def *internal_part,
+			  box_key_part_def_t *part, struct region *region)
+{
+	*internal_part = key_part_def_default;
+
+	/* Set internal_part->fieldno. */
+	internal_part->fieldno = part->fieldno;
+
+	/* Set internal_part->type. */
+	if (part->field_type == NULL) {
+		diag_set(IllegalParams, "Field type is mandatory");
+		return -1;
+	}
+	size_t type_len = strlen(part->field_type);
+	internal_part->type = field_type_by_name(part->field_type, type_len);
+	if (internal_part->type == field_type_MAX) {
+		diag_set(IllegalParams, "Unknown field type: \"%s\"",
+			 part->field_type);
+		return -1;
+	}
+
+	/* Set internal_part->{is_nullable,nullable_action}. */
+	bool is_nullable = (part->flags & BOX_KEY_PART_DEF_IS_NULLABLE) ==
+		BOX_KEY_PART_DEF_IS_NULLABLE;
+	if (is_nullable) {
+		internal_part->is_nullable = is_nullable;
+		internal_part->nullable_action = ON_CONFLICT_ACTION_NONE;
+	}
+
+	/* Set internal_part->coll_id. */
+	if (part->collation != NULL) {
+		size_t collation_len = strlen(part->collation);
+		struct coll_id *coll_id = coll_by_name(part->collation,
+						       collation_len);
+		if (coll_id == NULL) {
+			diag_set(IllegalParams, "Unknown collation: \"%s\"",
+				 part->collation);
+			return -1;
+		}
+		internal_part->coll_id = coll_id->id;
+	}
+
+	/* Set internal_part->path (JSON path). */
+	if (part->path) {
+		size_t path_len = strlen(part->path);
+		if (json_path_validate(part->path, path_len,
+				       TUPLE_INDEX_BASE) != 0) {
+			diag_set(IllegalParams, "Invalid JSON path: \"%s\"",
+				 part->path);
+			return -1;
+		}
+		char *tmp = region_alloc(region, path_len + 1);
+		if (tmp == NULL) {
+			diag_set(OutOfMemory, path_len + 1, "region", "path");
+			return -1;
+		}
+		memcpy(tmp, part->path, path_len + 1);
+		internal_part->path = tmp;
+	}
+
+	return 0;
+}
+
+/* }}} Module API helpers */
+
+/* {{{ Module API functions */
+
 box_key_def_t *
 box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
 {
@@ -368,6 +439,74 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
 	return key_def;
 }
 
+void
+box_key_part_def_create(box_key_part_def_t *part)
+{
+	memset(part, 0, sizeof(*part));
+}
+
+box_key_def_t *
+box_key_def_new_v2(box_key_part_def_t *parts, uint32_t part_count)
+{
+	if (part_count == 0) {
+		diag_set(IllegalParams, "At least one key part is required");
+		return NULL;
+	}
+
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	size_t internal_parts_size;
+	struct key_part_def *internal_parts =
+		region_alloc_array(region, typeof(internal_parts[0]),
+				   part_count, &internal_parts_size);
+	if (internal_parts == NULL) {
+		diag_set(OutOfMemory, internal_parts_size, "region_alloc_array",
+			 "parts");
+		return NULL;
+	}
+
+	/*
+	 * It is possible to implement a function similar to
+	 * key_def_new() and eliminate <box_key_part_def_t> ->
+	 * <struct key_part_def> copying. However this would lead
+	 * to code duplication and would complicate maintanence,
+	 * so it worth to do so only if key_def creation will
+	 * appear on a hot path in some meaningful use case.
+	 */
+	uint32_t min_field_count = 0;
+	for (uint32_t i = 0; i < part_count; ++i) {
+		if (key_def_set_internal_part(&internal_parts[i], &parts[i],
+					      region) != 0) {
+			region_truncate(region, region_svp);
+			return NULL;
+		}
+		bool is_nullable =
+			(parts[i].flags & BOX_KEY_PART_DEF_IS_NULLABLE) ==
+			BOX_KEY_PART_DEF_IS_NULLABLE;
+		if (!is_nullable && parts[i].fieldno > min_field_count)
+			min_field_count = parts[i].fieldno;
+	}
+
+	struct key_def *key_def = key_def_new(internal_parts, part_count,
+					      false);
+	region_truncate(region, region_svp);
+	if (key_def == NULL)
+		return NULL;
+
+	/*
+	 * Update key_def->has_optional_parts and function
+	 * pointers.
+	 *
+	 * FIXME: It seems, this call should be part of
+	 * key_def_new(), because otherwise a callee function may
+	 * obtain an incorrect key_def. However I don't know any
+	 * case that would prove this guess.
+	 */
+	key_def_update_optionality(key_def, min_field_count);
+
+	return key_def;
+}
+
 void
 box_key_def_delete(box_key_def_t *key_def)
 {
@@ -391,6 +530,8 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
 
 }
 
+/* }}} Module API functions */
+
 int
 key_part_cmp(const struct key_part *parts1, uint32_t part_count1,
 	     const struct key_part *parts2, uint32_t part_count2)
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 625bb6fea..9d7785c59 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -289,14 +289,108 @@ key_def_delete(struct key_def *def);
 
 typedef struct tuple box_tuple_t;
 
+/* {{{ Module API */
+
 /** \cond public */
 
 typedef struct key_def box_key_def_t;
 
+/** Key part definition flags. */
+enum {
+	BOX_KEY_PART_DEF_IS_NULLABLE = 1 << 0,
+};
+
+/**
+ * It is recommended to verify size of <box_key_part_def_t>
+ * against this constant on the module side at build time.
+ * Example:
+ *
+ * | #if !defined(__cplusplus) && !defined(static_assert)
+ * | #define static_assert _Static_assert
+ * | #endif
+ * |
+ * | (slash)*
+ * |  * Verify that <box_key_part_def_t> has the same size when
+ * |  * compiled within tarantool and within the module.
+ * |  *
+ * |  * It is important, because the module allocates an array of key
+ * |  * parts and passes it to <box_key_def_new_v2>() tarantool
+ * |  * function.
+ * |  *(slash)
+ * | static_assert(sizeof(box_key_part_def_t) == BOX_KEY_PART_DEF_T_SIZE,
+ * |               "sizeof(box_key_part_def_t)");
+ *
+ * This snippet is not part of module.h, because portability of
+ * static_assert() / _Static_assert() is dubious. It should be
+ * decision of a module author how portable its code should be.
+ */
+enum {
+	BOX_KEY_PART_DEF_T_SIZE = 64,
+};
+
 /**
- * Create key definition with key fields with passed typed on passed positions.
+ * Public representation of a key part definition.
+ *
+ * Usage: Allocate an array of such key parts, initialize each
+ * key part (call <box_key_part_def_create>() and set necessary
+ * fields), pass the array into <box_key_def_new_v2>() function.
+ *
+ * The idea of separation from internal <struct key_part_def> is
+ * to provide stable API and ABI for modules.
+ *
+ * New fields may be added into the end of the structure in later
+ * tarantool versions. Also new flags may be introduced within
+ * <flags> field. <collation> cannot be changed to a union (to
+ * reuse for some other value), because it is verified even for
+ * a non-string key part by <box_key_def_new_v2>().
+ *
+ * Fields that are unknown at given tarantool version are ignored.
+ */
+typedef union PACKED {
+	struct {
+		/** Index of a tuple field (zero based). */
+		uint32_t fieldno;
+		/** Flags, e.g. nullability. */
+		uint32_t flags;
+		/** Type of the tuple field. */
+		const char *field_type;
+		/** Collation name for string comparisons. */
+		const char *collation;
+		/**
+		 * JSON path to point a nested field.
+		 *
+		 * Example:
+		 *
+		 * tuple: [1, {"foo": "bar"}]
+		 * key parts: [
+		 *     {
+		 *         "fieldno": 2,
+		 *         "type": "string",
+		 *         "path": "foo"
+		 *     }
+		 * ]
+		 *
+		 * => key: ["bar"]
+		 *
+		 * Note: When the path is given, <field_type>
+		 * means type of the nested field.
+		 */
+		const char *path;
+	};
+	/**
+	 * Padding to guarantee certain size across different
+	 * tarantool versions.
+	 */
+	char padding[BOX_KEY_PART_DEF_T_SIZE];
+} box_key_part_def_t;
+
+/**
+ * Create key definition with given field numbers and field types.
+ *
  * May be used for tuple format creation and/or tuple comparison.
  *
+ * \sa <box_key_def_new_v2>().
+ *
  * \param fields array with key field identifiers
  * \param types array with key field types (see enum field_type)
  * \param part_count the number of key fields
@@ -305,6 +399,44 @@ typedef struct key_def box_key_def_t;
 API_EXPORT box_key_def_t *
 box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
 
+/**
+ * Initialize a key part with default values.
+ *
+ *  | Field       | Default value   | Details |
+ *  | ----------- | --------------- | ------- |
+ *  | fieldno     | 0               |         |
+ *  | flags       | <default flags> |         |
+ *  | field_type  | NULL            | [^1]    |
+ *  | collation   | NULL            |         |
+ *  | path        | NULL            |         |
+ *
+ * Default flag values are the following:
+ *
+ *  | Flag                         | Default value |
+ *  | ---------------------------- | ------------- |
+ *  | BOX_KEY_PART_DEF_IS_NULLABLE | 0 (unset)     |
+ *
+ * All trailing padding bytes are set to zero. The same for
+ * unknown <flags> bits.
+ *
+ * [^1]: <box_key_def_new_v2>() does not accept NULL as a
+ *       <field_type>, so it should be filled explicitly.
+ */
+API_EXPORT void
+box_key_part_def_create(box_key_part_def_t *part);
+
+/**
+ * Create a key_def from given key parts.
+ *
+ * Unlike <box_key_def_new>() this function allows to define
+ * nullability, collation and other options for each key part.
+ *
+ * <box_key_part_def_t> fields that are unknown at given tarantool
+ * version are ignored. The same for unknown <flags> bits.
+ */
+API_EXPORT box_key_def_t *
+box_key_def_new_v2(box_key_part_def_t *parts, uint32_t part_count);
+
 /**
  * Delete key definition
  *
@@ -343,6 +475,16 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
 
 /** \endcond public */
 
+/*
+ * Size of the structure should remain the same across all
+ * tarantool versions in order to allow to allocate an array of
+ * them.
+ */
+static_assert(sizeof(box_key_part_def_t) == BOX_KEY_PART_DEF_T_SIZE,
+	      "sizeof(box_key_part_def_t)");
+
+/* }}} Module API */
+
 static inline size_t
 key_def_sizeof(uint32_t part_count, uint32_t path_pool_size)
 {
diff --git a/src/exports.h b/src/exports.h
index 299bb9fc1..604c1dfaa 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -31,6 +31,8 @@ EXPORT(box_iterator_free)
 EXPORT(box_iterator_next)
 EXPORT(box_key_def_delete)
 EXPORT(box_key_def_new)
+EXPORT(box_key_def_new_v2)
+EXPORT(box_key_part_def_create)
 EXPORT(box_latch_delete)
 EXPORT(box_latch_lock)
 EXPORT(box_latch_new)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 184fdf8f8..26853dd9b 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -1,4 +1,5 @@
 #include <stdbool.h>
+#include <string.h>
 #include <module.h>
 
 #include <small/ibuf.h>
@@ -319,6 +320,8 @@ error:
 	return 1;
 }
 
+/* {{{ key_def api */
+
 static int
 test_key_def_api(lua_State *L)
 {
@@ -365,6 +368,242 @@ test_key_def_api(lua_State *L)
 	return 1;
 }
 
+/* }}} key_def api */
+
+/* {{{ key_def api v2 */
+
+/*
+ * More functions around key_def were exposed to the module API
+ * in order to implement external tuple.keydef and tuple.merger
+ * modules (gh-5273, gh-5384).
+ */
+
+/**
+ * Verify type and message of an error in the diagnostics area.
+ *
+ * Accepts a prefix of an actual type or a message. Pass an empty
+ * string if you need to verify only type or only message.
+ */
+static void
+check_diag(const char *exp_err_type, const char *exp_err_msg)
+{
+	box_error_t *e = box_error_last();
+	assert(!strcmp(box_error_type(e), exp_err_type));
+	assert(!strcmp(box_error_message(e), exp_err_msg));
+}
+
+/**
+ * Create a tuple on runtime arena.
+ *
+ * Release this tuple using <box_tuple_unref>().
+ */
+static box_tuple_t *
+new_runtime_tuple(const char *tuple_data, size_t tuple_size)
+{
+	box_tuple_format_t *fmt = box_tuple_format_default();
+	const char *tuple_end = tuple_data + tuple_size;
+	box_tuple_t *tuple = box_tuple_new(fmt, tuple_data, tuple_end);
+	assert(tuple != NULL);
+	box_tuple_ref(tuple);
+	return tuple;
+}
+
+/**
+ * Where padding bytes starts.
+ */
+static size_t
+key_part_padding_offset(void)
+{
+	if (sizeof(void *) * CHAR_BIT == 64)
+		return 32;
+	if (sizeof(void *) * CHAR_BIT == 32)
+		return 20;
+	assert(false);
+}
+
+/**
+ * Mask of all defined flags.
+ */
+static uint32_t
+key_part_def_known_flags(void)
+{
+	return BOX_KEY_PART_DEF_IS_NULLABLE;
+}
+
+/**
+ * Default flags value.
+ *
+ * All unknown bits are set to zero.
+ */
+static uint32_t
+key_part_def_default_flags(void)
+{
+	return 0;
+}
+
+/**
+ * Set all <box_key_part_def_t> fields to nondefault values.
+ *
+ * It also set all padding bytes and unknown flags to non-zero
+ * values.
+ */
+static void
+key_part_def_set_nondefault(box_key_part_def_t *part)
+{
+	size_t padding_offset = key_part_padding_offset();
+	uint32_t default_flags = key_part_def_default_flags();
+
+	/*
+	 * Give correct non-default values for known fields and
+	 * flags. Set unknown flags to non-zero values.
+	 */
+	part->fieldno = 1;
+	part->flags = ~default_flags;
+	part->field_type = "string";
+	part->collation = "unicode_ci";
+	part->path = "foo";
+
+	/* Fill padding with non-zero bytes. */
+	char *padding = ((char *) part) + padding_offset;
+	size_t padding_size = sizeof(box_key_part_def_t) - padding_offset;
+	memset(padding, 0xff, padding_size);
+}
+
+/**
+ * Verify that all known fields and flags are set to default
+ * values.
+ */
+static void
+key_part_def_check_default(box_key_part_def_t *part)
+{
+	uint32_t known_flags = key_part_def_known_flags();
+	uint32_t default_flags = key_part_def_default_flags();
+
+	assert(part->fieldno == 0);
+	assert((part->flags & known_flags) == default_flags);
+	assert(part->field_type == NULL);
+	assert(part->collation == NULL);
+	assert(part->path == NULL);
+}
+
+/**
+ * Verify that all padding bytes and unknown flags are set to
+ * zeros.
+ */
+static void
+key_part_def_check_zeros(const box_key_part_def_t *part)
+{
+	size_t padding_offset = key_part_padding_offset();
+	uint32_t unknown_flags = ~key_part_def_known_flags();
+
+	char *padding = ((char *) part) + padding_offset;
+	char *padding_end = ((char *) part) + sizeof(box_key_part_def_t);
+	for (char *p = padding; p < padding_end; ++p) {
+		assert(*p == 0);
+	}
+
+	assert((part->flags & unknown_flags) == 0);
+}
+
+/**
+ * Basic <box_key_part_def_create>() and <test_key_def_new_v2>()
+ * test.
+ */
+static int
+test_key_def_new_v2(struct lua_State *L)
+{
+	/* Verify <box_key_part_def_t> binary layout. */
+	assert(BOX_KEY_PART_DEF_T_SIZE == 64);
+	assert(sizeof(box_key_part_def_t) == BOX_KEY_PART_DEF_T_SIZE);
+	assert(offsetof(box_key_part_def_t, fieldno) == 0);
+	assert(offsetof(box_key_part_def_t, flags) == 4);
+	assert(offsetof(box_key_part_def_t, field_type) == 8);
+	if (sizeof(void *) * CHAR_BIT == 64) {
+		assert(offsetof(box_key_part_def_t, collation) == 16);
+		assert(offsetof(box_key_part_def_t, path) == 24);
+	} else if (sizeof(void *) * CHAR_BIT == 32) {
+		assert(offsetof(box_key_part_def_t, collation) == 12);
+		assert(offsetof(box_key_part_def_t, path) == 16);
+	} else {
+		assert(false);
+	}
+
+	/*
+	 * Fill key part definition with nondefault values.
+	 * Fill padding and unknown flags with non-zero values.
+	 */
+	box_key_part_def_t part;
+	key_part_def_set_nondefault(&part);
+
+	/*
+	 * Verify that all known fields are set to default values and
+	 * all unknown fields and flags are set to zeros.
+	 */
+	box_key_part_def_create(&part);
+	key_part_def_check_default(&part);
+	key_part_def_check_zeros(&part);
+
+	box_key_def_t *key_def;
+
+	/* Should not accept zero part count. */
+	key_def = box_key_def_new_v2(NULL, 0);
+	assert(key_def == NULL);
+	check_diag("IllegalParams", "At least one key part is required");
+
+	/* Should not accept NULL as a <field_type>. */
+	key_def = box_key_def_new_v2(&part, 1);
+	assert(key_def == NULL);
+	check_diag("IllegalParams", "Field type is mandatory");
+
+	/* Success case. */
+	part.field_type = "unsigned";
+	key_def = box_key_def_new_v2(&part, 1);
+	assert(key_def != NULL);
+
+	/*
+	 * Prepare tuples to do some comparisons.
+	 *
+	 * [1, 2, 3] and [3, 2, 1].
+	 */
+	box_tuple_t *tuple_1 = new_runtime_tuple("\x93\x01\x02\x03", 4);
+	box_tuple_t *tuple_2 = new_runtime_tuple("\x93\x03\x02\x01", 4);
+
+	/*
+	 * Verify that key_def actually can be used in functions
+	 * that accepts it.
+	 *
+	 * Do several comparisons. Far away from being an
+	 * exhaustive comparator test.
+	 */
+	int rc;
+	rc = box_tuple_compare(tuple_1, tuple_1, key_def);
+	assert(rc == 0);
+	rc = box_tuple_compare(tuple_2, tuple_2, key_def);
+	assert(rc == 0);
+	rc = box_tuple_compare(tuple_1, tuple_2, key_def);
+	assert(rc < 0);
+	rc = box_tuple_compare(tuple_2, tuple_1, key_def);
+	assert(rc > 0);
+
+	/* The same idea, but perform comparisons against keys. */
+	rc = box_tuple_compare_with_key(tuple_1, "\x91\x00", key_def);
+	assert(rc > 0);
+	rc = box_tuple_compare_with_key(tuple_1, "\x91\x01", key_def);
+	assert(rc == 0);
+	rc = box_tuple_compare_with_key(tuple_1, "\x91\x02", key_def);
+	assert(rc < 0);
+
+	/* Clean up. */
+	box_tuple_unref(tuple_1);
+	box_tuple_unref(tuple_2);
+	box_key_def_delete(key_def);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
+/* }}} key_def api v2 */
+
 static int
 check_error(lua_State *L)
 {
@@ -740,6 +979,7 @@ luaopen_module_api(lua_State *L)
 		{"test_box_region", test_box_region},
 		{"test_tuple_encode", test_tuple_encode},
 		{"test_tuple_new", test_tuple_new},
+		{"test_key_def_new_v2", test_key_def_new_v2},
 		{NULL, NULL}
 	};
 	luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 0e7ce37db..03cfec2ff 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -172,7 +172,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(28)
+    test:plan(29)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
-- 
2.25.0

[Tarantool-patches] [PATCH v2 11/15] module api: add box_key_def_dump_parts()

[Alexander Turenko]

The function dumps an opaque <box_key_def_t> structure into a non-opaque
array of <box_key_part_def_t> structures in order to allow an external
module to obtain information about the key definition.

Part of #5273
---
 src/box/key_def.c                |  78 +++++++++++++++++++++
 src/box/key_def.h                |  13 ++++
 src/exports.h                    |   1 +
 test/app-tap/module_api.c        | 116 +++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |   2 +-
 5 files changed, 209 insertions(+), 1 deletion(-)

diff --git a/src/box/key_def.c b/src/box/key_def.c
index e2bcfe321..27fdbfbf6 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -513,6 +513,84 @@ box_key_def_delete(box_key_def_t *key_def)
 	key_def_delete(key_def);
 }
 
+box_key_part_def_t *
+box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr)
+{
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	size_t size;
+	box_key_part_def_t *parts = region_alloc_array(
+		region, typeof(parts[0]), key_def->part_count, &size);
+	if (parts == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc_array", "parts");
+		return NULL;
+	}
+
+	for (uint32_t i = 0; i < key_def->part_count; i++) {
+		const struct key_part *part = &key_def->parts[i];
+		box_key_part_def_t *part_def = &parts[i];
+		box_key_part_def_create(part_def);
+
+		/* Set part->{fieldno,flags,field_type}. */
+		part_def->fieldno = part->fieldno;
+		part_def->flags = 0;
+		if (key_part_is_nullable(part))
+			part_def->flags |= BOX_KEY_PART_DEF_IS_NULLABLE;
+		assert(part->type >= 0 && part->type < field_type_MAX);
+		part_def->field_type = field_type_strs[part->type];
+
+		/* Set part->collation. */
+		if (part->coll_id != COLL_NONE) {
+			struct coll_id *coll_id = coll_by_id(part->coll_id);
+			/*
+			 * A collation may be removed after
+			 * key_def creation.
+			 */
+			if (coll_id == NULL) {
+				diag_set(CollationError,
+					 "key_def holds dead collation id %d",
+					 part->coll_id);
+				region_truncate(region, region_svp);
+				return NULL;
+			}
+			/*
+			 * A collation may be removed while the
+			 * resulting key parts array is in use.
+			 */
+			char *collation = region_alloc(region,
+						       coll_id->name_len + 1);
+			if (collation == NULL) {
+				diag_set(OutOfMemory, coll_id->name_len + 1,
+					 "region_alloc", "part_def->collation");
+				region_truncate(region, region_svp);
+				return NULL;
+			}
+			memcpy(collation, coll_id->name, coll_id->name_len);
+			collation[coll_id->name_len] = '\0';
+			part_def->collation = collation;
+		}
+
+		/* Set part->path. */
+		if (part->path != NULL) {
+			char *path = region_alloc(region, part->path_len + 1);
+			if (path == NULL) {
+				diag_set(OutOfMemory, part->path_len + 1,
+					 "region", "part_def->path");
+				region_truncate(region, region_svp);
+				return NULL;
+			}
+			memcpy(path, part->path, part->path_len);
+			path[part->path_len] = '\0';
+			part_def->path = path;
+		}
+	}
+
+	if (part_count_ptr != NULL)
+		*part_count_ptr = key_def->part_count;
+
+	return parts;
+}
+
 int
 box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
 		  box_key_def_t *key_def)
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 9d7785c59..a61b6e7e9 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -445,6 +445,19 @@ box_key_def_new_v2(box_key_part_def_t *parts, uint32_t part_count);
 API_EXPORT void
 box_key_def_delete(box_key_def_t *key_def);
 
+/**
+ * Dump key part definitions of given key_def.
+ *
+ * The function allocates key parts and storage for pointer fields
+ * (e.g. collation names) on the box region.
+ * @sa <box_region_truncate>().
+ *
+ * <box_key_part_def_t> fields that are unknown at given tarantool
+ * version are set to zero. The same for unknown <flags> bits.
+ */
+API_EXPORT box_key_part_def_t *
+box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr);
+
 /**
  * Compare tuples using the key definition.
  * @param tuple_a first tuple
diff --git a/src/exports.h b/src/exports.h
index 604c1dfaa..8ffc4d887 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -30,6 +30,7 @@ EXPORT(box_insert)
 EXPORT(box_iterator_free)
 EXPORT(box_iterator_next)
 EXPORT(box_key_def_delete)
+EXPORT(box_key_def_dump_parts)
 EXPORT(box_key_def_new)
 EXPORT(box_key_def_new_v2)
 EXPORT(box_key_part_def_create)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 26853dd9b..25c98ef63 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -378,6 +378,22 @@ test_key_def_api(lua_State *L)
  * modules (gh-5273, gh-5384).
  */
 
+/**
+ * Verify that two zero terminated strings are either both NULL
+ * or have equal values.
+ */
+static void
+string_check_equal(const char *a, const char *b)
+{
+	if (a == NULL) {
+		assert(b == NULL);
+	} else {
+		assert(b != NULL);
+		assert(strlen(a) == strlen(b));
+		assert(!strcmp(a, b));
+	}
+}
+
 /**
  * Verify type and message of an error in the diagnostics area.
  *
@@ -505,6 +521,25 @@ key_part_def_check_zeros(const box_key_part_def_t *part)
 	assert((part->flags & unknown_flags) == 0);
 }
 
+/**
+ * Check that two key part definitions are equal.
+ *
+ * It compares only known fields and flags, but ignores padding
+ * bytes and unknown flags.
+ */
+static void
+key_part_def_check_equal(const box_key_part_def_t *a,
+			 const box_key_part_def_t *b)
+{
+	uint32_t known_flags = key_part_def_known_flags();
+
+	assert(a->fieldno == b->fieldno);
+	assert((a->flags & known_flags) == (b->flags & known_flags));
+	string_check_equal(a->field_type, b->field_type);
+	string_check_equal(a->collation, b->collation);
+	string_check_equal(a->path, b->path);
+}
+
 /**
  * Basic <box_key_part_def_create>() and <test_key_def_new_v2>()
  * test.
@@ -602,6 +637,86 @@ test_key_def_new_v2(struct lua_State *L)
 	return 1;
 }
 
+/**
+ * Basic <test_key_def_dump_parts>() test.
+ */
+static int
+test_key_def_dump_parts(struct lua_State *L)
+{
+	size_t region_svp = box_region_used();
+	box_key_def_t *key_def = NULL;
+	box_key_part_def_t *dump = NULL;
+	uint32_t dump_part_count = 0;
+
+	/*
+	 * Create a key_def with a single key part with all fields
+	 * and flags set to non-default values.
+	 */
+	box_key_part_def_t part;
+	key_part_def_set_nondefault(&part);
+	key_def = box_key_def_new_v2(&part, 1);
+	assert(key_def != NULL);
+
+	/*
+	 * Verify that the same values are dumped, but unknown
+	 * fields and flags are set to zeros.
+	 */
+	dump = box_key_def_dump_parts(key_def, &dump_part_count);
+	assert(dump != NULL);
+	assert(dump_part_count == 1);
+	key_part_def_check_equal(&part, &dump[0]);
+	key_part_def_check_zeros(&dump[0]);
+
+	/* We can pass NULL as <part_count_ptr>. */
+	dump = box_key_def_dump_parts(key_def, NULL);
+	assert(dump != NULL);
+
+	/* Clean up. */
+	box_key_def_delete(key_def);
+
+	/* Create a key_def from two key part definitions. */
+	box_key_part_def_t parts[2];
+	box_key_part_def_create(&parts[0]);
+	box_key_part_def_create(&parts[1]);
+	parts[0].fieldno = 19;
+	parts[0].field_type = "unsigned";
+	parts[0].path = "foo";
+	parts[1].fieldno = 7;
+	parts[1].field_type = "string";
+	parts[1].collation = "unicode";
+	parts[1].flags |= BOX_KEY_PART_DEF_IS_NULLABLE;
+	key_def = box_key_def_new_v2(parts, 2);
+	assert(key_def != NULL);
+
+	/* Verify how it'll be dumped. */
+	dump = box_key_def_dump_parts(key_def, &dump_part_count);
+	assert(dump != NULL);
+	assert(dump_part_count == 2);
+	key_part_def_check_equal(&parts[0], &dump[0]);
+	key_part_def_check_equal(&parts[1], &dump[1]);
+
+	/* Clean up. */
+	box_key_def_delete(key_def);
+
+	/* Can we again create a key_def from the dumped parts? */
+	key_def = box_key_def_new_v2(dump, dump_part_count);
+	assert(key_def != NULL);
+
+	/* Verify this dump based key_def. */
+	dump = box_key_def_dump_parts(key_def, &dump_part_count);
+	assert(dump != NULL);
+	assert(dump_part_count == 2);
+	key_part_def_check_equal(&parts[0], &dump[0]);
+	key_part_def_check_equal(&parts[1], &dump[1]);
+
+	/* Clean up. */
+	box_key_def_delete(key_def);
+	box_region_truncate(region_svp);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
 /* }}} key_def api v2 */
 
 static int
@@ -980,6 +1095,7 @@ luaopen_module_api(lua_State *L)
 		{"test_tuple_encode", test_tuple_encode},
 		{"test_tuple_new", test_tuple_new},
 		{"test_key_def_new_v2", test_key_def_new_v2},
+		{"test_key_def_dump_parts", test_key_def_dump_parts},
 		{NULL, NULL}
 	};
 	luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 03cfec2ff..b751d5b98 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -172,7 +172,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(29)
+    test:plan(30)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
-- 
2.25.0

[Tarantool-patches] [PATCH v2 12/15] module api: expose box_key_def_validate_tuple()

[Alexander Turenko]

Part of #5273
---
 src/box/key_def.c                |  6 ++
 src/box/key_def.h                | 12 ++++
 src/exports.h                    |  1 +
 test/app-tap/module_api.c        | 94 ++++++++++++++++++++++++++++++++
 test/app-tap/module_api.test.lua |  2 +-
 5 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/src/box/key_def.c b/src/box/key_def.c
index 27fdbfbf6..e7ecb6ec5 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -591,6 +591,12 @@ box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr)
 	return parts;
 }
 
+int
+box_key_def_validate_tuple(box_key_def_t *key_def, box_tuple_t *tuple)
+{
+	return tuple_validate_key_parts(key_def, tuple);
+}
+
 int
 box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
 		  box_key_def_t *key_def)
diff --git a/src/box/key_def.h b/src/box/key_def.h
index a61b6e7e9..f33c20430 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -458,6 +458,18 @@ box_key_def_delete(box_key_def_t *key_def);
 API_EXPORT box_key_part_def_t *
 box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr);
 
+/**
+ * Check that tuple fields match with given key definition.
+ *
+ * @param key_def  Key definition.
+ * @param tuple    Tuple to validate.
+ *
+ * @retval 0   The tuple is valid.
+ * @retval -1  The tuple is invalid.
+ */
+API_EXPORT int
+box_key_def_validate_tuple(box_key_def_t *key_def, box_tuple_t *tuple);
+
 /**
  * Compare tuples using the key definition.
  * @param tuple_a first tuple
diff --git a/src/exports.h b/src/exports.h
index 8ffc4d887..71ec1a9b5 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -33,6 +33,7 @@ EXPORT(box_key_def_delete)
 EXPORT(box_key_def_dump_parts)
 EXPORT(box_key_def_new)
 EXPORT(box_key_def_new_v2)
+EXPORT(box_key_def_validate_tuple)
 EXPORT(box_key_part_def_create)
 EXPORT(box_latch_delete)
 EXPORT(box_latch_lock)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 25c98ef63..8b46cdb0d 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -717,6 +717,99 @@ test_key_def_dump_parts(struct lua_State *L)
 	return 1;
 }
 
+/**
+ * Basic <box_key_def_validate_tuple>() test.
+ */
+static int
+test_key_def_validate_tuple(struct lua_State *L)
+{
+	/*
+	 * Create a key_def.
+	 *
+	 *  |              tuple
+	 *  |            [x, x, x]
+	 *  | key_def     ^     ^
+	 *  |    |        |     |
+	 *  |   (0) <-----+---- string (optional)
+	 *  |    |        |
+	 *  |   (1) <---- unsigned
+	 */
+	box_key_part_def_t parts[2];
+	box_key_part_def_create(&parts[0]);
+	box_key_part_def_create(&parts[1]);
+	parts[0].fieldno = 2;
+	parts[0].field_type = "string";
+	parts[0].flags |= BOX_KEY_PART_DEF_IS_NULLABLE;
+	parts[1].fieldno = 0;
+	parts[1].field_type = "unsigned";
+	box_key_def_t *key_def = box_key_def_new_v2(parts, 2);
+	assert(key_def != NULL);
+
+	/*
+	 * Create tuples to validate them against given key_def.
+	 *
+	 *  | # | tuple         | Is valid? |
+	 *  | - | ------------- | --------- |
+	 *  | 0 | [1, 2, "moo"] | valid     |
+	 *  | 1 | [1, 2, null]  | valid     |
+	 *  | 2 | [1, 2]        | valid     |
+	 *  | 3 | [1]           | valid     |
+	 *  | 4 | []            | invalid   |
+	 *  | 5 | [1, 2, 3]     | invalid   |
+	 *  | 6 | ["moo"]       | invalid   |
+	 *  | 7 | [-1]          | invalid   |
+	 */
+	box_tuple_t *tuples[] = {
+		/* [0] = */ new_runtime_tuple("\x93\x01\x02\xa3moo", 7),
+		/* [1] = */ new_runtime_tuple("\x93\x01\x02\xc0", 4),
+		/* [2] = */ new_runtime_tuple("\x92\x01\x02", 3),
+		/* [3] = */ new_runtime_tuple("\x91\x01", 2),
+		/* [4] = */ new_runtime_tuple("\x90", 1),
+		/* [5] = */ new_runtime_tuple("\x93\x01\x02\x03", 4),
+		/* [6] = */ new_runtime_tuple("\x91\xa3moo", 5),
+		/* [7] = */ new_runtime_tuple("\x91\xff", 2),
+	};
+	int expected_results[] = {
+		/* [0] = */ 0,
+		/* [1] = */ 0,
+		/* [2] = */ 0,
+		/* [3] = */ 0,
+		/* [4] = */ -1,
+		/* [5] = */ -1,
+		/* [6] = */ -1,
+		/* [7] = */ -1,
+	};
+	uint32_t expected_error_codes[] = {
+		/* [0] = */ box_error_code_MAX,
+		/* [1] = */ box_error_code_MAX,
+		/* [2] = */ box_error_code_MAX,
+		/* [3] = */ box_error_code_MAX,
+		/* [4] = */ ER_FIELD_MISSING,
+		/* [5] = */ ER_KEY_PART_TYPE,
+		/* [6] = */ ER_KEY_PART_TYPE,
+		/* [7] = */ ER_KEY_PART_TYPE,
+	};
+
+	for (size_t i = 0; i < lengthof(tuples); ++i) {
+		int rc = box_key_def_validate_tuple(key_def, tuples[i]);
+		assert(rc == expected_results[i]);
+
+		if (expected_error_codes[i] != box_error_code_MAX) {
+			assert(rc != 0);
+			box_error_t *e = box_error_last();
+			assert(box_error_code(e) == expected_error_codes[i]);
+		}
+	}
+
+	/* Clean up. */
+	for (size_t i = 0; i < lengthof(tuples); ++i)
+		box_tuple_unref(tuples[i]);
+	box_key_def_delete(key_def);
+
+	lua_pushboolean(L, 1);
+	return 1;
+}
+
 /* }}} key_def api v2 */
 
 static int
@@ -1096,6 +1189,7 @@ luaopen_module_api(lua_State *L)
 		{"test_tuple_new", test_tuple_new},
 		{"test_key_def_new_v2", test_key_def_new_v2},
 		{"test_key_def_dump_parts", test_key_def_dump_parts},
+		{"test_key_def_validate_tuple", test_key_def_validate_tuple},
 		{NULL, NULL}
 	};
 	luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index b751d5b98..cfb0ca00b 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -172,7 +172,7 @@ local function test_iscdata(test, module)
 end
 
 local test = require('tap').test("module_api", function(test)
-    test:plan(30)
+    test:plan(31)
     local status, module = pcall(require, 'module_api')
     test:is(status, true, "module")
     test:ok(status, "module is loaded")
-- 
2.25.0

[Tarantool-patches] [PATCH v2 13/15] WIP: module api: expose box_key_def_merge()

[Alexander Turenko]

Part of #5273
---
 src/box/key_def.c |  6 ++++++
 src/box/key_def.h | 14 ++++++++++++++
 src/exports.h     |  1 +
 3 files changed, 21 insertions(+)

diff --git a/src/box/key_def.c b/src/box/key_def.c
index e7ecb6ec5..25a8decf0 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -614,6 +614,12 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
 
 }
 
+box_key_def_t *
+box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second)
+{
+	return key_def_merge(first, second);
+}
+
 /* }}} Module API functions */
 
 int
diff --git a/src/box/key_def.h b/src/box/key_def.h
index f33c20430..224b56eb9 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -498,6 +498,20 @@ API_EXPORT int
 box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
 			   box_key_def_t *key_def);
 
+/**
+ * Allocate a new key_def with a set union of key parts from
+ * first and second key defs.
+ *
+ * Parts of the new key_def consist of the first key_def's parts
+ * and those parts of the second key_def that were not among the
+ * first parts.
+ *
+ * @retval not NULL  Ok.
+ * @retval NULL      Memory error.
+ */
+API_EXPORT box_key_def_t *
+box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second);
+
 /** \endcond public */
 
 /*
diff --git a/src/exports.h b/src/exports.h
index 71ec1a9b5..223390d52 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_merge)
 EXPORT(box_key_def_new)
 EXPORT(box_key_def_new_v2)
 EXPORT(box_key_def_validate_tuple)
-- 
2.25.0

[Tarantool-patches] [PATCH v2 14/15] WIP: module api: expose box_key_def_extract_key()

[Alexander Turenko]

Unlike box_tuple_extract_key() it accepts a key_def structure, not
space_id, index_id pair.

Another difference from box_tuple_extract_key() is that this function
allows to pass so called multikey index. See commit 2.2.0-259-gf1d9f2575
('box: introduce multikey indexes in memtx') for details.

Note: The <multikey_idx> parameter is ignored on the backported version
of the patch on 1.10.

XXX: Add a module API test.

Part of #5273
---
 src/box/key_def.c |  7 +++++++
 src/box/key_def.h | 20 ++++++++++++++++++++
 src/exports.h     |  1 +
 3 files changed, 28 insertions(+)

diff --git a/src/box/key_def.c b/src/box/key_def.c
index 25a8decf0..c13951375 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -620,6 +620,13 @@ box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second)
 	return key_def_merge(first, second);
 }
 
+char *
+box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
+			int multikey_idx, uint32_t *key_size_ptr)
+{
+	return tuple_extract_key(tuple, key_def, multikey_idx, key_size_ptr);
+}
+
 /* }}} Module API functions */
 
 int
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 224b56eb9..d0ce92be3 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -512,6 +512,26 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
 API_EXPORT box_key_def_t *
 box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second);
 
+/**
+ * Extract key from tuple by given key definition and return
+ * buffer allocated on the box region with this key.
+ * @sa <box_region_truncate>().
+ *
+ * This function has O(n) complexity, where n is the number of key
+ * parts.
+ *
+ * @param key_def       Definition of key that need to extract.
+ * @param tuple         Tuple from which need to extract key.
+ * @param multikey_idx  Multikey index hint or -1.
+ * @param key_size_ptr  Here will be size of extracted key.
+ *
+ * @retval not NULL  Success.
+ * @retval NULL      Memory allocation error.
+ */
+API_EXPORT char *
+box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
+			int multikey_idx, uint32_t *key_size_ptr);
+
 /** \endcond public */
 
 /*
diff --git a/src/exports.h b/src/exports.h
index 223390d52..a0c7ac84d 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_extract_key)
 EXPORT(box_key_def_merge)
 EXPORT(box_key_def_new)
 EXPORT(box_key_def_new_v2)
-- 
2.25.0

[Tarantool-patches] [PATCH v2 15/15] WIP: module api: add box_key_def_validate_key()

[Alexander Turenko]

XXX: Add a module API test.

Part of #5273
---
 src/box/key_def.c | 13 +++++++++++++
 src/box/key_def.h | 12 ++++++++++++
 src/exports.h     |  1 +
 3 files changed, 26 insertions(+)

diff --git a/src/box/key_def.c b/src/box/key_def.c
index c13951375..af4e8f4cd 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -627,6 +627,19 @@ box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
 	return tuple_extract_key(tuple, key_def, multikey_idx, key_size_ptr);
 }
 
+int
+box_key_def_validate_key(const box_key_def_t *key_def, const char *key)
+{
+	uint32_t part_count = mp_decode_array(&key);
+	if (part_count > key_def->part_count) {
+		diag_set(ClientError, ER_KEY_PART_COUNT, key_def->part_count,
+			 part_count);
+		return -1;
+	}
+	const char *key_end;
+	return key_validate_parts(key_def, key, part_count, true, &key_end);
+}
+
 /* }}} Module API functions */
 
 int
diff --git a/src/box/key_def.h b/src/box/key_def.h
index d0ce92be3..9222c9240 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -532,6 +532,18 @@ API_EXPORT char *
 box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
 			int multikey_idx, uint32_t *key_size_ptr);
 
+/**
+ * Check that parts of the key match with the key definition.
+ *
+ * @param key_def         Key definition.
+ * @param key             MessagePack'ed data for matching.
+ *
+ * @retval 0   The key is valid.
+ * @retval -1  The key is invalid.
+ */
+API_EXPORT int
+box_key_def_validate_key(const box_key_def_t *key_def, const char *key);
+
 /** \endcond public */
 
 /*
diff --git a/src/exports.h b/src/exports.h
index a0c7ac84d..1d7deb518 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -35,6 +35,7 @@ EXPORT(box_key_def_extract_key)
 EXPORT(box_key_def_merge)
 EXPORT(box_key_def_new)
 EXPORT(box_key_def_new_v2)
+EXPORT(box_key_def_validate_key)
 EXPORT(box_key_def_validate_tuple)
 EXPORT(box_key_part_def_create)
 EXPORT(box_latch_delete)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 5 comments below.

On 11.10.2020 14:57, Alexander Turenko wrote:
> This change fixes incorrect behaviour at tuple serialization error in
> several places: <space_object>:frommap(), <key_def_object>:compare(),
> <merge_source>:select(). See more in #5382.
> 
> Disallow creating a tuple from objects on the Lua stack (idx == 0) in
> luaT_tuple_new() for simplicity. There are no such usages in tarantool.
> The function is not exposed yet to the module API. This is only
> necessary in box.tuple.new(), which anyway raises Lua errors by its
> contract.

1. But why? The case of creating a tuple from values may be much faster,
when there is a lot of values not wrapped into a table. Table wrap is
costly.

Could you just merge luaT_tuple_encode_values and luaT_tuple_encode_table
into one function, withuout splitting them?

> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index ed97c85e4..18cfef979 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */
>  
>  uint32_t CTID_STRUCT_TUPLE_REF;
>  
> +int luaT_tuple_encode_table_ref = LUA_NOREF;

2. Better make it static. It is not used outside of this file.

> +
>  box_tuple_t *
>  luaT_checktuple(struct lua_State *L, int idx)
>  {
> @@ -98,39 +100,79 @@ luaT_istuple(struct lua_State *L, int narg)
>  	return *(struct tuple **) data;
>  }
>  
> +/**
> + * Encode a Lua values on a Lua stack as an MsgPack array.
> + *
> + * Raise a Lua error when encoding fails.
> + */
> +static int
> +luaT_tuple_encode_values(struct lua_State *L)
> +{
> +	struct ibuf *buf = tarantool_lua_ibuf;
> +	ibuf_reset(buf);
> +	struct mpstream stream;
> +	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
> +		      L);
> +	int argc = lua_gettop(L);
> +	mpstream_encode_array(&stream, argc);
> +	for (int k = 1; k <= argc; ++k) {
> +		luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
> +	}
> +	mpstream_flush(&stream);
> +	return 0;
> +}
> +
> +/**
> + * Encode a Lua table or a tuple as MsgPack.
> + *
> + * Raise a Lua error when encoding fails.
> + *
> + * It is a kind of critical section to be run under luaT_call().
> + */
> +static int
> +luaT_tuple_encode_table(struct lua_State *L)
> +{
> +	struct ibuf *buf = tarantool_lua_ibuf;
> +	ibuf_reset(buf);
> +	struct mpstream stream;
> +	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
> +		      L);
> +	luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
> +	mpstream_flush(&stream);
> +	return 0;
> +}
> +
>  static char *
>  luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
>  			      size_t *tuple_len_ptr)
>  {
> -	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
> +	assert(idx != 0);
> +	if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
>  		diag_set(IllegalParams, "A tuple or a table expected, got %s",
>  			 lua_typename(L, lua_type(L, idx)));
>  		return NULL;
>  	}
>  
> -	struct ibuf *buf = tarantool_lua_ibuf;
> -	ibuf_reset(buf);
> -	struct mpstream stream;
> -	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
> -		      luamp_error, L);
> -	if (idx == 0) {
> -		/*
> -		 * Create the tuple from lua stack
> -		 * objects.
> -		 */
> -		int argc = lua_gettop(L);
> -		mpstream_encode_array(&stream, argc);
> -		for (int k = 1; k <= argc; ++k) {
> -			luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
> -		}
> -	} else {
> -		/* Create the tuple from a Lua table. */
> -		luamp_encode_tuple(L, &tuple_serializer, &stream, idx);
> -	}
> -	mpstream_flush(&stream);
> +	int top = lua_gettop(L);
> +
> +	/* Calculate absolute value in the stack. */
> +	if (idx < 0)
> +		idx = top + idx + 1;> +
> +	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
> +	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> +	assert(lua_isfunction(L, -1));
> +
> +	lua_pushvalue(L, idx);
> +
> +	int rc = luaT_call(L, 1, 0);
> +	lua_settop(L, top);
> +	if (rc != 0)
> +		return NULL;
> +
>  	if (tuple_len_ptr != NULL)
> -		*tuple_len_ptr = ibuf_used(buf);
> -	return buf->buf;
> +		*tuple_len_ptr = ibuf_used(tarantool_lua_ibuf);

3. Since you assume the function at luaT_tuple_encode_table_ref alwaus
uses ibuf, maybe it would be better to add 'on_lua_ibuf' to its name.
Or move the length calculation out of the current function, because
this one already has 'on_lua_ibuf', so it wouldn't look so strange to
get size of tarantool_lua_ibuf after it.

> +	return tarantool_lua_ibuf->buf;
>  }
>  
>  struct tuple *
> @@ -156,15 +198,30 @@ lbox_tuple_new(lua_State *L)
>  		lua_newtable(L); /* create an empty tuple */
>  		++argc;
>  	}
> +
> +	box_tuple_format_t *fmt = box_tuple_format_default();
> +

4. You could keep it after the comment below to reduce the diff.

>  	/*
>  	 * Use backward-compatible parameters format:
> -	 * box.tuple.new(1, 2, 3) (idx == 0), or the new one:
> -	 * box.tuple.new({1, 2, 3}) (idx == 1).
> +	 * box.tuple.new(1, 2, 3).
>  	 */
> -	int idx = argc == 1 && (lua_istable(L, 1) ||
> -		luaT_istuple(L, 1));
> -	box_tuple_format_t *fmt = box_tuple_format_default();
> -	struct tuple *tuple = luaT_tuple_new(L, idx, fmt);
> +	if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
> +		struct ibuf *buf = tarantool_lua_ibuf;
> +		luaT_tuple_encode_values(L); /* may raise */

5. We usually put comments on separate line.

> +		struct tuple *tuple = box_tuple_new(fmt, buf->buf,
> +						    buf->buf + ibuf_used(buf));
> +		ibuf_reinit(buf);
> +		if (tuple == NULL)
> +			return luaT_error(L);
> +		luaT_pushtuple(L, tuple);
> +		return 1;
> +	}

Re: [Tarantool-patches] [PATCH v2 06/15] WIP: module api/lua: add luaT_tuple_encode()

[Vladislav Shpilevoy]

Thanks for the patch!

See 4 comments below.

On 11.10.2020 14:57, Alexander Turenko wrote:
> It is the same as luaT_tuple_new(), but returns raw MsgPack data (not
> <box_tuple_t>) allocated on the box region.
> 
> The reason to expose this function is to provide ability to use
> box_tuple_compare_with_key() function from an external module for a key
> passed as a Lua table. The compare function has the following signature:
> 
>  | API_EXPORT int
>  | box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
>  |                            box_key_def_t *key_def);
> 
> The second parameter is a key encoded as an MsgPack array, not a tuple
> structure. So luaT_tuple_new() is not applicable here (it is not
> worthful to create a tuple structure if we need just MsgPack data).
> 
> Some complexity was introduced to support encoding on the Lua shared
> buffer and the box region both. The motivation is the following:
> 
> - luaT_tuple_encode() is exposed with encoding to the box region,
>   because it is more usual to the module API. In particular a user of
>   the API able to control when the tuple data should be released.
> - Encoding to the Lua shared buffer is kept internally, because there is
>   no strong reason to change it to the box region for box.tuple.new().
> 
> Part of #5273
> 
> XXX: Try to get rid of luaT_mpstream_init_*() functions: call
> mpstream_init() outside of the protected section with changed error
> handler.
> ---
>  src/box/lua/tuple.c              |  99 ++++++++++++++++++++++----
>  src/box/lua/tuple.h              |  17 +++++
>  src/exports.h                    |   1 +
>  test/app-tap/module_api.c        | 115 +++++++++++++++++++++++++++++++
>  test/app-tap/module_api.test.lua |   2 +-
>  5 files changed, 220 insertions(+), 14 deletions(-)
> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 18cfef979..8e2255a2b 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -100,10 +100,31 @@ luaT_istuple(struct lua_State *L, int narg)
>  	return *(struct tuple **) data;
>  }
>  
> +/* {{{ Encode a Lua table as an MsgPack array */
> +
> +/*
> + * A lot of functions are there, however the task per se looks
> + * simple. Reasons are the following.
> + *
> + * 1. box.tuple.new() supports two parameters conventions.
> + *    <luaT_tuple_encode_values>() implements the old API.
> + * 2. Serializer from Lua to MsgPack may raise a Lua error,
> + *    so it should be run under pcall. The dangerous code is
> + *    encapsulated into <luaT_tuple_encode_table>().
> + * 3. In particular <mpstream_init>() may raise an error in case
> + *    of OOM, so it also is run under pcall.
> + * 4. box.tuple.new() and <luaT_tuple_new>() use shared Lua ibuf
> + *    under the hood (because there is no strong reason to change
> + *    it), while <luaT_tuple_encode>() uses the box region
> + *    (because it is usual for the module API).
> + */
> +
>  /**
>   * Encode a Lua values on a Lua stack as an MsgPack array.
>   *
>   * Raise a Lua error when encoding fails.
> + *
> + * Helper for <lbox_tuple_new>().

1. Should be a part of the previous patch?

>   */
>  static int
>  luaT_tuple_encode_values(struct lua_State *L)
> @@ -122,6 +143,22 @@ luaT_tuple_encode_values(struct lua_State *L)
>  	return 0;
>  }
>  
> +typedef void luaT_mpstream_init_f(struct mpstream *stream, struct lua_State *L);
> +
> +static void
> +luaT_mpstream_init_lua_ibuf(struct mpstream *stream, struct lua_State *L)
> +{
> +	mpstream_init(stream, tarantool_lua_ibuf, ibuf_reserve_cb,
> +		      ibuf_alloc_cb, luamp_error, L);
> +}
> +
> +static void
> +luaT_mpstream_init_box_region(struct mpstream *stream, struct lua_State *L)
> +{
> +	mpstream_init(stream, &fiber()->gc, region_reserve_cb, region_alloc_cb,
> +		      luamp_error, L);

2. If you throw after some allocations are done, will the region leak? I don't
see a truncate in case luaT_tuple_encode_helper fails.

> +}
> +
>  /**
>   * Encode a Lua table or a tuple as MsgPack.
>   *
> @@ -132,25 +169,26 @@ luaT_tuple_encode_values(struct lua_State *L)
>  static int
>  luaT_tuple_encode_table(struct lua_State *L)
>  {
> -	struct ibuf *buf = tarantool_lua_ibuf;
> -	ibuf_reset(buf);
>  	struct mpstream stream;
> -	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
> -		      L);
> -	luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
> +	luaT_mpstream_init_f *luaT_mpstream_init_f = lua_topointer(L, 1);
> +	luaT_mpstream_init_f(&stream, L);
> +	luamp_encode_tuple(L, &tuple_serializer, &stream, 2);
>  	mpstream_flush(&stream);
>  	return 0;
>  }
>  
> -static char *
> -luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
> -			      size_t *tuple_len_ptr)
> +/**
> + * Encode a Lua table / tuple to given mpstream.
> + */
> +static int
> +luaT_tuple_encode_helper(struct lua_State *L, int idx,
> +			 luaT_mpstream_init_f *luaT_mpstream_init_f)

3. Function name is usually a verb. Here I can propose
luaT_tuple_encode_on_mpstream.

>  {
>  	assert(idx != 0);
>  	if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
>  		diag_set(IllegalParams, "A tuple or a table expected, got %s",
>  			 lua_typename(L, lua_type(L, idx)));
> -		return NULL;
> +		return -1;
>  	}
>  
>  	int top = lua_gettop(L);
> @@ -163,18 +201,53 @@ luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
>  	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
>  	assert(lua_isfunction(L, -1));
>  
> +	lua_pushlightuserdata(L, luaT_mpstream_init_f);
>  	lua_pushvalue(L, idx);
>  
> -	int rc = luaT_call(L, 1, 0);
> +	int rc = luaT_call(L, 2, 0);
>  	lua_settop(L, top);
> -	if (rc != 0)
> +	return rc == 0 ? 0 : -1;

4. Why not simply 'return rc;'?

Re: [Tarantool-patches] [PATCH v2 07/15] module api/lua: expose luaT_tuple_new()

[Vladislav Shpilevoy]

Thanks for the patch!

See 1 comment below.

> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
> index aadcf7f59..0c7e8a16f 100644
> --- a/src/box/lua/tuple.h
> +++ b/src/box/lua/tuple.h
> @@ -92,21 +92,35 @@ luaT_istuple(struct lua_State *L, int idx);
>   * should call <box_region_truncate>() to release the data.
>   *
>   * In case of an error set a diag and return NULL.
> + *
> + * @sa luaT_tuple_new()
>   */
>  API_EXPORT char *
>  luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr);
>  
> -/** \endcond public */
> -
>  /**
>   * Create a new tuple with specific format from a Lua table or a
>   * tuple.
>   *
> + * The new tuple is referenced in the same way as one created by
> + * <box_tuple_new>(). There are two possible usage scenarious:
> + *
> + * 1. A short living tuple may not be referenced explicitly and
> + *    will be collected automatically at the next module API call
> + *    that yields or returns a tuple.
> + * 2. A long living tuple must be referenced using
> + *    <box_tuple_ref>() and unreferenced than with

than -> then.

Re: [Tarantool-patches] [PATCH v2 10/15] module api: add box_key_def_new_v2()

[Vladislav Shpilevoy]

Thanks for the patch!

See 5 comments below.

On 11.10.2020 14:57, Alexander Turenko wrote:
> Unlike box_key_def_new() it allows to set nullability, collation and
> JSON path.
> 
> Note: JSON paths are not supported in the backported version of the
> patch for 1.10.
> 
> Provided public non-opaque key part definition structure to create a key
> definition. The next commit will also use this structure to dump a key
> definition.
> 
> There are several technical points around the box_key_part_def_t
> structure. They are mainly about providing stable ABI.
> 
> - Two uint32_t fields are placed first for better aligning of next
>   fields (pointers, which are usually 64 bit wide).
> 
> - A padding is used to guarantee that the structure size will remain the
>   same across tarantool versions. It allows to allocate an array of such
>   structures.
> 
> - The padding array is not a field of the structure itself, but added as
>   a union variant (see the code). It allows to get rid of manual
>   calculation of cumulative fields size, which is hard to do in a
>   platform independent way.
> 
> - A minimal size of the structure is guaranteed by the union with
>   padding, but a static assert is required to guarantee that the size
>   will not overrun the predefined value.
> 
> - PACKED is added as an extra remedy to make the structure layout
>   predictable (on given target architecture).
> 
> - A bit flag is used for is_nullable. bool is considered as too
>   expensive (it requires 8 bits). bitfields (int:1 and so on) do no
>   guarantee certain data layout (it is compiler implementation detail),
>   while a module is compiled outside of tarantool build and may use
>   different toolchain. A bit flag is the only choice.
> 
> - A collation is identified using a string. Different IDs may be used on
>   different tarantool instances for collations. The only 'real'
>   identifier is a collation name, so using it as identifier in the API
>   should be more convenient and less error-prone.

1. What if key_def/merger module is used on client tarantool? For example,
vshard router. It does not store collation structs in the hash you use
to lookup the name. Should we fix that on the clients, to make them fill
the hash?

This kind of issue exists for all schema-dependent values. Mostly for SQL,
which heavily depends on struct space/index/... even during parsing.

> - A field type is also identified using a string instead of a number. We
>   have <enum field_type> in the module API, but it cannot be used here,
>   because IDs are changed across tarantool versions. Aside of this, size
>   of a enum is compiler defined. Anyway, we can expose field types as
>   numbers and implement number-to-name and name-to-number mapping
>   functions, but IMHO it would just add extra complexity.
> 
> The dependency on the fiber compilation unit is added for key_def: it is
> purely to obtain the box region in the module API functions. The key_def
> compilation unit does not use anything fiber related in fact.
> ---
>  src/box/key_def.c                | 141 ++++++++++++++++++
>  src/box/key_def.h                | 144 ++++++++++++++++++-
>  src/exports.h                    |   2 +
>  test/app-tap/module_api.c        | 240 +++++++++++++++++++++++++++++++
>  test/app-tap/module_api.test.lua |   2 +-
>  5 files changed, 527 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/key_def.c b/src/box/key_def.c
> index a03537227..e2bcfe321 100644
> --- a/src/box/key_def.c
> +++ b/src/box/key_def.c
> @@ -341,6 +342,76 @@ key_def_dump_parts(const struct key_def *def, struct key_part_def *parts,
>  	return 0;
>  }
>  
> + /* {{{ Module API helpers */
> +
> +static int
> +key_def_set_internal_part(struct key_part_def *internal_part,
> +			  box_key_part_def_t *part, struct region *region)
> +{
> +	*internal_part = key_part_def_default;
> +
> +	/* Set internal_part->fieldno. */
> +	internal_part->fieldno = part->fieldno;
> +
> +	/* Set internal_part->type. */
> +	if (part->field_type == NULL) {
> +		diag_set(IllegalParams, "Field type is mandatory");
> +		return -1;
> +	}
> +	size_t type_len = strlen(part->field_type);
> +	internal_part->type = field_type_by_name(part->field_type, type_len);
> +	if (internal_part->type == field_type_MAX) {
> +		diag_set(IllegalParams, "Unknown field type: \"%s\"",
> +			 part->field_type);
> +		return -1;
> +	}
> +
> +	/* Set internal_part->{is_nullable,nullable_action}. */
> +	bool is_nullable = (part->flags & BOX_KEY_PART_DEF_IS_NULLABLE) ==
> +		BOX_KEY_PART_DEF_IS_NULLABLE;
> +	if (is_nullable) {
> +		internal_part->is_nullable = is_nullable;
> +		internal_part->nullable_action = ON_CONFLICT_ACTION_NONE;
> +	}
> +
> +	/* Set internal_part->coll_id. */
> +	if (part->collation != NULL) {
> +		size_t collation_len = strlen(part->collation);
> +		struct coll_id *coll_id = coll_by_name(part->collation,
> +						       collation_len);
> +		if (coll_id == NULL) {
> +			diag_set(IllegalParams, "Unknown collation: \"%s\"",
> +				 part->collation);
> +			return -1;
> +		}
> +		internal_part->coll_id = coll_id->id;
> +	}
> +
> +	/* Set internal_part->path (JSON path). */
> +	if (part->path) {

2. Better use != NULL. At least to be consistent with the other code in
in the patch.

> +		size_t path_len = strlen(part->path);
> +		if (json_path_validate(part->path, path_len,
> +				       TUPLE_INDEX_BASE) != 0) {
> +			diag_set(IllegalParams, "Invalid JSON path: \"%s\"",
> +				 part->path);
> +			return -1;
> +		}
> +		char *tmp = region_alloc(region, path_len + 1);
> +		if (tmp == NULL) {
> +			diag_set(OutOfMemory, path_len + 1, "region", "path");
> +			return -1;
> +		}
> diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
> index 184fdf8f8..26853dd9b 100644
> --- a/test/app-tap/module_api.c
> +++ b/test/app-tap/module_api.c
> @@ -365,6 +368,242 @@ test_key_def_api(lua_State *L)
>  	return 1;
>  }
>  
> +/* }}} key_def api */
> +
> +/* {{{ key_def api v2 */
> +
> +/*
> + * More functions around key_def were exposed to the module API
> + * in order to implement external tuple.keydef and tuple.merger
> + * modules (gh-5273, gh-5384).
> + */
> +
> +/**
> + * Verify type and message of an error in the diagnostics area.
> + *
> + * Accepts a prefix of an actual type or a message. Pass an empty
> + * string if you need to verify only type or only message.
> + */
> +static void
> +check_diag(const char *exp_err_type, const char *exp_err_msg)
> +{
> +	box_error_t *e = box_error_last();
> +	assert(!strcmp(box_error_type(e), exp_err_type));
> +	assert(!strcmp(box_error_message(e), exp_err_msg));

3. strcmp() returns not a boolean. Better check for == 0
explicitly. Otherwise !strcmp(a, b) looks like a != b intuitively.
In other patches too.

> +}
> +
> +/**
> + * Create a tuple on runtime arena.
> + *
> + * Release this tuple using <box_tuple_unref>().
> + */
> +static box_tuple_t *
> +new_runtime_tuple(const char *tuple_data, size_t tuple_size)
> +{
> +	box_tuple_format_t *fmt = box_tuple_format_default();
> +	const char *tuple_end = tuple_data + tuple_size;
> +	box_tuple_t *tuple = box_tuple_new(fmt, tuple_data, tuple_end);
> +	assert(tuple != NULL);
> +	box_tuple_ref(tuple);
> +	return tuple;
> +}
> +
> +/**
> + * Where padding bytes starts.
> + */
> +static size_t
> +key_part_padding_offset(void)
> +{
> +	if (sizeof(void *) * CHAR_BIT == 64)
> +		return 32;
> +	if (sizeof(void *) * CHAR_BIT == 32)
> +		return 20;
> +	assert(false);
> +}
> +
> +/**
> + * Mask of all defined flags.
> + */
> +static uint32_t
> +key_part_def_known_flags(void)
> +{
> +	return BOX_KEY_PART_DEF_IS_NULLABLE;
> +}
> +
> +/**
> + * Default flags value.
> + *
> + * All unknown bits are set to zero.
> + */
> +static uint32_t
> +key_part_def_default_flags(void)
> +{
> +	return 0;
> +}
> +
> +/**
> + * Set all <box_key_part_def_t> fields to nondefault values.
> + *
> + * It also set all padding bytes and unknown flags to non-zero
> + * values.
> + */
> +static void
> +key_part_def_set_nondefault(box_key_part_def_t *part)
> +{
> +	size_t padding_offset = key_part_padding_offset();
> +	uint32_t default_flags = key_part_def_default_flags();
> +
> +	/*
> +	 * Give correct non-default values for known fields and
> +	 * flags. Set unknown flags to non-zero values.
> +	 */
> +	part->fieldno = 1;
> +	part->flags = ~default_flags;
> +	part->field_type = "string";
> +	part->collation = "unicode_ci";
> +	part->path = "foo";
> +
> +	/* Fill padding with non-zero bytes. */
> +	char *padding = ((char *) part) + padding_offset;
> +	size_t padding_size = sizeof(box_key_part_def_t) - padding_offset;
> +	memset(padding, 0xff, padding_size);
> +}
> +
> +/**
> + * Verify that all known fields and flags are set to default
> + * values.
> + */
> +static void
> +key_part_def_check_default(box_key_part_def_t *part)
> +{
> +	uint32_t known_flags = key_part_def_known_flags();
> +	uint32_t default_flags = key_part_def_default_flags();
> +
> +	assert(part->fieldno == 0);
> +	assert((part->flags & known_flags) == default_flags);
> +	assert(part->field_type == NULL);
> +	assert(part->collation == NULL);
> +	assert(part->path == NULL);
> +}
> +
> +/**
> + * Verify that all padding bytes and unknown flags are set to
> + * zeros.
> + */
> +static void
> +key_part_def_check_zeros(const box_key_part_def_t *part)
> +{
> +	size_t padding_offset = key_part_padding_offset();
> +	uint32_t unknown_flags = ~key_part_def_known_flags();
> +
> +	char *padding = ((char *) part) + padding_offset;
> +	char *padding_end = ((char *) part) + sizeof(box_key_part_def_t);
> +	for (char *p = padding; p < padding_end; ++p) {
> +		assert(*p == 0);
> +	}
> +
> +	assert((part->flags & unknown_flags) == 0);
> +}
> +
> +/**
> + * Basic <box_key_part_def_create>() and <test_key_def_new_v2>()

4. test_key_def_new_v2 -> box_key_def_new_v2.

> + * test.
> + */
> +static int
> +test_key_def_new_v2(struct lua_State *L)
> +{
> +	/* Verify <box_key_part_def_t> binary layout. */
> +	assert(BOX_KEY_PART_DEF_T_SIZE == 64);
> +	assert(sizeof(box_key_part_def_t) == BOX_KEY_PART_DEF_T_SIZE);
> +	assert(offsetof(box_key_part_def_t, fieldno) == 0);
> +	assert(offsetof(box_key_part_def_t, flags) == 4);
> +	assert(offsetof(box_key_part_def_t, field_type) == 8);
> +	if (sizeof(void *) * CHAR_BIT == 64) {
> +		assert(offsetof(box_key_part_def_t, collation) == 16);
> +		assert(offsetof(box_key_part_def_t, path) == 24);
> +	} else if (sizeof(void *) * CHAR_BIT == 32) {
> +		assert(offsetof(box_key_part_def_t, collation) == 12);
> +		assert(offsetof(box_key_part_def_t, path) == 16);
> +	} else {
> +		assert(false);
> +	}
> +
> +	/*
> +	 * Fill key part definition with nondefault values.
> +	 * Fill padding and unknown flags with non-zero values.
> +	 */
> +	box_key_part_def_t part;
> +	key_part_def_set_nondefault(&part);
> +
> +	/*
> +	 * Verify that all known fields are set to default values and
> +	 * all unknown fields and flags are set to zeros.
> +	 */
> +	box_key_part_def_create(&part);
> +	key_part_def_check_default(&part);
> +	key_part_def_check_zeros(&part);
> +
> +	box_key_def_t *key_def;
> +
> +	/* Should not accept zero part count. */
> +	key_def = box_key_def_new_v2(NULL, 0);
> +	assert(key_def == NULL);
> +	check_diag("IllegalParams", "At least one key part is required");
> +
> +	/* Should not accept NULL as a <field_type>. */
> +	key_def = box_key_def_new_v2(&part, 1);
> +	assert(key_def == NULL);
> +	check_diag("IllegalParams", "Field type is mandatory");

5. Will you test not existing collation, unknown field type, bad JSON path?

> +
> +	/* Success case. */
> +	part.field_type = "unsigned";
> +	key_def = box_key_def_new_v2(&part, 1);
> +	assert(key_def != NULL);
> +
> +	/*
> +	 * Prepare tuples to do some comparisons.
> +	 *
> +	 * [1, 2, 3] and [3, 2, 1].
> +	 */
> +	box_tuple_t *tuple_1 = new_runtime_tuple("\x93\x01\x02\x03", 4);
> +	box_tuple_t *tuple_2 = new_runtime_tuple("\x93\x03\x02\x01", 4);
> +
> +	/*
> +	 * Verify that key_def actually can be used in functions
> +	 * that accepts it.
> +	 *
> +	 * Do several comparisons. Far away from being an
> +	 * exhaustive comparator test.
> +	 */
> +	int rc;
> +	rc = box_tuple_compare(tuple_1, tuple_1, key_def);
> +	assert(rc == 0);
> +	rc = box_tuple_compare(tuple_2, tuple_2, key_def);
> +	assert(rc == 0);
> +	rc = box_tuple_compare(tuple_1, tuple_2, key_def);
> +	assert(rc < 0);
> +	rc = box_tuple_compare(tuple_2, tuple_1, key_def);
> +	assert(rc > 0);
> +
> +	/* The same idea, but perform comparisons against keys. */
> +	rc = box_tuple_compare_with_key(tuple_1, "\x91\x00", key_def);
> +	assert(rc > 0);
> +	rc = box_tuple_compare_with_key(tuple_1, "\x91\x01", key_def);
> +	assert(rc == 0);
> +	rc = box_tuple_compare_with_key(tuple_1, "\x91\x02", key_def);
> +	assert(rc < 0);
> +
> +	/* Clean up. */
> +	box_tuple_unref(tuple_1);
> +	box_tuple_unref(tuple_2);
> +	box_key_def_delete(key_def);
> +
> +	lua_pushboolean(L, 1);
> +	return 1;
> +}

Re: [Tarantool-patches] [PATCH v2 11/15] module api: add box_key_def_dump_parts()

[Vladislav Shpilevoy]

Thanks for the patch!

See 1 comment below.

> diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
> index 26853dd9b..25c98ef63 100644
> --- a/test/app-tap/module_api.c
> +++ b/test/app-tap/module_api.c
> @@ -602,6 +637,86 @@ test_key_def_new_v2(struct lua_State *L)
>  	return 1;
>  }
>  
> +/**
> + * Basic <test_key_def_dump_parts>() test.

test_key_def_dump_parts -> box_key_def_dump_parts.

Re: [Tarantool-patches] [PATCH v2 02/15] module api: expose box region

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> index 16ee9f414..11ca278e5 100644
> --- a/src/lib/core/fiber.h
> +++ b/src/lib/core/fiber.h
> @@ -386,6 +386,82 @@ struct slab_cache;
> +/**
> + * Allocate size bytes from the box region.
> + *
> + * Don't use this function to allocate a memory block for a value
> + * or array of values of a type with alignment requirements. A
> + * violation of alignment requrements leads to undefined

1. requrements -> requirements.

> + * behaviour.
> + */
> +API_EXPORT void *
> +box_region_alloc(size_t size);
> +
> +/**
> + * Allocate size bytes from the box region with given alignment.
> + *
> + * Alignment must be a power of 2.
> + */
> +API_EXPORT void *
> +box_region_aligned_alloc(size_t size, size_t alignment);

2. Do you think we should add a protection against stupidness?
For example, validate that the alignment is actually a power of 2,
at runtime, in release build too? Or leave it on user's
conscience?

> +
> +/**
> + * Truncate the box region to the given size.
> + */
> +void
> +box_region_truncate(size_t size);
> +
>  /** \endcond public */
>  
>  /**
> diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
> index a79fbed0d..b5949cde4 100644
> --- a/test/app-tap/module_api.c
> +++ b/test/app-tap/module_api.c
> @@ -15,6 +15,10 @@
>  #define STR2(x) #x
>  #define STR(x) STR2(x)
>  
> +#ifndef lengthof
> +#define lengthof(array) (sizeof (array) / sizeof ((array)[0]))
> +#endif

3. Extra whitespace after 'sizeof'.

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Igor Munkin]

Sasha,

It is not full review, but Vlad asked me offline regarding a passage
below. So I dump our discussion results.

On 11.10.20, Alexander Turenko wrote:

<snipped>

> +	int top = lua_gettop(L);

This value can't be used, since <lua_rawgeti> can trigger Lua stack
reallocation, ergo this value will be invalidated.

> +
> +	/* Calculate absolute value in the stack. */

At first, it was tough to me to understand the reason you transform the
given relative index to an absolute one, since there is everything
within <lua_pushvalue> for it. I finally got the issue after Vlad's
comments and another (more thorough) look to the sources. I believe it's
nice to drop a few words regarding it. Here are the key points (IMHO):
* whether index is less than zero, it is considered relative to the top
  Lua stack slot
* when you obtain the function object to be called, top pointer is
  incremented, so index ought to be adjusted respectively

> +	if (idx < 0)
> +		idx = top + idx + 1;

Well, is this math even correct? AFAICS, you copy the <idx> slot on the
top as a first argument for <luaT_tuple_encode_table_ref>, right? So,
this is the original guest stack layout:
| nil | <- L->top
| ... |
| val | <- idx
And this is the resulting one:
| nil | <- L->top
| val |
| fun | <- old L->top
| ... |
| val | <- idx

So, it looks like you need to subtract 1 instead of adding it, since
<idx> is negative. Feel free to correct me if I'm bad in this math.

Anyway, technically, you don't need to calculate the absolute value by
yourself, just adjust the offset to the given slot. I guess the
following line is enough (with the verbose comment I mentioned above):
| idx -= idx < 0;

> +
> +	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
> +	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> +	assert(lua_isfunction(L, -1));
> +
> +	lua_pushvalue(L, idx);

There is also another way: simply leave the comment prior to
<lua_pushvalue> call and pass the proper index as an argument
| lua_pushvalue(L, idx - (idx < 0));

> +
> +	int rc = luaT_call(L, 1, 0);

<snipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Igor Munkin]

Well, I looked to the sources once more :)

On 11.10.20, Igor Munkin wrote:
> Sasha,
> 
> It is not full review, but Vlad asked me offline regarding a passage
> below. So I dump our discussion results.
> 
> On 11.10.20, Alexander Turenko wrote:
> 
> <snipped>
> 
> > +	int top = lua_gettop(L);
> 
> This value can't be used, since <lua_rawgeti> can trigger Lua stack
> reallocation, ergo this value will be invalidated.

This is not the address, but offset, so nevermind.

> 
> > +
> > +	/* Calculate absolute value in the stack. */
> 

<snipped>

> 
> > -- 
> > 2.25.0
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 02/15] module api: expose box region

[Alexander Turenko]

> > +/**
> > + * Allocate size bytes from the box region.
> > + *
> > + * Don't use this function to allocate a memory block for a value
> > + * or array of values of a type with alignment requirements. A
> > + * violation of alignment requrements leads to undefined
> 
> 1. requrements -> requirements.

Thanks!

> > +/**
> > + * Allocate size bytes from the box region with given alignment.
> > + *
> > + * Alignment must be a power of 2.
> > + */
> > +API_EXPORT void *
> > +box_region_aligned_alloc(size_t size, size_t alignment);
> 
> 2. Do you think we should add a protection against stupidness?
> For example, validate that the alignment is actually a power of 2,
> at runtime, in release build too? Or leave it on user's
> conscience?

I think that a build time check is enough, considering that the C
standard requires alignment to be a power of 2.

However the comment may be useful. I didn't check whether it is actually
required to be a power of 2 and made attempt to pass an invalid value
from in my test (got an assertion fail). So the comment may be useful.
At least as the extra precaution against inadvertence.

> > +#ifndef lengthof
> > +#define lengthof(array) (sizeof (array) / sizeof ((array)[0]))
> > +#endif
> 
> 3. Extra whitespace after 'sizeof'.

Thanks!

Re: [Tarantool-patches] [PATCH v2 07/15] module api/lua: expose luaT_tuple_new()

[Alexander Turenko]

> > + * The new tuple is referenced in the same way as one created by
> > + * <box_tuple_new>(). There are two possible usage scenarious:
> > + *
> > + * 1. A short living tuple may not be referenced explicitly and
> > + *    will be collected automatically at the next module API call
> > + *    that yields or returns a tuple.
> > + * 2. A long living tuple must be referenced using
> > + *    <box_tuple_ref>() and unreferenced than with
> 
> than -> then.

Thanks!

Re: [Tarantool-patches] [PATCH v2 11/15] module api: add box_key_def_dump_parts()

[Alexander Turenko]

> > +/**
> > + * Basic <test_key_def_dump_parts>() test.
> 
> test_key_def_dump_parts -> box_key_def_dump_parts.

Thanks!

Fixed both such places.

Re: [Tarantool-patches] [PATCH v2 10/15] module api: add box_key_def_new_v2()

[Alexander Turenko]

On Sun, Oct 11, 2020 at 05:25:54PM +0200, Vladislav Shpilevoy wrote:
> 1. What if key_def/merger module is used on client tarantool? For example,
> vshard router. It does not store collation structs in the hash you use
> to lookup the name. Should we fix that on the clients, to make them fill
> the hash?
> 
> This kind of issue exists for all schema-dependent values. Mostly for SQL,
> which heavily depends on struct space/index/... even during parsing.

I don't know that we can do with it on this level.

It looks closer to https://github.com/tarantool/tarantool/issues/3982
('New cluster-wide replication group').

> > +	/* Set internal_part->path (JSON path). */
> > +	if (part->path) {
> 
> 2. Better use != NULL. At least to be consistent with the other code in
> in the patch.

Agree, just inattention.

> > +/**
> > + * Verify type and message of an error in the diagnostics area.
> > + *
> > + * Accepts a prefix of an actual type or a message. Pass an empty
> > + * string if you need to verify only type or only message.
> > + */
> > +static void
> > +check_diag(const char *exp_err_type, const char *exp_err_msg)
> > +{
> > +	box_error_t *e = box_error_last();
> > +	assert(!strcmp(box_error_type(e), exp_err_type));
> > +	assert(!strcmp(box_error_message(e), exp_err_msg));
> 
> 3. strcmp() returns not a boolean. Better check for == 0
> explicitly. Otherwise !strcmp(a, b) looks like a != b intuitively.
> In other patches too.

Agree, just inattention. Fixed over all patches.

> > +
> > +/**
> > + * Basic <box_key_part_def_create>() and <test_key_def_new_v2>()
> 
> 4. test_key_def_new_v2 -> box_key_def_new_v2.

Fixed (as well as the second such place).

> > +	/* Should not accept NULL as a <field_type>. */
> > +	key_def = box_key_def_new_v2(&part, 1);
> > +	assert(key_def == NULL);
> > +	check_diag("IllegalParams", "Field type is mandatory");
> 
> 5. Will you test not existing collation, unknown field type, bad JSON path?

It is already in my backlog: I would do it after the initial work.
Sorry, tight deadlines...

Even without bad cases, I spent a lot of time on tests already.

Re: [Tarantool-patches] [PATCH v2 06/15] WIP: module api/lua: add luaT_tuple_encode()

[Alexander Turenko]

> >  /**
> >   * Encode a Lua values on a Lua stack as an MsgPack array.
> >   *
> >   * Raise a Lua error when encoding fails.
> > + *
> > + * Helper for <lbox_tuple_new>().
> 
> 1. Should be a part of the previous patch?

Sure. Sorry for the mess.

> 
> >   */
> >  static int
> >  luaT_tuple_encode_values(struct lua_State *L)
> > @@ -122,6 +143,22 @@ luaT_tuple_encode_values(struct lua_State *L)
> >  	return 0;
> >  }
> >  
> > +typedef void luaT_mpstream_init_f(struct mpstream *stream, struct lua_State *L);
> > +
> > +static void
> > +luaT_mpstream_init_lua_ibuf(struct mpstream *stream, struct lua_State *L)
> > +{
> > +	mpstream_init(stream, tarantool_lua_ibuf, ibuf_reserve_cb,
> > +		      ibuf_alloc_cb, luamp_error, L);
> > +}
> > +
> > +static void
> > +luaT_mpstream_init_box_region(struct mpstream *stream, struct lua_State *L)
> > +{
> > +	mpstream_init(stream, &fiber()->gc, region_reserve_cb, region_alloc_cb,
> > +		      luamp_error, L);
> 
> 2. If you throw after some allocations are done, will the region leak? I don't
> see a truncate in case luaT_tuple_encode_helper fails.

Good point. Added truncate in luaT_tuple_encode().

> > +/**
> > + * Encode a Lua table / tuple to given mpstream.
> > + */
> > +static int
> > +luaT_tuple_encode_helper(struct lua_State *L, int idx,
> > +			 luaT_mpstream_init_f *luaT_mpstream_init_f)
> 
> 3. Function name is usually a verb. Here I can propose
> luaT_tuple_encode_on_mpstream.

Nice proposal! I'll apply.

> > @@ -163,18 +201,53 @@ luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
> >  	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> >  	assert(lua_isfunction(L, -1));
> >  
> > +	lua_pushlightuserdata(L, luaT_mpstream_init_f);
> >  	lua_pushvalue(L, idx);
> >  
> > -	int rc = luaT_call(L, 1, 0);
> > +	int rc = luaT_call(L, 2, 0);
> >  	lua_settop(L, top);
> > -	if (rc != 0)
> > +	return rc == 0 ? 0 : -1;
> 
> 4. Why not simply 'return rc;'?

Some mess occurs. Thanks for the catch!

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Alexander Turenko]

> > Disallow creating a tuple from objects on the Lua stack (idx == 0) in
> > luaT_tuple_new() for simplicity. There are no such usages in tarantool.
> > The function is not exposed yet to the module API. This is only
> > necessary in box.tuple.new(), which anyway raises Lua errors by its
> > contract.
> 
> 1. But why? The case of creating a tuple from values may be much faster,
> when there is a lot of values not wrapped into a table. Table wrap is
> costly.
> 
> Could you just merge luaT_tuple_encode_values and luaT_tuple_encode_table
> into one function, withuout splitting them?

I started with this variant, but then found that it'll require copying
of all arguments before pcall() (at least if we must leave them on the
stack after exiting from the function). Even if we'll decide to include
a remark like 'the function pops all values in case of idx == 0', we'll
need to put a function before the arguments and so we'll move all stack
values. Anyway it looks lopsided: in one case arguments are popped, but
in another they are kept on the stack.

I guess it would have a chance to be useful if it would allow to pass a
range of lua stack indices. But not sure.

If we'll find it actually useful, nothing prevent us from exposing
luaT_tuple_field_encode() (to control what to encode from a Lua stack in
a user side code). Or consider some other way (even support idx == 0 in
luaT_tuple_encode()): a real use case will help us with designing good
API.

> > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> > index ed97c85e4..18cfef979 100644
> > --- a/src/box/lua/tuple.c
> > +++ b/src/box/lua/tuple.c
> > @@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */
> >  
> >  uint32_t CTID_STRUCT_TUPLE_REF;
> >  
> > +int luaT_tuple_encode_table_ref = LUA_NOREF;
> 
> 2. Better make it static. It is not used outside of this file.

Thanks!

> >  static char *
> >  luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
> >  			      size_t *tuple_len_ptr)
> >  {
> > -	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
> > +	assert(idx != 0);
> > +	if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
> >  		diag_set(IllegalParams, "A tuple or a table expected, got %s",
> >  			 lua_typename(L, lua_type(L, idx)));
> >  		return NULL;
> >  	}
> >  
> > -	struct ibuf *buf = tarantool_lua_ibuf;
> > -	ibuf_reset(buf);
> > -	struct mpstream stream;
> > -	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
> > -		      luamp_error, L);
> > -	if (idx == 0) {
> > -		/*
> > -		 * Create the tuple from lua stack
> > -		 * objects.
> > -		 */
> > -		int argc = lua_gettop(L);
> > -		mpstream_encode_array(&stream, argc);
> > -		for (int k = 1; k <= argc; ++k) {
> > -			luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
> > -		}
> > -	} else {
> > -		/* Create the tuple from a Lua table. */
> > -		luamp_encode_tuple(L, &tuple_serializer, &stream, idx);
> > -	}
> > -	mpstream_flush(&stream);
> > +	int top = lua_gettop(L);
> > +
> > +	/* Calculate absolute value in the stack. */
> > +	if (idx < 0)
> > +		idx = top + idx + 1;> +
> > +	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
> > +	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> > +	assert(lua_isfunction(L, -1));
> > +
> > +	lua_pushvalue(L, idx);
> > +
> > +	int rc = luaT_call(L, 1, 0);
> > +	lua_settop(L, top);
> > +	if (rc != 0)
> > +		return NULL;
> > +
> >  	if (tuple_len_ptr != NULL)
> > -		*tuple_len_ptr = ibuf_used(buf);
> > -	return buf->buf;
> > +		*tuple_len_ptr = ibuf_used(tarantool_lua_ibuf);
> 
> 3. Since you assume the function at luaT_tuple_encode_table_ref alwaus
> uses ibuf, maybe it would be better to add 'on_lua_ibuf' to its name.

The next commit passes mpstream initialization function to it and I want
to avoid double renaming to make diffs more readable.

> Or move the length calculation out of the current function, because
> this one already has 'on_lua_ibuf', so it wouldn't look so strange to
> get size of tarantool_lua_ibuf after it.

Don't get a purpose of the proposed changes.

Anyway, for defense of given variant, the next commit adds
luaT_tuple_encode() with quite similar behaviour and signature and it
looks nice, when those functions are virtually same, but based on top of
different allocators.

> > @@ -156,15 +198,30 @@ lbox_tuple_new(lua_State *L)
> >  		lua_newtable(L); /* create an empty tuple */
> >  		++argc;
> >  	}
> > +
> > +	box_tuple_format_t *fmt = box_tuple_format_default();
> > +
> 
> 4. You could keep it after the comment below to reduce the diff.

Heh. Good spot. I'll do.

> >  	/*
> >  	 * Use backward-compatible parameters format:
> > -	 * box.tuple.new(1, 2, 3) (idx == 0), or the new one:
> > -	 * box.tuple.new({1, 2, 3}) (idx == 1).
> > +	 * box.tuple.new(1, 2, 3).
> >  	 */
> > -	int idx = argc == 1 && (lua_istable(L, 1) ||
> > -		luaT_istuple(L, 1));
> > -	box_tuple_format_t *fmt = box_tuple_format_default();
> > -	struct tuple *tuple = luaT_tuple_new(L, idx, fmt);
> > +	if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
> > +		struct ibuf *buf = tarantool_lua_ibuf;
> > +		luaT_tuple_encode_values(L); /* may raise */
> 
> 5. We usually put comments on separate line.

 | /* May raise. */
 | luaT_tuple_encode_values(L);

...would make unclear whether it applies to just next call or several
ones. We can wrap the call and the comment with empty lines above and
below, but it is too much for such small thing. Or we can explicitly
point to the call in the comment:

 | /* The next call may raise. */
 | luaT_tuple_encode_values(L);

However such comment sometimes displaced by inaccurate commits and
befomes useless. So we should mention the function name explicitly, but
it, in turn, looks too tautological.

I'm aware about the convention, but I still think that one-two word
remarks are complelety okay to be placed inline. I don't exploit this
much.

I'll change it somehow, if it disturbs (and you'll highlight it), but
personally I think it is okay.

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Alexander Turenko]

It seems we look at this code from some very different positions. I have
my patterns in the mind and you have your ones.

WBR, Alexander Turenko.

On Sun, Oct 11, 2020 at 08:47:55PM +0300, Igor Munkin wrote:
> Sasha,
> 
> It is not full review, but Vlad asked me offline regarding a passage
> below. So I dump our discussion results.
> 
> On 11.10.20, Alexander Turenko wrote:
> 
> <snipped>
> 
> > +	int top = lua_gettop(L);
> 
> This value can't be used, since <lua_rawgeti> can trigger Lua stack
> reallocation, ergo this value will be invalidated.

AFAIU, you rejected this comment in the next email?

 | This is not the address, but offset, so nevermind.

> 
> > +
> > +	/* Calculate absolute value in the stack. */
> 
> At first, it was tough to me to understand the reason you transform the
> given relative index to an absolute one, since there is everything
> within <lua_pushvalue> for it. I finally got the issue after Vlad's
> comments and another (more thorough) look to the sources. I believe it's
> nice to drop a few words regarding it. Here are the key points (IMHO):
> * whether index is less than zero, it is considered relative to the top
>   Lua stack slot
> * when you obtain the function object to be called, top pointer is
>   incremented, so index ought to be adjusted respectively
> 
> > +	if (idx < 0)
> > +		idx = top + idx + 1;
> 
> Well, is this math even correct? AFAICS, you copy the <idx> slot on the
> top as a first argument for <luaT_tuple_encode_table_ref>, right? So,
> this is the original guest stack layout:
> | nil | <- L->top
> | ... |
> | val | <- idx
> And this is the resulting one:
> | nil | <- L->top
> | val |
> | fun | <- old L->top
> | ... |
> | val | <- idx
> 
> So, it looks like you need to subtract 1 instead of adding it, since
> <idx> is negative. Feel free to correct me if I'm bad in this math.
> 
> Anyway, technically, you don't need to calculate the absolute value by
> yourself, just adjust the offset to the given slot. I guess the
> following line is enough (with the verbose comment I mentioned above):
> | idx -= idx < 0;

Hm. Hmmm.

The math is correct. We have the linear dependency, so it seems we can
just verify one negative idx and others should be good too for any valid
composition of idx and top.

top: 4; idx: -1 -> 4 -- ok
top: 4; idx: -2 -> 3 -- ok

This snippet is used several times across tarantool code base: say, in
luaL_checkcdata().

Let's show the sketchy code:

 | int top = gettop(L);
 | <some stack manipulations>
 | int rc = lua_pcall(<...>);
 | lua_settop(L, top);
 | <handle rc>

It is much, MUCH better than doing all those lua_pop(L, 1) or
lua_pop(L, 2) depending on lua_pcall() return value.

Now, look at another schetchy code:

 | int idx = absolute(idx);
 | <doing some stack manipulations>
 | lua_pushvalue(L, idx);

It again much, MUCH better than doing all those idx + 1 or -1 or even -2
depending on how top is changed.

> 
> > +
> > +	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
> > +	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> > +	assert(lua_isfunction(L, -1));
> > +
> > +	lua_pushvalue(L, idx);
> 
> There is also another way: simply leave the comment prior to
> <lua_pushvalue> call and pass the proper index as an argument
> | lua_pushvalue(L, idx - (idx < 0));

It'll be <idx - 2 * (idx < 0)> in the next commit. I don't want to play
this game and still think that it is much better to just use an
absolute index.

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Igor Munkin]

Sasha,

On 12.10.20, Alexander Turenko wrote:
> It seems we look at this code from some very different positions. I have
> my patterns in the mind and you have your ones.

Totally agree here.

> 
> WBR, Alexander Turenko.
> 
> On Sun, Oct 11, 2020 at 08:47:55PM +0300, Igor Munkin wrote:
> > Sasha,
> > 

<snipped>

> 
> AFAIU, you rejected this comment in the next email?
> 
>  | This is not the address, but offset, so nevermind.

Yes, I did.

> 
> > 
> > > +
> > > +	/* Calculate absolute value in the stack. */
> > 
> > At first, it was tough to me to understand the reason you transform the
> > given relative index to an absolute one, since there is everything
> > within <lua_pushvalue> for it. I finally got the issue after Vlad's
> > comments and another (more thorough) look to the sources. I believe it's
> > nice to drop a few words regarding it. Here are the key points (IMHO):
> > * whether index is less than zero, it is considered relative to the top
> >   Lua stack slot
> > * when you obtain the function object to be called, top pointer is
> >   incremented, so index ought to be adjusted respectively

I hope, we are on the same page here, aren't we?

> > 
> > > +	if (idx < 0)
> > > +		idx = top + idx + 1;
> > 
> > Well, is this math even correct? AFAICS, you copy the <idx> slot on the
> > top as a first argument for <luaT_tuple_encode_table_ref>, right? So,
> > this is the original guest stack layout:
> > | nil | <- L->top
> > | ... |
> > | val | <- idx
> > And this is the resulting one:
> > | nil | <- L->top
> > | val |
> > | fun | <- old L->top
> > | ... |
> > | val | <- idx
> > 
> > So, it looks like you need to subtract 1 instead of adding it, since
> > <idx> is negative. Feel free to correct me if I'm bad in this math.
> > 
> > Anyway, technically, you don't need to calculate the absolute value by
> > yourself, just adjust the offset to the given slot. I guess the
> > following line is enough (with the verbose comment I mentioned above):
> > | idx -= idx < 0;
> 
> Hm. Hmmm.
> 
> The math is correct. We have the linear dependency, so it seems we can
> just verify one negative idx and others should be good too for any valid
> composition of idx and top.
> 
> top: 4; idx: -1 -> 4 -- ok
> top: 4; idx: -2 -> 3 -- ok

I re-checked these calculations manually, so both approaches are fine.

> 
> This snippet is used several times across tarantool code base: say, in
> luaL_checkcdata().
> 
> Let's show the sketchy code:
> 
>  | int top = gettop(L);
>  | <some stack manipulations>
>  | int rc = lua_pcall(<...>);
>  | lua_settop(L, top);
>  | <handle rc>
> 
> It is much, MUCH better than doing all those lua_pop(L, 1) or
> lua_pop(L, 2) depending on lua_pcall() return value.
> 
> Now, look at another schetchy code:
> 
>  | int idx = absolute(idx);
>  | <doing some stack manipulations>
>  | lua_pushvalue(L, idx);
> 
> It again much, MUCH better than doing all those idx + 1 or -1 or even -2
> depending on how top is changed.
> 
> > 
> > > +
> > > +	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
> > > +	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> > > +	assert(lua_isfunction(L, -1));
> > > +
> > > +	lua_pushvalue(L, idx);
> > 
> > There is also another way: simply leave the comment prior to
> > <lua_pushvalue> call and pass the proper index as an argument
> > | lua_pushvalue(L, idx - (idx < 0));
> 
> It'll be <idx - 2 * (idx < 0)> in the next commit. I don't want to play
> this game and still think that it is much better to just use an
> absolute index.

I don't want to argue about which approach is *much better*. This is the
only spot I can agree with you. Other ones above are just based on the
patterns we have in our minds. However, despite the patters we have in
our minds, such non-trivial (at least to me) places should be described
with a nice comment.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Timur Safin]

The hottest discussions are always about silly style preferrences,
and here I couldn't resist to not get involved as well...

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua
: error from luaT_tuple_new()
: 
...
: > > +	if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
: > > +		struct ibuf *buf = tarantool_lua_ibuf;
: > > +		luaT_tuple_encode_values(L); /* may raise */
: >
: > 5. We usually put comments on separate line.
: 
:  | /* May raise. */
:  | luaT_tuple_encode_values(L);
: 
: ...would make unclear whether it applies to just next call or several
: ones. We can wrap the call and the comment with empty lines above and
: below, but it is too much for such small thing. Or we can explicitly
: point to the call in the comment:
: 
:  | /* The next call may raise. */
:  | luaT_tuple_encode_values(L);
: 
: However such comment sometimes displaced by inaccurate commits and
: befomes useless. So we should mention the function name explicitly, but
: it, in turn, looks too tautological.
: 
: I'm aware about the convention, but I still think that one-two word
: remarks are complelety okay to be placed inline. I don't exploit this
: much.
: 
: I'll change it somehow, if it disturbs (and you'll highlight it), but
: personally I think it is okay.

I'm totally with Sasha here - this comment as being put to the right of 
a function it comments on - is the most appropriate usage case, IMVHO.

Regards,
Timur

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Alexander Turenko]

> > > > +	/* Calculate absolute value in the stack. */
> > > 
> > > At first, it was tough to me to understand the reason you transform the
> > > given relative index to an absolute one, since there is everything
> > > within <lua_pushvalue> for it. I finally got the issue after Vlad's
> > > comments and another (more thorough) look to the sources. I believe it's
> > > nice to drop a few words regarding it. Here are the key points (IMHO):
> > > * whether index is less than zero, it is considered relative to the top
> > >   Lua stack slot
> > > * when you obtain the function object to be called, top pointer is
> > >   incremented, so index ought to be adjusted respectively
> 
> I hope, we are on the same page here, aren't we?

Not sure I understood your proposal right. Anyway, I'll leave short
descriptions regarding ideas behind the code. I hope it is better now.

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 6cccb0ce1..8c3d29f71 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -158,9 +158,13 @@ luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
 		return NULL;
 	}
 
+	/* To restore before leaving the function. */
 	int top = lua_gettop(L);
 
-	/* Calculate absolute value in the stack. */
+	/*
+	 * An absolute index doesn't need to be recalculated after
+	 * the stack size change.
+	 */
 	if (idx < 0)
 		idx = top + idx + 1;

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Vladislav Shpilevoy]

On 12.10.2020 12:37, Alexander Turenko wrote:
>>> Disallow creating a tuple from objects on the Lua stack (idx == 0) in
>>> luaT_tuple_new() for simplicity. There are no such usages in tarantool.
>>> The function is not exposed yet to the module API. This is only
>>> necessary in box.tuple.new(), which anyway raises Lua errors by its
>>> contract.
>>
>> 1. But why? The case of creating a tuple from values may be much faster,
>> when there is a lot of values not wrapped into a table. Table wrap is
>> costly.
>>
>> Could you just merge luaT_tuple_encode_values and luaT_tuple_encode_table
>> into one function, withuout splitting them?
> 
> I started with this variant, but then found that it'll require copying
> of all arguments before pcall() (at least if we must leave them on the
> stack after exiting from the function). Even if we'll decide to include
> a remark like 'the function pops all values in case of idx == 0', we'll
> need to put a function before the arguments and so we'll move all stack
> values. Anyway it looks lopsided: in one case arguments are popped, but
> in another they are kept on the stack.
> 
> I guess it would have a chance to be useful if it would allow to pass a
> range of lua stack indices. But not sure.

I created an issue for that: https://github.com/tarantool/tarantool/issues/5406
(At least it is related, and this place is not the only problematic one.)

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Alexander Turenko]

On Thu, Oct 15, 2020 at 01:41:45AM +0200, Vladislav Shpilevoy wrote:
> On 12.10.2020 12:37, Alexander Turenko wrote:
> >>> Disallow creating a tuple from objects on the Lua stack (idx == 0) in
> >>> luaT_tuple_new() for simplicity. There are no such usages in tarantool.
> >>> The function is not exposed yet to the module API. This is only
> >>> necessary in box.tuple.new(), which anyway raises Lua errors by its
> >>> contract.
> >>
> >> 1. But why? The case of creating a tuple from values may be much faster,
> >> when there is a lot of values not wrapped into a table. Table wrap is
> >> costly.
> >>
> >> Could you just merge luaT_tuple_encode_values and luaT_tuple_encode_table
> >> into one function, withuout splitting them?
> > 
> > I started with this variant, but then found that it'll require copying
> > of all arguments before pcall() (at least if we must leave them on the
> > stack after exiting from the function). Even if we'll decide to include
> > a remark like 'the function pops all values in case of idx == 0', we'll
> > need to put a function before the arguments and so we'll move all stack
> > values. Anyway it looks lopsided: in one case arguments are popped, but
> > in another they are kept on the stack.
> > 
> > I guess it would have a chance to be useful if it would allow to pass a
> > range of lua stack indices. But not sure.
> 
> I created an issue for that: https://github.com/tarantool/tarantool/issues/5406
> (At least it is related, and this place is not the only problematic one.)

Hmm. I don't see how it is related to the question.

Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Vladislav Shpilevoy]

On 15.10.2020 21:43, Alexander Turenko wrote:
> On Thu, Oct 15, 2020 at 01:41:45AM +0200, Vladislav Shpilevoy wrote:
>> On 12.10.2020 12:37, Alexander Turenko wrote:
>>>>> Disallow creating a tuple from objects on the Lua stack (idx == 0) in
>>>>> luaT_tuple_new() for simplicity. There are no such usages in tarantool.
>>>>> The function is not exposed yet to the module API. This is only
>>>>> necessary in box.tuple.new(), which anyway raises Lua errors by its
>>>>> contract.
>>>>
>>>> 1. But why? The case of creating a tuple from values may be much faster,
>>>> when there is a lot of values not wrapped into a table. Table wrap is
>>>> costly.
>>>>
>>>> Could you just merge luaT_tuple_encode_values and luaT_tuple_encode_table
>>>> into one function, withuout splitting them?
>>>
>>> I started with this variant, but then found that it'll require copying
>>> of all arguments before pcall() (at least if we must leave them on the
>>> stack after exiting from the function). Even if we'll decide to include
>>> a remark like 'the function pops all values in case of idx == 0', we'll
>>> need to put a function before the arguments and so we'll move all stack
>>> values. Anyway it looks lopsided: in one case arguments are popped, but
>>> in another they are kept on the stack.
>>>
>>> I guess it would have a chance to be useful if it would allow to pass a
>>> range of lua stack indices. But not sure.
>>
>> I created an issue for that: https://github.com/tarantool/tarantool/issues/5406
>> (At least it is related, and this place is not the only problematic one.)
> 
> Hmm. I don't see how it is related to the question.

When the issue is fixed, we will be able to serialized a range of values,
without them being places on top of the stack. So it will make it faster
to call

	box.tuple.new(1, 2, 3, 4, 5)

than

	box.tuple.new({1, 2, 3, 4, 5})