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

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

[Alexander Turenko]

A lot of code to review... but nobody will do it better than you, Vlad!
Please, look when your time will permit.

This patchset extends the module API with a set of functions (mainly
key_def and tuple related) in order to implement an external key_def Lua
module on top of them.

Timur will do the same for functions he need for the external merger
module (outside of this patchset, but on top of it).

I don't wrote necessary tests ATM, so the patchset is not ready to push
and marked as RFC (request for comments). However I did my best for all
other aspects and decided to send the patchset for review to collect a
feedback earlier. I'll work on tests while waiting for review. (I
promise I'll not ask to push it until all tests will be written.)

There is backport of this patchset to 1.10 (see the links below). It
adds two extra commits and reworks some of those fourteen ones. Please,
consider it as part of this work and take glance too (I can send it too
if there will be demand).

Regarding those two extra commits:

- d7c8a7888 collation: allow to find a collation by a name

  The alternative would be a function like [1], but direct call to the
  database from key_def_api.c looks strange for me. I think it is better
  to backport the hashmap by collation names: the patch is simple and
  proven by the time.

  A long time ago we agreed with Vladimir Davydov to backport this patch
  to 1.10 if we'll backport merger to 1.10. It didn't occur, but the
  situation is similar.

- 078239e37 refactoring: adjust contract of luaT_tuple_new()

  Here I'm in doubts.

  The alternative would be duplicate the code: rename old
  luaT_tuple_new() to somewhat like luaT_tuple_new_110() and add the new
  luaT_tuple_new() to expose it into the module API. I don't know, to
  be honest. The code is proven by time and the risk is minimal. I would
  prefer to avoid code duplication, but I'm not strict on this position
  here: I'll implement the alternative if there will be reasoned
  complains against the current variant.

What also worries me: I found that actual behaviour of luaT_tuple_new()
is different from what I expected in the past: it may raise an error
when serialization fails. I didn't expect this, so I'll look how much it
may affect key_def and merger modules (built-in and external ones). For
now I just described the actual contract in the API comments for
luaT_tuple_new() and luaT_tuple_encode().

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 14 commits)
https://github.com/tarantool/tarantool/tree/Totktonada/gh-5273-expand-module-api-1.10
(top 16 commits)

The module based on this API:

https://github.com/Totktonada/key_def

The module repository will be moved to the tarantool organization when
it'll be ready (surely not before this patchset). Please, consider the
module as PoC (everything work, though; even on Mac OS).

References:

[1]: https://github.com/tarantool/tarantool/blob/b7c787152d8984523d0b8b4d5b08f79173c57115/src/lua/merger.c#L1686-L1708

Alexander Turenko (14):
  module api: get rid of typedef redefinitions
  WIP: module api: expose box region
  WIP: module api/lua: add luaL_iscdata() function
  WIP: module api/lua: expose luaT_tuple_new()
  WIP: module api/lua: add luaT_tuple_encode()
  WIP: refactoring: add API_EXPORT to lua/tuple functions
  WIP: refactoring: add API_EXPORT to key_def functions
  WIP: refactoring: extract key_def module API functions
  WIP: module api: add box_key_def_new_ex()
  WIP: module api: add box_key_def_dump_parts()
  WIP: module api: expose box_tuple_validate_key_parts()
  WIP: module api: expose box_key_def_merge()
  WIP: module api: expose box_tuple_extract_key_ex()
  WIP: module api: add box_key_def_validate_key()

 src/CMakeLists.txt              |   7 +-
 src/box/CMakeLists.txt          |   1 +
 src/box/field_map.h             |   4 +
 src/box/index.h                 |   5 +-
 src/box/key_def.c               |  62 ++----
 src/box/key_def.h               |  85 +++------
 src/box/key_def_api.c           | 323 ++++++++++++++++++++++++++++++++
 src/box/key_def_api.h           | 295 +++++++++++++++++++++++++++++
 src/box/lua/tuple.c             |  21 ++-
 src/box/lua/tuple.h             |  56 +++++-
 src/exports.h                   |  14 ++
 src/lib/core/fiber.c            |  24 +++
 src/lib/core/fiber.h            | 101 ++++++++++
 src/lua/utils.c                 |   6 +
 src/lua/utils.h                 |  20 ++
 test/app-tap/CMakeLists.txt     |   6 +
 test/app-tap/module_api.c       |   2 +
 test/unit/vy_iterators_helper.c |   1 +
 test/unit/vy_mem.c              |   1 +
 test/unit/vy_point_lookup.c     |   1 +
 test/unit/vy_write_iterator.c   |   1 +
 21 files changed, 915 insertions(+), 121 deletions(-)
 create mode 100644 src/box/key_def_api.c
 create mode 100644 src/box/key_def_api.h

-- 
2.25.0

[Tarantool-patches] [PATCH 01/14] 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 ee67cf533..f11ffacff 100644
--- a/test/app-tap/CMakeLists.txt
+++ b/test/app-tap/CMakeLists.txt
@@ -1 +1,7 @@
 build_module(module_api module_api.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 02/14] WIP: 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.

XXX: Write a test for the new module API calls.

[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 | 101 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

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 483ae3ce1..0ef7eed43 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -1358,6 +1358,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..618ba8045 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -386,6 +386,107 @@ 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.
+ *
+ * Another useful property is that data allocated on this region
+ * are freed eventually, so a cold path may omit
+ * <box_region_truncate>() call: it will not lead to a memory
+ * leak. It is not recommended to exploit this property on a hot
+ * path, because of the risk to exhaust available memory for this
+ * region before it'll be freed.
+ *
+ * @sa <fiber_gc>() (internal function)
+ */
+
+/** How much memory is used by the box region. */
+API_EXPORT size_t
+box_region_used(void);
+
+/** Allocate size bytes from the box region. */
+API_EXPORT void *
+box_region_alloc(size_t size);
+
+/**
+ * Allocate size bytes from the box region with given alingment.
+ */
+API_EXPORT void *
+box_region_aligned_alloc(size_t size, size_t alignment);
+
+/*
+ * Allocate storage for an object of given type with respect to
+ * its alignment.
+ *
+ * @param T    type of an object
+ * @param size where to store size of an object (size_t *)
+ */
+#define box_region_alloc_object(T, size) ({					\
+	*(size) = sizeof(T);							\
+	(T *)box_region_aligned_alloc(sizeof(T), alignof(T));			\
+})
+
+/*
+ * Allocate storage for array of objects of given type.
+ *
+ * @param T     type of an object
+ * @param count amount of objects to allocate (size_t)
+ * @param size  where to store size of an object (size_t *)
+ */
+#define box_region_alloc_array(T, count, size) ({				\
+	int _tmp_ = sizeof(T) * (count);					\
+	*(size) = _tmp_;							\
+	(T *)box_region_aligned_alloc(_tmp_, alignof(T));			\
+})
+
+/**
+ * Truncate the box region to the given size.
+ */
+void
+box_region_truncate(size_t size);
+
 /** \endcond public */
 
 /**
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 2 comments below.

> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> index 16ee9f414..618ba8045 100644
> --- a/src/lib/core/fiber.h
> +++ b/src/lib/core/fiber.h
> @@ -386,6 +386,107 @@ 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.
> + *
> + * Another useful property is that data allocated on this region
> + * are freed eventually, so a cold path may omit
> + * <box_region_truncate>() call: it will not lead to a memory
> + * leak. It is not recommended to exploit this property on a hot
> + * path, because of the risk to exhaust available memory for this
> + * region before it'll be freed.

1. That looks like a dangerous advice. We do truncate the region in some
places, but that may change any moment. For example, fiber_gc() in
the transactions may be replaced with
region_truncate(<size_at_transaction_start>). I would better say, that
if you allocated something, or get something allocated implicitly from
another module.h function, you shall free it. You can't rely on it be
freed automatically. If you use it in a hot path to allocate a small
number of objects, and box_region_truncate() really becomes a bottleneck,
you may need to reconsider design of your allocation policy, and use
something else but the region.

> + *
> + * @sa <fiber_gc>() (internal function)
> + */
> +
> +/** How much memory is used by the box region. */
> +API_EXPORT size_t
> +box_region_used(void);
> +
> +/** Allocate size bytes from the box region. */
> +API_EXPORT void *
> +box_region_alloc(size_t size);
> +
> +/**
> + * Allocate size bytes from the box region with given alingment.
> + */
> +API_EXPORT void *
> +box_region_aligned_alloc(size_t size, size_t alignment);
> +
> +/*
> + * Allocate storage for an object of given type with respect to
> + * its alignment.
> + *
> + * @param T    type of an object
> + * @param size where to store size of an object (size_t *)
> + */
> +#define box_region_alloc_object(T, size) ({					\
> +	*(size) = sizeof(T);							\
> +	(T *)box_region_aligned_alloc(sizeof(T), alignof(T));			\
> +})
> +
> +/*
> + * Allocate storage for array of objects of given type.
> + *
> + * @param T     type of an object
> + * @param count amount of objects to allocate (size_t)
> + * @param size  where to store size of an object (size_t *)
> + */
> +#define box_region_alloc_array(T, count, size) ({				\
> +	int _tmp_ = sizeof(T) * (count);					\
> +	*(size) = _tmp_;							\
> +	(T *)box_region_aligned_alloc(_tmp_, alignof(T));			\
> +})

2. Nah, lets better drop this sugar. If somebody wants them, they can
implement them in their own code. It is not something what needs to
be in module.h, and IMO we need to keep here only really necessary
things.

> +
> +/**
> + * Truncate the box region to the given size.
> + */
> +void
> +box_region_truncate(size_t size);
> +
>  /** \endcond public */
>  
>  /**
> 

Re: [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region

[Alexander Turenko]

On Fri, Sep 25, 2020 at 12:31:58AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 2 comments below.
> 
> > diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> > index 16ee9f414..618ba8045 100644
> > --- a/src/lib/core/fiber.h
> > +++ b/src/lib/core/fiber.h
> > @@ -386,6 +386,107 @@ 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.
> > + *
> > + * Another useful property is that data allocated on this region
> > + * are freed eventually, so a cold path may omit
> > + * <box_region_truncate>() call: it will not lead to a memory
> > + * leak. It is not recommended to exploit this property on a hot
> > + * path, because of the risk to exhaust available memory for this
> > + * region before it'll be freed.
> 
> 1. That looks like a dangerous advice. We do truncate the region in some
> places, but that may change any moment. For example, fiber_gc() in
> the transactions may be replaced with
> region_truncate(<size_at_transaction_start>). I would better say, that
> if you allocated something, or get something allocated implicitly from
> another module.h function, you shall free it. You can't rely on it be
> freed automatically. If you use it in a hot path to allocate a small
> number of objects, and box_region_truncate() really becomes a bottleneck,
> you may need to reconsider design of your allocation policy, and use
> something else but the region.

Re the dangerous advice itself: I agree and will remove it in the next
patchset version. We cannot guarantee that fiber_gc() will be called
periodically in future and even now it highly depends on a workload.

Re 'you shall free it': it is already described at top of the
description (with examples).

Regarding 'if <...> box_region_truncate() really becomes a bottleneck':
I think that 'It is useful for allocating tons of small objects and free
them at once' at top of the description is enough.

> > +/*
> > + * Allocate storage for an object of given type with respect to
> > + * its alignment.
> > + *
> > + * @param T    type of an object
> > + * @param size where to store size of an object (size_t *)
> > + */
> > +#define box_region_alloc_object(T, size) ({					\
> > +	*(size) = sizeof(T);							\
> > +	(T *)box_region_aligned_alloc(sizeof(T), alignof(T));			\
> > +})
> > +
> > +/*
> > + * Allocate storage for array of objects of given type.
> > + *
> > + * @param T     type of an object
> > + * @param count amount of objects to allocate (size_t)
> > + * @param size  where to store size of an object (size_t *)
> > + */
> > +#define box_region_alloc_array(T, count, size) ({				\
> > +	int _tmp_ = sizeof(T) * (count);					\
> > +	*(size) = _tmp_;							\
> > +	(T *)box_region_aligned_alloc(_tmp_, alignof(T));			\
> > +})
> 
> 2. Nah, lets better drop this sugar. If somebody wants them, they can
> implement them in their own code. It is not something what needs to
> be in module.h, and IMO we need to keep here only really necessary
> things.

I looked at this once again and now I agree. My points:

1. We anyway should describe when box_region_alloc() and
   box_region_aligned_alloc() should be used, because otherwise it is
   easy to step into UB by violation of a structure alignment rules.
2. box_region_alloc_array() does not check the multiplication for
   overflow (and stores the result in int, not even size_t). But
   calloc() checks it and the expectation of a developer may be that our
   function do the same.

I'll update the comment to box_region_alloc() with the following warning
about alignment in the next patchset version:

 | /**
 |  * 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);

[Tarantool-patches] [PATCH 03/14] WIP: 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.

XXX: Add a test to test/app-tap/module_api.{c,test.lua}.

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

diff --git a/src/exports.h b/src/exports.h
index 7861bb529..08ca03507 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -355,6 +355,7 @@ EXPORT(luaL_fileresult)
 EXPORT(luaL_findtable)
 EXPORT(luaL_getmetafield)
 EXPORT(luaL_gsub)
+EXPORT(luaL_iscdata)
 EXPORT(luaL_iscallable)
 EXPORT(luaL_loadbuffer)
 EXPORT(luaL_loadbufferx)
diff --git a/src/lua/utils.c b/src/lua/utils.c
index af114b0a2..bf5452d3e 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -112,6 +112,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..e9dd27d08 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -78,6 +78,26 @@ luaL_pushuuid(struct lua_State *L);
 
 /** \cond public */
 
+/**
+ * @brief 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 stack index
+ *
+ * @return 1 if the value at the given acceptable index is a cdata
+ * and 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
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/exports.h b/src/exports.h
> index 7861bb529..08ca03507 100644
> --- a/src/exports.h
> +++ b/src/exports.h
> @@ -355,6 +355,7 @@ EXPORT(luaL_fileresult)
>  EXPORT(luaL_findtable)
>  EXPORT(luaL_getmetafield)
>  EXPORT(luaL_gsub)
> +EXPORT(luaL_iscdata)
>  EXPORT(luaL_iscallable)

1. 'd' > 'a', so luaL_isc*d*ata should come after luaL_isc*a*llable.

>  EXPORT(luaL_loadbuffer)
>  EXPORT(luaL_loadbufferx)
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index 7e02a05f2..e9dd27d08 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -78,6 +78,26 @@ luaL_pushuuid(struct lua_State *L);
>  
>  /** \cond public */
>  
> +/**
> + * @brief 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 stack index

2. I am going to go OCD here, but lets start sentences from a capital
letter and end with a dot. Here and in all the other commits in
their places.

> + *
> + * @return 1 if the value at the given acceptable index is a cdata
> + * and 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
> 

Re: [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function

[Alexander Turenko]

> > diff --git a/src/exports.h b/src/exports.h
> > index 7861bb529..08ca03507 100644
> > --- a/src/exports.h
> > +++ b/src/exports.h
> > @@ -355,6 +355,7 @@ EXPORT(luaL_fileresult)
> >  EXPORT(luaL_findtable)
> >  EXPORT(luaL_getmetafield)
> >  EXPORT(luaL_gsub)
> > +EXPORT(luaL_iscdata)
> >  EXPORT(luaL_iscallable)
> 
> 1. 'd' > 'a', so luaL_isc*d*ata should come after luaL_isc*a*llable.

You're machine!

Sure, will fix in the next patchset version.

> 
> >  EXPORT(luaL_loadbuffer)
> >  EXPORT(luaL_loadbufferx)
> > diff --git a/src/lua/utils.h b/src/lua/utils.h
> > index 7e02a05f2..e9dd27d08 100644
> > --- a/src/lua/utils.h
> > +++ b/src/lua/utils.h
> > @@ -78,6 +78,26 @@ luaL_pushuuid(struct lua_State *L);
> >  
> >  /** \cond public */
> >  
> > +/**
> > + * @brief 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 stack index
> 
> 2. I am going to go OCD here, but lets start sentences from a capital
> letter and end with a dot. Here and in all the other commits in
> their places.

Okay.

[Tarantool-patches] [PATCH 04/14] WIP: 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).

XXX: Add a module API test. There is unit test already, so maybe just
verify some basic case in the module API test. Or, better, transform the
unit test into a module API test.

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

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 03b4b8a2e..7baab467b 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -98,7 +98,7 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-struct tuple *
+box_tuple_t *
 luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
 {
 	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
@@ -127,8 +127,8 @@ 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));
+	box_tuple_t *tuple = box_tuple_new(format, buf->buf,
+					   buf->buf + ibuf_used(buf));
 	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 fd280f565..766275ff2 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -79,8 +79,6 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
 box_tuple_t *
 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.
@@ -88,11 +86,27 @@ luaT_istuple(struct lua_State *L, int idx);
  * Set idx to zero to create the new tuple from objects on the lua
  * stack.
  *
- * In case of an error set a diag and return NULL.
+ * 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()
+ *
+ * If encoding fails, raise an error.
+ *
+ * In case of any other 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 08ca03507..9ebb082c8 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_new)
 EXPORT(mp_char2escape)
 EXPORT(mp_decode_double)
 EXPORT(mp_decode_extl)
-- 
2.25.0

[Tarantool-patches] [PATCH 05/14] 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 Lua shared ibuf (<tarantool_lua_ibuf>).

The reason to introduce 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).

XXX: Add a module API test.

Part of #5273
---
 src/box/lua/tuple.c | 21 +++++++++++++++++----
 src/box/lua/tuple.h | 29 +++++++++++++++++++++++++++++
 src/exports.h       |  1 +
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 7baab467b..843458870 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -98,8 +98,8 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-box_tuple_t *
-luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
+char *
+luaT_tuple_encode(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 +127,21 @@ 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);
-	box_tuple_t *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;
+
+}
+
+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(L, idx, &tuple_len);
+	if (tuple_data == NULL)
+		return NULL;
+	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 766275ff2..759df0b89 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -79,6 +79,35 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
 box_tuple_t *
 luaT_istuple(struct lua_State *L, int idx);
 
+/**
+ * Encode a Lua table, a tuple or objects on a Lua stack as raw
+ * tuple data (MsgPack).
+ *
+ * @param L             Lua state
+ * @param idx           acceptable index on the Lua stack
+ *                      (or zero, see below)
+ * @param tuple_len_ptr where to store tuple data size in bytes
+ *                      (or NULL)
+ *
+ * Set idx to zero to create the new tuple from objects on the Lua
+ * stack.
+ *
+ * The data is encoded on the shared buffer: so called
+ * <tarantool_lua_ibuf> (it also available as <buffer.SHARED_IBUF>
+ * in Lua). The data is valid until next similar call. It is
+ * generally safe to pass the result to a box function (copy it if
+ * doubt). No need to release this buffer explicitly, it'll be
+ * reused by later calls.
+ *
+ * If encoding fails, raise an error.
+ *
+ * In case of any other 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);
+
 /**
  * Create a new tuple with specific format from a Lua table, a
  * tuple, or objects on the lua stack.
diff --git a/src/exports.h b/src/exports.h
index 9ebb082c8..10baefa6e 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(luaT_tuple_new)
 EXPORT(mp_char2escape)
 EXPORT(mp_decode_double)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode()

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

I will pause the review of the next patches until I get
back to work tomorrow/after tomorrow.

> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
> index 766275ff2..759df0b89 100644
> --- a/src/box/lua/tuple.h
> +++ b/src/box/lua/tuple.h
> @@ -79,6 +79,35 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
>  box_tuple_t *
>  luaT_istuple(struct lua_State *L, int idx);
>  
> +/**
> + * Encode a Lua table, a tuple or objects on a Lua stack as raw
> + * tuple data (MsgPack).
> + *
> + * @param L             Lua state
> + * @param idx           acceptable index on the Lua stack
> + *                      (or zero, see below)
> + * @param tuple_len_ptr where to store tuple data size in bytes
> + *                      (or NULL)
> + *
> + * Set idx to zero to create the new tuple from objects on the Lua
> + * stack.
> + *
> + * The data is encoded on the shared buffer: so called
> + * <tarantool_lua_ibuf> (it also available as <buffer.SHARED_IBUF>

1. Both are private. I wouldn't mention them. I think it is enough to
say, that the allocation is 'internal', and may be freed by any next
lua-related call.

> + * in Lua). The data is valid until next similar call. It is
> + * generally safe to pass the result to a box function (copy it if
> + * doubt). No need to release this buffer explicitly, it'll be
> + * reused by later calls.

2. Why exactly are we doing this export? I need a remainder. I remember
we discussed if we could return the encoded tuple on a region, but
decided it could be too slow, because could lead to oscillation of the
region's slab if the region would be truncated after every call.

Also I remember we discussed if we could expose the ibuf too. Because it
is a nice allocator to allocate contiguous memory blocks but without
an oscillation. Then we looked at it and found, that it actually
oscillates. Because we call ibuf_reinit, which puts the slab back to the
cache.

That makes me wonder why not use the region, if both suck at oscillation,
but at least we exposed and documented the region already.

If you worry about code duplication, I think it wouldn't be that terrible.
luaT_tuple_encode() uses mpstream. So most of it could be moved to a new
common static function inside lua/tuple.c which takes an mpstream, and
stores Lua objects into it. And 2 wrappers on top - one creates mpstream
from an ibuf, another from the region.

What I do worry about - the function may throw, and something would be left
on the region memory. Is it critical? Can't it happen with like any other
allocator too, so the problem is not really in the region itself?

> + *
> + * If encoding fails, raise an error.
> + *
> + * In case of any other 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);

Re: [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode()

[Alexander Turenko]

> > +/**
> > + * Encode a Lua table, a tuple or objects on a Lua stack as raw
> > + * tuple data (MsgPack).
> > + *
> > + * @param L             Lua state
> > + * @param idx           acceptable index on the Lua stack
> > + *                      (or zero, see below)
> > + * @param tuple_len_ptr where to store tuple data size in bytes
> > + *                      (or NULL)
> > + *
> > + * Set idx to zero to create the new tuple from objects on the Lua
> > + * stack.
> > + *
> > + * The data is encoded on the shared buffer: so called
> > + * <tarantool_lua_ibuf> (it also available as <buffer.SHARED_IBUF>
> 
> 1. Both are private. I wouldn't mention them. I think it is enough to
> say, that the allocation is 'internal', and may be freed by any next
> lua-related call.

It is not more actual: I changed luaT_tuple_encode() to use the box
region.

> 
> > + * in Lua). The data is valid until next similar call. It is
> > + * generally safe to pass the result to a box function (copy it if
> > + * doubt). No need to release this buffer explicitly, it'll be
> > + * reused by later calls.
> 
> 2. Why exactly are we doing this export? I need a remainder. I remember
> we discussed if we could return the encoded tuple on a region, but
> decided it could be too slow, because could lead to oscillation of the
> region's slab if the region would be truncated after every call.

Not exactly. I doubt about changing the existing user visible function
from ibuf to region: box.tuple.new(), but I'm okay with exposing the new
one that is initially based on the region allocator. The reason is that
I unable to provide a good reason for such change for an existing
function. Maybe it'll degrade by performance in some cases. Maybe not.
Anyway, there is no reason to change the allocator under box.tuple.new()
(except our internal coding problems).

Maybe I worry more than necessary. I don't know, so I would go by the
more safe pathway.

> 
> Also I remember we discussed if we could expose the ibuf too. Because it
> is a nice allocator to allocate contiguous memory blocks but without
> an oscillation. Then we looked at it and found, that it actually
> oscillates. Because we call ibuf_reinit, which puts the slab back to the
> cache.

Not exactly. It is seems, it is just mistake on the caller side:
ibuf_reinit() is called instead of ibuf_reset(). It is in my backlog,
I'll file an issue and fix it when time will permit. So ibuf itself is
out of suspicions.

> 
> That makes me wonder why not use the region, if both suck at oscillation,
> but at least we exposed and documented the region already.

This arguments works for me. It seems, we usually lean on the region in
the public APIs, so it would be better to continue to do so and keep
some consistency. At least it is easier to understand and use.

> 
> If you worry about code duplication, I think it wouldn't be that terrible.
> luaT_tuple_encode() uses mpstream. So most of it could be moved to a new
> common static function inside lua/tuple.c which takes an mpstream, and
> stores Lua objects into it. And 2 wrappers on top - one creates mpstream
> from an ibuf, another from the region.

Not exactly so: mpstream initialization also may raise an error. But,
yep, it is still possible to do some code unification.

It does not look nice, but at least not so disgusting as code
duplication.

> 
> What I do worry about - the function may throw, and something would be left
> on the region memory. Is it critical? Can't it happen with like any other
> allocator too, so the problem is not really in the region itself?

I filed #5382 regarding this and fixed it within the patchset.

[Tarantool-patches] [PATCH 06/14] WIP: refactoring: add API_EXPORT to lua/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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index 759df0b89..f3fc84d00 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" {
@@ -56,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);
 
 /**
@@ -65,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);
 
 /**
@@ -76,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 07/14] WIP: refactoring: 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 08/14] WIP: refactoring: extract key_def module API functions

[Alexander Turenko]

I plan to expand module API for key_def in further commits and mix of
public and private structures and functions becomes hard to read. So it
looks meaningful to extract module API wrappers into its own compilation
unit.

Added libtuple.a to the so called reexport libraries list, because
otherwise the functions from key_def_api compilation unit are not
exported on 1.10 (in the backported patch).

Part of #5273
---
 src/CMakeLists.txt              |   4 +-
 src/box/CMakeLists.txt          |   1 +
 src/box/key_def.c               |  62 ++++----------------
 src/box/key_def.h               |  86 ++++++++++-----------------
 src/box/key_def_api.c           |  79 +++++++++++++++++++++++++
 src/box/key_def_api.h           | 101 ++++++++++++++++++++++++++++++++
 test/unit/vy_iterators_helper.c |   1 +
 test/unit/vy_mem.c              |   1 +
 test/unit/vy_point_lookup.c     |   1 +
 test/unit/vy_write_iterator.c   |   1 +
 10 files changed, 228 insertions(+), 109 deletions(-)
 create mode 100644 src/box/key_def_api.c
 create mode 100644 src/box/key_def_api.h

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 699536652..89e5bbc34 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -145,7 +145,7 @@ set(api_headers
     ${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/key_def_api.h
     ${CMAKE_SOURCE_DIR}/src/box/lua/key_def.h
     ${CMAKE_SOURCE_DIR}/src/box/field_def.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple_format.h
@@ -179,7 +179,7 @@ target_link_libraries(server core coll http_parser bit uri uuid swim swim_udp
 # Rule of thumb: if exporting a symbol from a static library, list the
 # library here.
 set (reexport_libraries server core misc bitset csv swim swim_udp swim_ev
-     ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} ${ICU_LIBRARIES})
+     tuple ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} ${ICU_LIBRARIES})
 
 set (common_libraries
     ${reexport_libraries}
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index b8b2689d2..7d8170f93 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -104,6 +104,7 @@ add_library(tuple STATIC
     tuple_bloom.c
     tuple_dictionary.c
     key_def.c
+    key_def_api.c
     coll_id_def.c
     coll_id.c
     coll_id_cache.c
diff --git a/src/box/key_def.c b/src/box/key_def.c
index a03537227..fc1666eff 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -146,7 +146,7 @@ key_def_delete(struct key_def *def)
 	free(def);
 }
 
-static void
+void
 key_def_set_func(struct key_def *def)
 {
 	key_def_set_compare_func(def);
@@ -341,56 +341,6 @@ key_def_dump_parts(const struct key_def *def, struct key_part_def *parts,
 	return 0;
 }
 
-box_key_def_t *
-box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
-{
-	size_t sz = key_def_sizeof(part_count, 0);
-	struct key_def *key_def = calloc(1, sz);
-	if (key_def == NULL) {
-		diag_set(OutOfMemory, sz, "malloc", "struct key_def");
-		return NULL;
-	}
-
-	key_def->part_count = part_count;
-	key_def->unique_part_count = part_count;
-
-	for (uint32_t item = 0; item < part_count; ++item) {
-		if (key_def_set_part(key_def, item, fields[item],
-				     (enum field_type)types[item],
-				     ON_CONFLICT_ACTION_DEFAULT, NULL,
-				     COLL_NONE, SORT_ORDER_ASC, NULL, 0, NULL,
-				     TUPLE_OFFSET_SLOT_NIL, 0) != 0) {
-			key_def_delete(key_def);
-			return NULL;
-		}
-	}
-	key_def_set_func(key_def);
-	return key_def;
-}
-
-void
-box_key_def_delete(box_key_def_t *key_def)
-{
-	key_def_delete(key_def);
-}
-
-int
-box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
-		  box_key_def_t *key_def)
-{
-	return tuple_compare(tuple_a, HINT_NONE, tuple_b, HINT_NONE, key_def);
-}
-
-int
-box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
-			   box_key_def_t *key_def)
-{
-	uint32_t part_count = mp_decode_array(&key_b);
-	return tuple_compare_with_key(tuple_a, HINT_NONE, key_b,
-				      part_count, HINT_NONE, key_def);
-
-}
-
 int
 key_part_cmp(const struct key_part *parts1, uint32_t part_count1,
 	     const struct key_part *parts2, uint32_t part_count2)
@@ -892,3 +842,13 @@ key_validate_parts(const struct key_def *key_def, const char *key,
 	*key_end = key;
 	return 0;
 }
+
+int
+key_def_set_part_175(struct key_def *def, uint32_t part_no, uint32_t fieldno,
+		     enum field_type type)
+{
+	return key_def_set_part(def, part_no, fieldno, type,
+				ON_CONFLICT_ACTION_DEFAULT, NULL, COLL_NONE,
+				SORT_ORDER_ASC, NULL, 0, NULL,
+				TUPLE_OFFSET_SLOT_NIL, 0);
+}
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 625bb6fea..a4be3aef7 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -287,62 +287,6 @@ 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;
-
-/**
- * Create key definition with key fields with passed typed on passed positions.
- * May be used for tuple format creation and/or tuple comparison.
- *
- * \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
- * \returns a new key definition object
- */
-API_EXPORT box_key_def_t *
-box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
-
-/**
- * Delete key definition
- *
- * \param key_def key definition to delete
- */
-API_EXPORT void
-box_key_def_delete(box_key_def_t *key_def);
-
-/**
- * Compare tuples using the key definition.
- * @param tuple_a first tuple
- * @param tuple_b second tuple
- * @param key_def key definition
- * @retval 0  if key_fields(tuple_a) == key_fields(tuple_b)
- * @retval <0 if key_fields(tuple_a) < key_fields(tuple_b)
- * @retval >0 if key_fields(tuple_a) > key_fields(tuple_b)
- */
-API_EXPORT int
-box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
-		  box_key_def_t *key_def);
-
-/**
- * @brief Compare tuple with key using the key definition.
- * @param tuple tuple
- * @param key key with MessagePack array header
- * @param key_def key definition
- *
- * @retval 0  if key_fields(tuple) == parts(key)
- * @retval <0 if key_fields(tuple) < parts(key)
- * @retval >0 if key_fields(tuple) > parts(key)
- */
-
-API_EXPORT int
-box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
-			   box_key_def_t *key_def);
-
-/** \endcond public */
-
 static inline size_t
 key_def_sizeof(uint32_t part_count, uint32_t path_pool_size)
 {
@@ -755,6 +699,36 @@ key_hint(const char *key, uint32_t part_count, struct key_def *key_def)
 	return key_def->key_hint(key, part_count, key_def);
 }
 
+/* {{{ <box_key_def_new>() helpers */
+
+/**
+ * Set key part within @a key_def.
+ *
+ * The same as module private <key_def_set_part>(), but with less
+ * parameters. It is helper for <box_key_def_new>().
+ *
+ * 1.7.5 does not support collation, nullability and further
+ * key_def features.
+ */
+int
+key_def_set_part_175(struct key_def *def, uint32_t part_no, uint32_t fieldno,
+		     enum field_type type);
+
+/**
+ * Update compare, hash and extract functions.
+ *
+ * This function should be called after modification of
+ * @a key_def parts, because different compare, hash and extract
+ * functions should work depending of whether key parts are
+ * sequential, are nullable, whether a tuple may omit several
+ * fields at the end, whether a collation is used and so on.
+ * It is helper for <box_key_def_new>().
+ */
+void
+key_def_set_func(struct key_def *def);
+
+/* }}} <box_key_def_new>() helpers */
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
new file mode 100644
index 000000000..7f6c0ac55
--- /dev/null
+++ b/src/box/key_def_api.c
@@ -0,0 +1,79 @@
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include "key_def_api.h"
+#include "key_def.h"
+
+box_key_def_t *
+box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
+{
+	size_t sz = key_def_sizeof(part_count, 0);
+	struct key_def *key_def = calloc(1, sz);
+	if (key_def == NULL) {
+		diag_set(OutOfMemory, sz, "malloc", "struct key_def");
+		return NULL;
+	}
+
+	key_def->part_count = part_count;
+	key_def->unique_part_count = part_count;
+
+	for (uint32_t item = 0; item < part_count; ++item) {
+		if (key_def_set_part_175(key_def, item, fields[item],
+					 (enum field_type)types[item]) != 0) {
+			key_def_delete(key_def);
+			return NULL;
+		}
+	}
+	key_def_set_func(key_def);
+	return key_def;
+}
+
+void
+box_key_def_delete(box_key_def_t *key_def)
+{
+	key_def_delete(key_def);
+}
+
+int
+box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
+		  box_key_def_t *key_def)
+{
+	return tuple_compare(tuple_a, HINT_NONE, tuple_b, HINT_NONE, key_def);
+}
+
+int
+box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
+			   box_key_def_t *key_def)
+{
+	uint32_t part_count = mp_decode_array(&key_b);
+	return tuple_compare_with_key(tuple_a, HINT_NONE, key_b,
+				      part_count, HINT_NONE, key_def);
+
+}
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
new file mode 100644
index 000000000..5b1c861f5
--- /dev/null
+++ b/src/box/key_def_api.h
@@ -0,0 +1,101 @@
+#ifndef TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
+#define TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+#include <stdint.h>
+#include "trivia/util.h"
+
+typedef struct tuple box_tuple_t;
+
+/** \cond public */
+
+typedef struct key_def box_key_def_t;
+
+/**
+ * Create key definition with given field numbers and field types.
+ *
+ * May be used for tuple format creation and/or tuple comparison.
+ *
+ * \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
+ * \returns a new key definition object
+ */
+API_EXPORT box_key_def_t *
+box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
+
+/**
+ * Delete key definition
+ *
+ * \param key_def key definition to delete
+ */
+API_EXPORT void
+box_key_def_delete(box_key_def_t *key_def);
+
+/**
+ * Compare tuples using the key definition.
+ * @param tuple_a first tuple
+ * @param tuple_b second tuple
+ * @param key_def key definition
+ * @retval 0  if key_fields(tuple_a) == key_fields(tuple_b)
+ * @retval <0 if key_fields(tuple_a) < key_fields(tuple_b)
+ * @retval >0 if key_fields(tuple_a) > key_fields(tuple_b)
+ */
+API_EXPORT int
+box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
+		  box_key_def_t *key_def);
+
+/**
+ * @brief Compare tuple with key using the key definition.
+ * @param tuple tuple
+ * @param key key with MessagePack array header
+ * @param key_def key definition
+ *
+ * @retval 0  if key_fields(tuple) == parts(key)
+ * @retval <0 if key_fields(tuple) < parts(key)
+ * @retval >0 if key_fields(tuple) > parts(key)
+ */
+API_EXPORT int
+box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
+			   box_key_def_t *key_def);
+
+/** \endcond public */
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_BOX_KEY_API_DEF_H_INCLUDED */
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index 0d20f19ef..1897d341c 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -1,3 +1,4 @@
+#include "key_def_api.h"
 #include "vy_iterators_helper.h"
 #include "memory.h"
 #include "fiber.h"
diff --git a/test/unit/vy_mem.c b/test/unit/vy_mem.c
index 2e461f312..1ad6c9641 100644
--- a/test/unit/vy_mem.c
+++ b/test/unit/vy_mem.c
@@ -1,6 +1,7 @@
 #include <trivia/config.h>
 #include "memory.h"
 #include "fiber.h"
+#include "key_def_api.h"
 #include "vy_history.h"
 #include "vy_iterators_helper.h"
 
diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c
index fb075c578..e55f0edd9 100644
--- a/test/unit/vy_point_lookup.c
+++ b/test/unit/vy_point_lookup.c
@@ -1,5 +1,6 @@
 #include "trivia/util.h"
 #include "unit.h"
+#include "key_def_api.h"
 #include "vy_lsm.h"
 #include "vy_cache.h"
 #include "vy_run.h"
diff --git a/test/unit/vy_write_iterator.c b/test/unit/vy_write_iterator.c
index 97fb2df37..173eb600c 100644
--- a/test/unit/vy_write_iterator.c
+++ b/test/unit/vy_write_iterator.c
@@ -1,5 +1,6 @@
 #include "memory.h"
 #include "fiber.h"
+#include "key_def_api.h"
 #include "vy_write_iterator.h"
 #include "vy_iterators_helper.h"
 
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 2 comments below.

On 23.09.2020 03:14, Alexander Turenko wrote:
> I plan to expand module API for key_def in further commits and mix of
> public and private structures and functions becomes hard to read. So it
> looks meaningful to extract module API wrappers into its own compilation
> unit.

1. I don't think it is a good idea. Not only it is inconsistent with other
modules such as tuple and fiber, but also it makes not possible to use
key_def.c static functions, and you need to start moving things to the
header to be able to use them. In the result, the code becomes harder
to follow.

If you want to separate public and non-public methods, you may consider
grouping them inside the same file. But moving to a new file does not
look good. Also the new file name looks somewhat 'ugly'. You are
basically trying to re-invent module.h and the way it is built but
per-submodule.

> Added libtuple.a to the so called reexport libraries list, because
> otherwise the functions from key_def_api compilation unit are not
> exported on 1.10 (in the backported patch).
> 
> Part of #5273
> ---
> diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> new file mode 100644
> index 000000000..5b1c861f5
> --- /dev/null
> +++ b/src/box/key_def_api.h
> @@ -0,0 +1,101 @@
> +#ifndef TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
> +#define TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED

2. Lets use '#pragma once'.

> +/*
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */

Re: [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions

[Alexander Turenko]

> On 23.09.2020 03:14, Alexander Turenko wrote:
> > I plan to expand module API for key_def in further commits and mix of
> > public and private structures and functions becomes hard to read. So it
> > looks meaningful to extract module API wrappers into its own compilation
> > unit.
> 
> 1. I don't think it is a good idea. Not only it is inconsistent with other
> modules such as tuple and fiber, but also it makes not possible to use
> key_def.c static functions, and you need to start moving things to the
> header to be able to use them. In the result, the code becomes harder
> to follow.
> 
> If you want to separate public and non-public methods, you may consider
> grouping them inside the same file. But moving to a new file does not
> look good. Also the new file name looks somewhat 'ugly'. You are
> basically trying to re-invent module.h and the way it is built but
> per-submodule.

Now I remember my primary motivation to splitting key_def and
key_def_api (sorry that I didn't described it in the commit message: I
forgot about it). My wish was to don't depend on the fiber compilation
unit to obtain &fiber()->gc (the box region) in key_def.c.

However the root of the problem is that the box region is exposed from
the fiber compilation unit. So I agree: it should be resolved in some
other way.

As Vlad pointed me, a dependency on the core library is okay for a
compilation unit of the box library in general.

I'll use grouping inside key_def.[ch] the next patchset version.

> 
> > Added libtuple.a to the so called reexport libraries list, because
> > otherwise the functions from key_def_api compilation unit are not
> > exported on 1.10 (in the backported patch).
> > 
> > Part of #5273
> > ---
> > diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> > new file mode 100644
> > index 000000000..5b1c861f5
> > --- /dev/null
> > +++ b/src/box/key_def_api.h
> > @@ -0,0 +1,101 @@
> > +#ifndef TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
> > +#define TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
> 
> 2. Lets use '#pragma once'.

 | Use header guards.

https://www.tarantool.io/en/doc/latest/dev_guide/c_style_guide/

Rules are rules.

You know, I don't like '#pragra once', because it works in non-obvious
way, had various bugs in the past and may slow down compilation ([1]).
So I will follow the accepted style.

(Anyway, I'm going to drop this file and move everything to key_def.h as
you suggested.)

[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58770

Re: [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions

[Vladislav Shpilevoy]

Hi! Thanks for the answers!

>>> Added libtuple.a to the so called reexport libraries list, because
>>> otherwise the functions from key_def_api compilation unit are not
>>> exported on 1.10 (in the backported patch).
>>>
>>> Part of #5273
>>> ---
>>> diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
>>> new file mode 100644
>>> index 000000000..5b1c861f5
>>> --- /dev/null
>>> +++ b/src/box/key_def_api.h
>>> @@ -0,0 +1,101 @@
>>> +#ifndef TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
>>> +#define TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
>>
>> 2. Lets use '#pragma once'.
> 
>  | Use header guards.
> 
> https://www.tarantool.io/en/doc/latest/dev_guide/c_style_guide/
> 
> Rules are rules.
> 
> You know, I don't like '#pragra once', because it works in non-obvious
> way, had various bugs in the past and may slow down compilation ([1]).
> So I will follow the accepted style.

Then you should use pragma, because they are our rule. I don't believe
you don't remember that discussion. I remember it was assigned to Kirill
to update the docs, since he approved that style change, but it seems he
just didn't.

[Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex()

[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 techinal 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.

- 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.

XXX: Add a module API test.

Part of #5273
---
 src/box/key_def_api.c | 146 ++++++++++++++++++++++++++++++++++++++++++
 src/box/key_def_api.h | 123 +++++++++++++++++++++++++++++++++++
 src/exports.h         |   2 +
 3 files changed, 271 insertions(+)

diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 7f6c0ac55..19590095d 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -30,6 +30,89 @@
  */
 #include "key_def_api.h"
 #include "key_def.h"
+#include "small/region.h"
+#include "json/json.h"
+#include "coll_id_cache.h"
+#include "tuple_format.h"
+#include "field_def.h"
+#include "coll_id_cache.h"
+#include "fiber.h"
+
+/* {{{ 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_MASK) ==
+		BOX_KEY_PART_DEF_IS_NULLABLE_MASK;
+	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;
+}
+
+/* }}} Helpers */
+
+/* {{{ API functions implementations */
+
+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(uint32_t *fields, uint32_t *types, uint32_t part_count)
@@ -55,6 +138,67 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
 	return key_def;
 }
 
+box_key_def_t *
+box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count)
+{
+	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 (parts == NULL) {
+		diag_set(OutOfMemory, internal_parts_size, "region_alloc_array",
+			 "parts");
+		return NULL;
+	}
+	if (part_count == 0) {
+		diag_set(IllegalParams, "At least one key part is required");
+		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_MASK) ==
+			BOX_KEY_PART_DEF_IS_NULLABLE_MASK;
+		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)
 {
@@ -77,3 +221,5 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
 				      part_count, HINT_NONE, key_def);
 
 }
+
+/* }}} API functions implementations */
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index 5b1c861f5..328a58c70 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -44,11 +44,104 @@ typedef struct tuple box_tuple_t;
 
 typedef struct key_def box_key_def_t;
 
+/** Key part definition flags. */
+enum {
+	BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT = 0,
+	BOX_KEY_PART_DEF_IS_NULLABLE_MASK =
+		1 << BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT,
+};
+
+/**
+ * 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_ex>() 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,
+};
+
+/**
+ * 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_ex>() 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_ex>().
+ *
+ * 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_ex>().
+ *
  * \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
@@ -57,6 +150,28 @@ 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.
+ *
+ * All trailing padding bytes are set to zero.
+ *
+ * All unknown <flags> bits are set to zero.
+ */
+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_ex(box_key_part_def_t *parts, uint32_t part_count);
+
 /**
  * Delete key definition
  *
@@ -94,6 +209,14 @@ 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)");
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/exports.h b/src/exports.h
index 10baefa6e..80dd952c7 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_ex)
+EXPORT(box_key_part_def_create)
 EXPORT(box_latch_delete)
 EXPORT(box_latch_lock)
 EXPORT(box_latch_new)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex()

[Vladislav Shpilevoy]

Thanks for the patch!

See 12 comments below.

1. Why _ex()? Why not 2? What are we going to do when will need a new
parameter? _ex_ex()? _ex2()? With a number at least we could just bump
it.

On 23.09.2020 03:14, 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.

2. Did you think of making a function which would create a key_def taking
only part_count, and then exporting functions like

    box_key_def_set_part_type(...);
    box_key_def_set_part_json_path(...);
    box_key_def_set_part_collation(...);
    etc.

to customize it after creation? Then we wouldn't need to export
box_key_part_def_t and care about ABI shit.

> There are several techinal points around the box_key_part_def_t

3. techinal -> techincal.

> 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.

4. Is pointer size predictable?

> - 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.

5. For record - I still don't like the strings. Because it is not
obvious what to specify here. There is no a set of values to use, and it
makes the creation slower.

It simplifies API, but not module.h. It simplifies the Lua module you
built on top of this. For pure C modules and for the module.h itself it
complicates things.

But I do not like it not enough to insist.

> XXX: Add a module API test.
> 
> Part of #5273
> ---
>  src/box/key_def_api.c | 146 ++++++++++++++++++++++++++++++++++++++++++
>  src/box/key_def_api.h | 123 +++++++++++++++++++++++++++++++++++
>  src/exports.h         |   2 +
>  3 files changed, 271 insertions(+)
> 
> diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
> index 7f6c0ac55..19590095d 100644
> --- a/src/box/key_def_api.c
> +++ b/src/box/key_def_api.c
> @@ -55,6 +138,67 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
>  	return key_def;
>  }
>  
> +box_key_def_t *
> +box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count)
> +{
> +	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 (parts == NULL) {

6. I assume you wanted to check for internal_parts == NULL.

> +		diag_set(OutOfMemory, internal_parts_size, "region_alloc_array",
> +			 "parts");
> +		return NULL;
> +	}
> +	if (part_count == 0) {

7. internal_parts leaks here? I don't even know if region allocates
anything when size is 0. I propose to move the check before the
allocation.

> +		diag_set(IllegalParams, "At least one key part is required");
> +		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_MASK) ==
> +			BOX_KEY_PART_DEF_IS_NULLABLE_MASK;

8. You have internal_parts[i].is_nullable for that.

> +		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.

9. Just do that and if all works, then it is fine to change. Looks like
a bug which didn't have a place to explode until now.

> +	 */
> +	key_def_update_optionality(key_def, min_field_count);
> +
> +	return key_def;
> +}
> +
>  void
>  box_key_def_delete(box_key_def_t *key_def)
> diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> index 5b1c861f5..328a58c70 100644
> --- a/src/box/key_def_api.h
> +++ b/src/box/key_def_api.h
> @@ -44,11 +44,104 @@ typedef struct tuple box_tuple_t;
>  
>  typedef struct key_def box_key_def_t;
>  
> +/** Key part definition flags. */
> +enum {
> +	BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT = 0,
> +	BOX_KEY_PART_DEF_IS_NULLABLE_MASK =
> +		1 << BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT,

10. Does not it seem a little overcomplicated to you? Why
do you need BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT? Why not
call BOX_KEY_PART_DEF_IS_NULLABLE_MASK
BOX_KEY_PART_DEF_IS_NULLABLE_FLAG? Or
BOX_KEY_PART_DEF_IS_NULLABLE.

> +};
> +
> +/**
> + * 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_ex>() 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,
> +};
> +
> +/**
> + * 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_ex>() 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_ex>().
> + *
> + * 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;

11. You know, we could also export a function like box_key_def_part_size(),
which would return sizeof(key_part_def). Then the module would need
to call it to allocate the part. And then use functions box_key_def_set_part_*()
to set the part def fields. So the struct would be opaque, could be
allocated in an array, and would be ABI stable. However probably it is
the same what I proposed in the beginning of this email but with extra
steps.

> +
>  /**
>   * 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_ex>().
> + *
>   * \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
> @@ -57,6 +150,28 @@ 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.
> + *
> + * All trailing padding bytes are set to zero.

12. Why does it need to touch the padding? How is it
related to ABI?

> + *
> + * All unknown <flags> bits are set to zero.
> + */
> +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_ex(box_key_part_def_t *parts, uint32_t part_count);

Re: [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex()

[Alexander Turenko]

> 1. Why _ex()? Why not 2? What are we going to do when will need a new
> parameter? _ex_ex()? _ex2()? With a number at least we could just bump
> it.

_ex() and _v2() have certain meaning (extended, updated version), but
_2() may be interpreted differently: two parameters (like wait3() /
wait4()), second version (update of the first one), another variant (not
update, but just the same thing with some other properties).

'How to name the next version when we'll need to bump it' is the good
question. _ex() does not give us good advice. So it seems we should
prefer _v2() in the module API.

I'll change it.

> 
> On 23.09.2020 03:14, 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.
> 
> 2. Did you think of making a function which would create a key_def taking
> only part_count, and then exporting functions like
> 
>     box_key_def_set_part_type(...);
>     box_key_def_set_part_json_path(...);
>     box_key_def_set_part_collation(...);
>     etc.
> 
> to customize it after creation? Then we wouldn't need to export
> box_key_part_def_t and care about ABI shit.

Of course. I would even say that this variant have no strict blockers
considering my use case. However I would want to avoid the situation,
when an API design lead to performance restrictions in areas, where it
can be bypassed. And also I think that an opaque structure pointer + set
of accessor functions just to access fields is not how usual C API is
designed. Example: getaddrinfo() and <struct addrinfo>.

Sure, an ABI needs care, but I hope that when we have defined
modification rules, which should guarantee ABI compatibility, further
maintanence should not be hard.

If you don't have strict objections, I would go this way.

> 
> > There are several techinal points around the box_key_part_def_t
> 
> 3. techinal -> techincal.

Will fix.

> 
> > 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.
> 
> 4. Is pointer size predictable?

I'll add 'on given target architecture' here in the next patchset
version.

> 
> > - 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.
> 
> 5. For record - I still don't like the strings. Because it is not
> obvious what to specify here. There is no a set of values to use, and it
> makes the creation slower.
> 
> It simplifies API, but not module.h. It simplifies the Lua module you
> built on top of this. For pure C modules and for the module.h itself it
> complicates things.
> 
> But I do not like it not enough to insist.

(Just hold my thoughts.)

It looks very like 'natural key' vs 'surrogate key' debate. There are
cons and pros.

Anyway, we have the natural ID for collations and let's look why. A user
can define it's own collations and in fact nothing prevents the
situation when one surrogate ID is used for different collations on
different instances. We want to overcome the subtle problem, when a user
assumes one collation, but a different one (maybe very slightly
different) holds the same ID locally.

Okay. How field types are different? Can we (in theory) give a user
ability to register it's own mp_ext (from a specified range), provide
a comparator and name it somehow? If it'll occur (that does not look as
impossible), the surrogate IDs may similarly lead to a confusion, say,
just because of autoincremented / autogenerated numeric IDs. It is
harder to step into such situation with string IDs.

That's aside of extra complexity I already mentioned in the commit
message. I guess most of usages of the <field_type> field will be to
grab it from Lua (where it is identified using a string) or from a human
(which usually operates on strings) or to show it to a human (when
requested or in an error message).

I guess it is not enough to convince you, but I at least made attempt to
show how I think around it (if some future internet archeologist will
curious).

> 
> > XXX: Add a module API test.
> > 
> > Part of #5273
> > ---
> >  src/box/key_def_api.c | 146 ++++++++++++++++++++++++++++++++++++++++++
> >  src/box/key_def_api.h | 123 +++++++++++++++++++++++++++++++++++
> >  src/exports.h         |   2 +
> >  3 files changed, 271 insertions(+)
> > 
> > diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
> > index 7f6c0ac55..19590095d 100644
> > --- a/src/box/key_def_api.c
> > +++ b/src/box/key_def_api.c
> > @@ -55,6 +138,67 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
> >  	return key_def;
> >  }
> >  
> > +box_key_def_t *
> > +box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count)
> > +{
> > +	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 (parts == NULL) {
> 
> 6. I assume you wanted to check for internal_parts == NULL.

Argh. Thanks!

> 
> > +		diag_set(OutOfMemory, internal_parts_size, "region_alloc_array",
> > +			 "parts");
> > +		return NULL;
> > +	}
> > +	if (part_count == 0) {
> 
> 7. internal_parts leaks here? I don't even know if region allocates
> anything when size is 0. I propose to move the check before the
> allocation.

Ugh. Thank you again.

> 
> > +		diag_set(IllegalParams, "At least one key part is required");
> > +		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_MASK) ==
> > +			BOX_KEY_PART_DEF_IS_NULLABLE_MASK;
> 
> 8. You have internal_parts[i].is_nullable for that.

See, there is a logic: this function (and key_def_set_internal_part())
reads everything from the public key part defs and write to the internal
key part defs and the resulting key def.

So the data dependency graph is the following:

 | <public parts> +-> <private parts> ---+
 |                |                      |
 |                |                      +-> <key_def>
 |                |                      |
 |                +-> <min_field_count> -+

But it would be so if I'll implement your suggestion:

 | <public parts> +-> <private parts> +----------------------+
 |                |                   |                      |
 |                |                   |                      +-> <key_def>
 |                |                   |                      |
 |                +-------------------+-> <min_field_count> -+

Or, if I'll change fieldno accesses to read from an internal part too:

 | <public parts> -> <private parts> +----------------------+
 |                                   |                      |
 |                                   |                      +-> <key_def>
 |                                   |                      |
 |                                   +-> <min_field_count> -+

I found the first graph as the simpler one. At least within this code
part, where all accesses are like on the first graph. When we'll move
this code to key_def_new(), we'll follow data access pattern of the
surrounding code.

Acquiring a flag from `parts[i].flags` looks huge, but it just because
of using bit arithmetic / masks and because the public name is large. I
don't think it should be a reason to discourage acquiring a flag from
a public key part.

Anyway, I hope it'll be part of key_def_new() in a future (see the
response to your 9th comment).

> 
> > +		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.
> 
> 9. Just do that and if all works, then it is fine to change. Looks like
> a bug which didn't have a place to explode until now.

I don't want to do it in a hurry, without ability to poke the code and
to make an attempt to write a test case. It is in my backlog, alongside
with several other possible problems found during working on the
patchset. I'll file issues for them and, if time will permit, will
propose fixes.

> 
> > +	 */
> > +	key_def_update_optionality(key_def, min_field_count);
> > +
> > +	return key_def;
> > +}
> > +
> >  void
> >  box_key_def_delete(box_key_def_t *key_def)
> > diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> > index 5b1c861f5..328a58c70 100644
> > --- a/src/box/key_def_api.h
> > +++ b/src/box/key_def_api.h
> > @@ -44,11 +44,104 @@ typedef struct tuple box_tuple_t;
> >  
> >  typedef struct key_def box_key_def_t;
> >  
> > +/** Key part definition flags. */
> > +enum {
> > +	BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT = 0,
> > +	BOX_KEY_PART_DEF_IS_NULLABLE_MASK =
> > +		1 << BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT,
> 
> 10. Does not it seem a little overcomplicated to you? Why
> do you need BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT? Why not
> call BOX_KEY_PART_DEF_IS_NULLABLE_MASK
> BOX_KEY_PART_DEF_IS_NULLABLE_FLAG? Or
> BOX_KEY_PART_DEF_IS_NULLABLE.

Let me think...

Check whether a flag is set:

 | bool is_nullable = (part_def.flags &
 |         BOX_KEY_PART_DEF_IS_NULLABLE_MASK) ==
 |         BOX_KEY_PART_DEF_IS_NULLABLE_MASK;

Set a flag:

 | part_def.flags |= BOX_KEY_PART_DEF_IS_NULLABLE_MASK;

Clear a flag:

 | part_def.flags &= ~BOX_KEY_PART_DEF_IS_NULLABLE_MASK;

In fact, _SHIFT is not needed. So we can simplify it to just
BOX_KEY_PART_DEF_IS_NULLABLE. I'll do.

> 
> > +};
> > +
> > +/**
> > + * 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_ex>() 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,
> > +};
> > +
> > +/**
> > + * 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_ex>() 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_ex>().
> > + *
> > + * 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;
> 
> 11. You know, we could also export a function like box_key_def_part_size(),
> which would return sizeof(key_part_def). Then the module would need
> to call it to allocate the part. And then use functions box_key_def_set_part_*()
> to set the part def fields. So the struct would be opaque, could be
> allocated in an array, and would be ABI stable. However probably it is
> the same what I proposed in the beginning of this email but with extra
> steps.

I hope I made my wishes clear enough in the response to your 2nd comment.

> 
> > +
> >  /**
> >   * 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_ex>().
> > + *
> >   * \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
> > @@ -57,6 +150,28 @@ 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.
> > + *
> > + * All trailing padding bytes are set to zero.
> 
> 12. Why does it need to touch the padding? How is it
> related to ABI?

In fact, I just had desire to specify certain behaviour for functions,
which read from or write to <box_key_part_def_t>. There are the
following variants what to do with unknown fields and flags:

 | #   | Create / dump | Read              |
 | --- | ------------- | ----------------- |
 | (1) | unspecified   | ignore            |
 | (2) | don't touch   | ignore            |
 | (3) | zeroing       | ignore            |
 | (4) | zeroing       | error if not zero |

First, I don't like (1) and (2), because I fear to leave garbage after
initialization, it looks as a bad pattern.

(3) is for the robustness principle and (4) is against. I don't know
what is better here. I can implement my module based on any API with any
desired behaviour (now it forbids to create a key definition with a JSON
path on tarantool versions, which do not support it).

I guess most of times a new field or flag will be added, it will affect
how some key def functions will work (on a new tarantool version), so
(3) assumes explicit check of supported features on the module side.

(4) looks like allowing a module to lean on tarantool to perform the
check. However I'm not sure a module actually can miss parameters
checking. Say, if a default value of an option that is not supported at
given tarantool version is passed, the module should give an error (it
is debatable, but looks better for me).

Does it mean that (4) is more error-prone? I don't know. That's complex
question.

Anyway, both ways work for me. (3) assumes that I can test supported
features in runtime by passing non-zero valid values to _new(), than do
_dump_parts(): supported fields are ones that I got back. With (4) I can
test them in several iterations: pass a field and look whether _new()
gives an error. (See the code for (3) in [1].)

To be honest, I don't know how to better design those APIs. I guess (3)
is a bit more flexible (considering all sides perfectly understand
guarantees and strictly follow the API), so I decided to stick here,
because I don't know a future. Maybe it will allow us to do some nice
thing around behaviour of old version of a module on a new version of
tarantool. But it is hard to imagine something certain.

Okay, let me rephrase. I didn't think so much until you asked and I
chose (3) (because it look more or less natural for me) with the thought
"I'll reconsider it if I'll find some problem while looking at it from
the module side". I didn't find any problem and left it as is.

BTW, I stated explicitly that box_key_def_dump_parts() set unknown
fields and flags to zero.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019997.html

> 
> > + *
> > + * All unknown <flags> bits are set to zero.
> > + */
> > +API_EXPORT void
> > +box_key_part_def_create(box_key_part_def_t *part);

[Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts()

[Alexander Turenko]

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

XXX: Add a module API test.

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

diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 19590095d..25dd416aa 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -205,6 +205,70 @@ 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_MASK;
+		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;
+			}
+			part_def->collation = coll_id->name;
+		}
+
+		/* 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_api.h b/src/box/key_def_api.h
index 328a58c70..f78203a3a 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -180,6 +180,16 @@ box_key_def_new_ex(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. JSON paths) on the box region.
+ * @sa <box_region_truncate>().
+ */
+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 80dd952c7..ff03cfa68 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_ex)
 EXPORT(box_key_part_def_create)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts()

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

On 23.09.2020 03:14, Alexander Turenko wrote:
> The function dumps an opacue <box_key_def_t> structure into a non-opacue

1. opacue -> opaque.

> array of <box_key_part_def_t> structures in order to allow an external
> module to obtain information about the key definition.

2. Why would a module need it, if it already had all the parameters used
to create this key def?

> XXX: Add a module API test.
> 
> Part of #5273
> ---
>  src/box/key_def_api.c | 64 +++++++++++++++++++++++++++++++++++++++++++
>  src/box/key_def_api.h | 10 +++++++
>  src/exports.h         |  1 +
>  3 files changed, 75 insertions(+)
> 
> diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
> index 19590095d..25dd416aa 100644
> --- a/src/box/key_def_api.c
> +++ b/src/box/key_def_api.c
> @@ -205,6 +205,70 @@ 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_MASK;
> +		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;
> +			}
> +			part_def->collation = coll_id->name;

3. What if coll_id is removed after return from this function? For
example, by deleting a tuple from _collation. The pointer would
be dead. We need to copy the name.

> +		}
> +
> +		/* 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;
> +}

Re: [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts()

[Alexander Turenko]

> > The function dumps an opacue <box_key_def_t> structure into a non-opacue
> 
> 1. opacue -> opaque.

Will fix in the next version of the patchset.

> > array of <box_key_part_def_t> structures in order to allow an external
> > module to obtain information about the key definition.
> 
> 2. Why would a module need it, if it already had all the parameters used
> to create this key def?

When __serialize metamethod is called on cdata<struct key_def_key_def *>
(it is a pointer to <struct key_def>, see [^1]), we only have the
pointer to the opaque structure. Of course, we can create our own
structure around <struct key_def> and duplicate all information there,
but it would be...  I don't know... wasteful.

[^1]: Side note regarding <struct key_def_key_def> name:

       | Modification of cdata metatype is not allowed in LuaJIT, so we
       | should use another structure name for Lua (not
       | <struct key_def>). The reason is that built-in module is
       | already loaded and already calls its ffi.metatype() on
       | <struct key_def>.

      https://github.com/Totktonada/key_def/commit/645c8902c8242f0d1e5168b207676af6f8824b70

      It is key_def prefixed by the module name ('key_def'). May be
      changed after the module renaming to tuple-keydef.

Presence of box_key_def_dump_parts() also opens abitility to do a
runtime test, whether a particular feature is supported on given
tarantool version: say, we can set a JSON path and look whether it is
present in the dumped key parts. That's also why I always set all
unknown fields and flags to zero in box_key_part_def_create().

I think, this approach is good, because it works by construction: we
don't need to introduce a feature test function for any new key_def
feature: I guess it may be forgotten.

That's how runtime check whether JSON path is supported is performed in
the external module:

 | /**
 |  * Runtime check whether JSON path is supported.
 |  */
 | static bool
 | json_path_is_supported(void)
 | {
 |         bool res = false;
 |
 |         /* Create a key_def with JSON path. */
 |         box_key_part_def_t part;
 |         box_key_part_def_create(&part);
 |         part.fieldno = 0;
 |         part.field_type = "unsigned";
 |         JSON_PATH_SET(&part, "[1]");
 |         box_key_def_t *key_def = box_key_def_new_ex(&part, 1);
 |
 |         /* Dump parts and look whether JSON path is dumped. */
 |         size_t region_svp = box_region_used();
 |         uint32_t part_count = 0;
 |         box_key_part_def_t *parts = box_key_def_dump_parts(key_def,
 |                                                            &part_count);
 |         assert(parts != NULL);
 |         res = JSON_PATH(&parts[0]) != NULL;
 |         box_region_truncate(region_svp);
 |
 |         /* Delete the key_def. */
 |         box_key_def_delete(key_def);
 |
 |         return res;
 | }

https://github.com/Totktonada/key_def/commit/73dae9d26ad887ca38872a40962b958c2e4bbd84

This, however, was not the main purpose of the function, it is more like
the side effect. The main idea was to don't duplicate information from
<struct key_def> in a module level wrapping structure.

> > +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_MASK;
> > +		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;
> > +			}
> > +			part_def->collation = coll_id->name;
> 
> 3. What if coll_id is removed after return from this function? For
> example, by deleting a tuple from _collation. The pointer would
> be dead. We need to copy the name.

Sure. I'll do.

[Tarantool-patches] [PATCH 11/14] WIP: module api: expose box_tuple_validate_key_parts()

[Alexander Turenko]

XXX: Add a module API test.

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

diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 25dd416aa..b8a0d5807 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -269,6 +269,12 @@ box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr)
 	return parts;
 }
 
+int
+box_tuple_validate_key_parts(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_api.h b/src/box/key_def_api.h
index f78203a3a..16934eeb4 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -190,6 +190,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_tuple_validate_key_parts(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 ff03cfa68..45a62421e 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -74,6 +74,7 @@ EXPORT(box_tuple_to_buf)
 EXPORT(box_tuple_unref)
 EXPORT(box_tuple_update)
 EXPORT(box_tuple_upsert)
+EXPORT(box_tuple_validate_key_parts)
 EXPORT(box_txn)
 EXPORT(box_txn_alloc)
 EXPORT(box_txn_begin)
-- 
2.25.0

[Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge()

[Alexander Turenko]

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

diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index b8a0d5807..740fb4339 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -292,4 +292,10 @@ 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);
+}
+
 /* }}} API functions implementations */
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index 16934eeb4..0cef7d1ea 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -229,6 +229,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 45a62421e..be8144036 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_ex)
 EXPORT(box_key_part_def_create)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge()

[Vladislav Shpilevoy]

Thanks for the patch!

Why would a module need that function? Its usage case is very
specific - merge primary and secondary parts to create a
unique key def for non-unique indexes. Is there a case outside
of the storage engines for that function?

Re: [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge()

[Alexander Turenko]

On Sat, Sep 26, 2020 at 12:58:21AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Why would a module need that function? Its usage case is very
> specific - merge primary and secondary parts to create a
> unique key def for non-unique indexes. Is there a case outside
> of the storage engines for that function?

Sure. At least I can give a case from merger usage: if we have several
sources of tuples that are sorted in the order of a secondary key +
primary key and want to merge them keeping the order, we need to
construct a key_def that is union of those key parts. The functions does
exactly this, that is very convenient.

Usage example:

https://github.com/Totktonada/tarantool-merger-examples/blob/557c15a191b465e8e9673203dd0e388a3345f625/chunked_example_fast/frontend.lua#L32-L36

Here we create a key_def object to pass it to merger as a tuple
comparator. The main function of this example, mr_call(), selects tuples
from given vshard cluster using given space, index and key (just like
usual box select).

If an index is not unique, we call :merge() to mix primary
key parts into secondary ones.

A predicatable order may be important in different tasks, but first
example that I have in the mind is pagination. It does not work, if the
order is not strictly defined (say, depend on how storages -- and so
merge source -- are enumerated when passed to merger).

[Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex()

[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. Also there is no <MULTIKEY_NONE> constant in the
backport.

XXX: Add a module API test.

Part of #5273
---
 src/CMakeLists.txt    |  1 +
 src/box/field_map.h   |  4 ++++
 src/box/key_def_api.c |  7 +++++++
 src/box/key_def_api.h | 20 ++++++++++++++++++++
 src/exports.h         |  1 +
 5 files changed, 33 insertions(+)

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 89e5bbc34..9efc636df 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -155,6 +155,7 @@ set(api_headers
     ${CMAKE_SOURCE_DIR}/src/box/index.h
     ${CMAKE_SOURCE_DIR}/src/box/iterator_type.h
     ${CMAKE_SOURCE_DIR}/src/box/error.h
+    ${CMAKE_SOURCE_DIR}/src/box/field_map.h
     ${CMAKE_SOURCE_DIR}/src/box/lua/call.h
     ${CMAKE_SOURCE_DIR}/src/box/lua/tuple.h
     ${CMAKE_SOURCE_DIR}/src/lib/core/latch.h
diff --git a/src/box/field_map.h b/src/box/field_map.h
index d8ef726a1..6e3c5be25 100644
--- a/src/box/field_map.h
+++ b/src/box/field_map.h
@@ -38,12 +38,16 @@
 struct region;
 struct field_map_builder_slot;
 
+/** \cond public */
+
 /**
  * A special value of multikey index that means that the key
  * definition is not multikey and no indirection is expected.
  */
 enum { MULTIKEY_NONE = -1 };
 
+/** \endcond public */
+
 /**
  * A field map is a special area is reserved before tuple's
  * MessagePack data. It is a sequence of the 32-bit unsigned
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 740fb4339..1257e9060 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -298,4 +298,11 @@ box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second)
 	return key_def_merge(first, second);
 }
 
+char *
+box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
+			 int multikey_idx, uint32_t *key_size_ptr)
+{
+	return tuple_extract_key(tuple, key_def, multikey_idx, key_size_ptr);
+}
+
 /* }}} API functions implementations */
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index 0cef7d1ea..28fcc32da 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -243,6 +243,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 tuple - tuple from which need to extract key
+ * @param key_def - definition of key that need to extract
+ * @param multikey_idx - multikey index hint
+ * @param key_size_ptr - here will be size of extracted key
+ *
+ * @retval not NULL Success
+ * @retval NULL     Memory allocation error
+ */
+API_EXPORT char *
+box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
+			 int multikey_idx, uint32_t *key_size_ptr);
+
 /** \endcond public */
 
 /*
diff --git a/src/exports.h b/src/exports.h
index be8144036..6ddc2e9b0 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -56,6 +56,7 @@ EXPORT(box_tuple_bsize)
 EXPORT(box_tuple_compare)
 EXPORT(box_tuple_compare_with_key)
 EXPORT(box_tuple_extract_key)
+EXPORT(box_tuple_extract_key_ex)
 EXPORT(box_tuple_field)
 EXPORT(box_tuple_field_count)
 EXPORT(box_tuple_format)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex()

[Vladislav Shpilevoy]

Thanks for the patch!

Or instead of _ex you could make it a method of key_def and
call it box_key_def_extract_key(...). IMO that would be more
appropriate as a key_def method.

On 23.09.2020 03:14, Alexander Turenko wrote:
> 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.

I hate multikeys even more now.

See 2 comments below.

> Note: The <multikey_idx> parameter is ignored on the backported version
> of the patch on 1.10. Also there is no <MULTIKEY_NONE> constant in the
> backport.
> 
> XXX: Add a module API test.
> 
> Part of #5273
> diff --git a/src/box/field_map.h b/src/box/field_map.h
> index d8ef726a1..6e3c5be25 100644
> --- a/src/box/field_map.h
> +++ b/src/box/field_map.h
> @@ -38,12 +38,16 @@
>  struct region;
>  struct field_map_builder_slot;
>  
> +/** \cond public */
> +
>  /**
>   * A special value of multikey index that means that the key
>   * definition is not multikey and no indirection is expected.
>   */
>  enum { MULTIKEY_NONE = -1 };

1. Why didn't you add a new BOX_KEY_DEF_MULTIKEY_NONE constant?
I thought we don't want to expose any internal definitions.

Also you could specify in the comment that pass -1 if don't want
to use multikey. Without a enum. I think it would be totally
fine.

> diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> index 0cef7d1ea..28fcc32da 100644
> --- a/src/box/key_def_api.h
> +++ b/src/box/key_def_api.h
> @@ -243,6 +243,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 tuple - tuple from which need to extract key

2. Lets be consistent and not use '-' between '@param name'
and the parameter description. In addition to using capital
letter in the beginning and dots in the end. Sorry for OCD.

> + * @param key_def - definition of key that need to extract
> + * @param multikey_idx - multikey index hint
> + * @param key_size_ptr - here will be size of extracted key
> + *
> + * @retval not NULL Success
> + * @retval NULL     Memory allocation error
> + */
> +API_EXPORT char *
> +box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
> +			 int multikey_idx, uint32_t *key_size_ptr);
> +
>  /** \endcond public */

Re: [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex()

[Alexander Turenko]

On Sat, Sep 26, 2020 at 12:58:27AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Or instead of _ex you could make it a method of key_def and
> call it box_key_def_extract_key(...). IMO that would be more
> appropriate as a key_def method.

Since more questions about functions naming arose, I put my thoughts to
[1].

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019989.html

> > +/** \cond public */
> > +
> >  /**
> >   * A special value of multikey index that means that the key
> >   * definition is not multikey and no indirection is expected.
> >   */
> >  enum { MULTIKEY_NONE = -1 };
> 
> 1. Why didn't you add a new BOX_KEY_DEF_MULTIKEY_NONE constant?
> I thought we don't want to expose any internal definitions.
> 
> Also you could specify in the comment that pass -1 if don't want
> to use multikey. Without a enum. I think it would be totally
> fine.

I agree. Will remove it in the next patchset version.

> 
> > diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> > index 0cef7d1ea..28fcc32da 100644
> > --- a/src/box/key_def_api.h
> > +++ b/src/box/key_def_api.h
> > @@ -243,6 +243,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 tuple - tuple from which need to extract key
> 
> 2. Lets be consistent and not use '-' between '@param name'
> and the parameter description. In addition to using capital
> letter in the beginning and dots in the end. Sorry for OCD.

Okay.

Re: [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex()

[Alexander Turenko]

On Fri, Oct 09, 2020 at 04:14:23AM +0300, Alexander Turenko wrote:
> On Sat, Sep 26, 2020 at 12:58:27AM +0200, Vladislav Shpilevoy wrote:
> > Thanks for the patch!
> > 
> > Or instead of _ex you could make it a method of key_def and
> > call it box_key_def_extract_key(...). IMO that would be more
> > appropriate as a key_def method.
> 
> Since more questions about functions naming arose, I put my thoughts to
> [1].
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019989.html

It seems, we agreed on box_key_def_extract_key(). See [2] for details.

[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/020013.html

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

[Alexander Turenko]

XXX: Add a module API test.

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

diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 1257e9060..2555b9fdd 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -305,4 +305,19 @@ box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
 	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,
+			 bool allow_nullable)
+{
+	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, allow_nullable,
+				  &key_end);
+}
+
 /* }}} API functions implementations */
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index 28fcc32da..8dd6eb10b 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -36,6 +36,7 @@ extern "C" {
 #endif /* defined(__cplusplus) */
 
 #include <stdint.h>
+#include <stdbool.h>
 #include "trivia/util.h"
 
 typedef struct tuple box_tuple_t;
@@ -263,6 +264,20 @@ API_EXPORT char *
 box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
 			 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.
+ * @param allow_nullable True if nullable parts are allowed.
+ *
+ * @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,
+			 bool allow_nullable);
+
 /** \endcond public */
 
 /*
diff --git a/src/exports.h b/src/exports.h
index 6ddc2e9b0..48894ea72 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -34,6 +34,7 @@ EXPORT(box_key_def_dump_parts)
 EXPORT(box_key_def_merge)
 EXPORT(box_key_def_new)
 EXPORT(box_key_def_new_ex)
+EXPORT(box_key_def_validate_key)
 EXPORT(box_key_part_def_create)
 EXPORT(box_latch_delete)
 EXPORT(box_latch_lock)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key()

[Vladislav Shpilevoy]

Thanks for the patch!

On 23.09.2020 03:14, Alexander Turenko wrote:
> XXX: Add a module API test.
> 
> Part of #5273
> ---
>  src/box/key_def_api.c | 15 +++++++++++++++
>  src/box/key_def_api.h | 15 +++++++++++++++
>  src/exports.h         |  1 +
>  3 files changed, 31 insertions(+)
> 
> diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
> index 1257e9060..2555b9fdd 100644
> --- a/src/box/key_def_api.c
> +++ b/src/box/key_def_api.c
> @@ -305,4 +305,19 @@ box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
>  	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,
> +			 bool allow_nullable)

I don't like exposing such internal parameters as 'allow_nullable'. It
took me some time to find why is it even needed. Maybe better export 2
separate functions?

    box_key_def_validate_exact_key
    box_key_def_validate_range_key

Or

    box_key_def_validate_unique_key
    box_key_def_validate_key

Or

    box_key_def_validate_unique_key
    box_key_def_validate_non_unique_key

Or

    box_key_def_validate_primary_key
    box_key_def_validate_secondary_key

Also did you think about making this function not setting any error?
What if a module code does not want to spoil diag, and just wants
to check the key? So as to set an own error later. Maybe would be
more flexible to expose a simple 'is_valid' instead of 'validate'?

Or do we do that to get a more precise error message and this is
more important than not setting a diag when not needed?

> +{
> +	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, allow_nullable,
> +				  &key_end);
> +}

Re: [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key()

[Alexander Turenko]

> > +int
> > +box_key_def_validate_key(const box_key_def_t *key_def, const char *key,
> > +			 bool allow_nullable)
> 
> I don't like exposing such internal parameters as 'allow_nullable'. It
> took me some time to find why is it even needed. Maybe better export 2
> separate functions?
> 
>     box_key_def_validate_exact_key
>     box_key_def_validate_range_key
> 
> Or
> 
>     box_key_def_validate_unique_key
>     box_key_def_validate_key
> 
> Or
> 
>     box_key_def_validate_unique_key
>     box_key_def_validate_non_unique_key
> 
> Or
> 
>     box_key_def_validate_primary_key
>     box_key_def_validate_secondary_key
> 
> Also did you think about making this function not setting any error?
> What if a module code does not want to spoil diag, and just wants
> to check the key? So as to set an own error later. Maybe would be
> more flexible to expose a simple 'is_valid' instead of 'validate'?

To be honest, I don't like 'allow_nullable' name too. I had guess that
it would be more intuitive than 'is_exact', but it seems, it is not.

In fact, the flag is like a hack to validate a key against a key_def as
if all parts would be defined as non-nullable. Nothing prevents a user
to create the same key_def, but non-nullable (_dump_parts(), change
nullability of parts, call _new_v2()).

So I will just remove the flag. If we'll really need it, we surely will
able to add a function that will be named using words 'exact' and 'key'.

Aside of this, please, look at the discussion around names of tuple and
key_def related functions in [1]. It relates to this function as well.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019989.html

> 
> Or do we do that to get a more precise error message and this is
> more important than not setting a diag when not needed?

I would say 'yes'. The diagnostic area is a kind of errno and I don't
see anything bad in filling it even when not any caller will read the
error.

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

[Alexander Turenko]

The backport of the patchset to 1.10. I decided to send it to ease
commenting (despite that there is many repeating code).

The status is the same as for the master patchset: it looks ready except
tests. I'll write them.

See the cover letter for the master patchset for details ([1]). My
doubts around the backport are also in the master cover letter.

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')

Branch:

https://github.com/tarantool/tarantool/tree/Totktonada/gh-5273-expand-module-api-1.10

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

Alexander Turenko (16):
  collation: allow to find a collation by a name
  refactoring: adjust contract of luaT_tuple_new()
  module api: get rid of typedef redefinitions
  WIP: module api: expose box region
  WIP: module api/lua: add luaL_iscdata() function
  WIP: module api/lua: expose luaT_tuple_new()
  WIP: module api/lua: add luaT_tuple_encode()
  WIP: refactoring: add API_EXPORT to lua/tuple functions
  WIP: refactoring: add API_EXPORT to key_def functions
  WIP: refactoring: extract key_def module API functions
  WIP: module api: add box_key_def_new_ex()
  WIP: module api: add box_key_def_dump_parts()
  WIP: module api: expose box_tuple_validate_key_parts()
  WIP: module api: expose box_key_def_merge()
  WIP: module api: expose box_tuple_extract_key_ex()
  WIP: module api: add box_key_def_validate_key()

 extra/exports                   |  14 ++
 src/CMakeLists.txt              |   6 +-
 src/box/CMakeLists.txt          |   1 +
 src/box/coll_id_cache.c         |  50 +++++-
 src/box/coll_id_cache.h         |   6 +
 src/box/index.h                 |   5 +-
 src/box/key_def.c               |  53 +-----
 src/box/key_def.h               |  85 ++++-----
 src/box/key_def_api.c           | 298 ++++++++++++++++++++++++++++++++
 src/box/key_def_api.h           | 274 +++++++++++++++++++++++++++++
 src/box/lua/space.cc            |   2 +-
 src/box/lua/tuple.c             |  62 +++++--
 src/box/lua/tuple.h             |  65 ++++++-
 src/fiber.c                     |  24 +++
 src/fiber.h                     | 101 +++++++++++
 src/lua/utils.c                 |   6 +
 src/lua/utils.h                 |  20 +++
 test/app-tap/CMakeLists.txt     |   6 +
 test/app-tap/module_api.c       |   2 +
 test/unit/CMakeLists.txt        |   4 +
 test/unit/luaT_tuple_new.c      | 175 +++++++++++++++++++
 test/unit/luaT_tuple_new.result |  22 +++
 test/unit/vy_iterators_helper.c |   1 +
 test/unit/vy_mem.c              |   1 +
 test/unit/vy_point_lookup.c     |   1 +
 test/unit/vy_write_iterator.c   |   1 +
 26 files changed, 1151 insertions(+), 134 deletions(-)
 create mode 100644 src/box/key_def_api.c
 create mode 100644 src/box/key_def_api.h
 create mode 100644 test/unit/luaT_tuple_new.c
 create mode 100644 test/unit/luaT_tuple_new.result

-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 01/16] collation: allow to find a collation by a name

[Alexander Turenko]

Module API key part structure, which will be introduced in a next
commit, holds a collation name. We need to transform it to a collation
id in order to create a key definition.

The change is backported from commit 1.8.3-58-g8629bd897 ('Add search
collations by name') with aware of changes made after the commit (up to
2.6.0-78-gc1f72aeb7).

Part of #5273

(backported from commit 8629bd8979927f8befede9a028eed0564a92a558)
---
 src/box/coll_id_cache.c | 50 ++++++++++++++++++++++++++++++++++++-----
 src/box/coll_id_cache.h |  6 +++++
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/src/box/coll_id_cache.c b/src/box/coll_id_cache.c
index 122863937..45d295eb9 100644
--- a/src/box/coll_id_cache.c
+++ b/src/box/coll_id_cache.c
@@ -33,6 +33,8 @@
 #include "diag.h"
 #include "assoc.h"
 
+/** mhash table (name, len -> collation) */
+static struct mh_strnptr_t *coll_cache_name = NULL;
 /** mhash table (id -> collation) */
 static struct mh_i32ptr_t *coll_id_cache = NULL;
 
@@ -45,12 +47,20 @@ coll_id_cache_init()
 			 "coll_id_cache");
 		return -1;
 	}
+	coll_cache_name = mh_strnptr_new();
+	if (coll_cache_name == NULL) {
+		diag_set(OutOfMemory, sizeof(*coll_cache_name), "malloc",
+			 "coll_cache_name");
+		mh_i32ptr_delete(coll_id_cache);
+		return -1;
+	}
 	return 0;
 }
 
 void
 coll_id_cache_destroy()
 {
+	mh_strnptr_delete(coll_cache_name);
 	mh_i32ptr_delete(coll_id_cache);
 }
 
@@ -60,12 +70,27 @@ coll_id_cache_replace(struct coll_id *coll_id, struct coll_id **replaced_id)
 	const struct mh_i32ptr_node_t id_node = {coll_id->id, coll_id};
 	struct mh_i32ptr_node_t repl_id_node = {0, NULL};
 	struct mh_i32ptr_node_t *prepl_id_node = &repl_id_node;
-	if (mh_i32ptr_put(coll_id_cache, &id_node, &prepl_id_node, NULL) ==
-	    mh_end(coll_id_cache)) {
+	mh_int_t i =
+		mh_i32ptr_put(coll_id_cache, &id_node, &prepl_id_node, NULL);
+	if (i == mh_end(coll_id_cache)) {
 		diag_set(OutOfMemory, sizeof(id_node), "malloc",
 			 "coll_id_cache");
 		return -1;
 	}
+
+	uint32_t hash = mh_strn_hash(coll_id->name, coll_id->name_len);
+	const struct mh_strnptr_node_t name_node =
+		{ coll_id->name, coll_id->name_len, hash, coll_id };
+	struct mh_strnptr_node_t repl_name_node = { NULL, 0, 0, NULL };
+	struct mh_strnptr_node_t *prepl_node_name = &repl_name_node;
+	if (mh_strnptr_put(coll_cache_name, &name_node, &prepl_node_name,
+			   NULL) == mh_end(coll_id_cache)) {
+		diag_set(OutOfMemory, sizeof(name_node), "malloc",
+			 "coll_cache_name");
+		mh_i32ptr_del(coll_id_cache, i, NULL);
+		return -1;
+	}
+	assert(repl_id_node.val == repl_name_node.val);
 	assert(repl_id_node.val == NULL);
 	*replaced_id = repl_id_node.val;
 	return 0;
@@ -74,10 +99,11 @@ coll_id_cache_replace(struct coll_id *coll_id, struct coll_id **replaced_id)
 void
 coll_id_cache_delete(const struct coll_id *coll_id)
 {
-	mh_int_t i = mh_i32ptr_find(coll_id_cache, coll_id->id, NULL);
-	if (i == mh_end(coll_id_cache))
-		return;
-	mh_i32ptr_del(coll_id_cache, i, NULL);
+	mh_int_t id_i = mh_i32ptr_find(coll_id_cache, coll_id->id, NULL);
+	mh_i32ptr_del(coll_id_cache, id_i, NULL);
+	mh_int_t name_i = mh_strnptr_find_inp(coll_cache_name, coll_id->name,
+					      coll_id->name_len);
+	mh_strnptr_del(coll_cache_name, name_i, NULL);
 }
 
 struct coll_id *
@@ -88,3 +114,15 @@ coll_by_id(uint32_t id)
 		return NULL;
 	return mh_i32ptr_node(coll_id_cache, pos)->val;
 }
+
+/**
+ * Find a collation object by its name.
+ */
+struct coll_id *
+coll_by_name(const char *name, uint32_t len)
+{
+	mh_int_t pos = mh_strnptr_find_inp(coll_cache_name, name, len);
+	if (pos == mh_end(coll_cache_name))
+		return NULL;
+	return mh_strnptr_node(coll_cache_name, pos)->val;
+}
diff --git a/src/box/coll_id_cache.h b/src/box/coll_id_cache.h
index 4bbbc85de..e3115ddb5 100644
--- a/src/box/coll_id_cache.h
+++ b/src/box/coll_id_cache.h
@@ -71,6 +71,12 @@ coll_id_cache_delete(const struct coll_id *coll_id);
 struct coll_id *
 coll_by_id(uint32_t id);
 
+/**
+ * Find a collation object by its name.
+ */
+struct coll_id *
+coll_by_name(const char *name, uint32_t len);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new()

[Alexander Turenko]

Accept an index on the Lua stack. Report an error using the diagnostics
area (not raise a Lua error).

The function is used by built-in key_def and merger modules on
tarantool-2.*. Now we plan to expose necessary APIs from tarantool of
all supported versions (including 1.10) and create external key_def and
merger modules.

Part of #5273

(cherry picked from commit 64a6464d31c92ea11c4f5c3ccdaada2ae9666455)

The original commit message:

 | lua: add luaT_tuple_new()
 |
 | The function allows to create a tuple with specific tuple format in C
 | code using a Lua table, another tuple, or objects on a Lua stack.
 |
 | Needed for #3276, #3398, #4025

The original message is a bit misleading, because it was not rewritten
after introduction of the function with the same name in
9e2a905c069e58cdd95d91868025391ecd9404e6 ('box: fix format of tuple
produced with frommap()').
---
 src/box/lua/space.cc            |   2 +-
 src/box/lua/tuple.c             |  47 ++++++---
 src/box/lua/tuple.h             |  13 ++-
 test/unit/CMakeLists.txt        |   4 +
 test/unit/luaT_tuple_new.c      | 175 ++++++++++++++++++++++++++++++++
 test/unit/luaT_tuple_new.result |  22 ++++
 6 files changed, 245 insertions(+), 18 deletions(-)
 create mode 100644 test/unit/luaT_tuple_new.c
 create mode 100644 test/unit/luaT_tuple_new.result

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 94e895148..9ea0d6f7e 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -507,7 +507,7 @@ lbox_space_frommap(struct lua_State *L)
 
 	lua_replace(L, 1);
 	lua_settop(L, 1);
-	tuple = luaT_tuple_new(L, space->format);
+	tuple = luaT_tuple_new(L, -1, space->format);
 	if (tuple == NULL) {
 		struct error *e = diag_last_error(diag_get());
 		lua_pushnil(L);
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index cff0beca5..8a8fd4d34 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -98,37 +98,38 @@ luaT_istuple(struct lua_State *L, int narg)
 }
 
 struct tuple *
-luaT_tuple_new(struct lua_State *L, struct tuple_format *format)
+luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
 {
-	int argc = lua_gettop(L);
-	if (argc < 1) {
-		lua_newtable(L); /* create an empty tuple */
-		++argc;
+	if (idx != 0 && !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;
 
+	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 (argc == 1 && (lua_istable(L, 1) || luaT_istuple(L, 1))) {
-		/* New format: box.tuple.new({1, 2, 3}) */
-		luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
-	} else {
-		/* Backward-compatible format: box.tuple.new(1, 2, 3). */
+	if (idx == 0) {
+		/*
+		 * Create the tuple from lua stack
+		 * objects.
+		 */
+		int argc = lua_gettop(L);
 		luamp_encode_array(luaL_msgpack_default, &stream, argc);
 		for (int k = 1; k <= argc; ++k) {
 			luamp_encode(L, luaL_msgpack_default, &stream, k);
 		}
+	} else {
+		/* Create the tuple from a Lua table. */
+		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));
+					    buf->buf + ibuf_used(buf));
 	if (tuple == NULL)
 		return NULL;
-	/* box_tuple_new() doesn't leak on exception, see public API doc */
 	ibuf_reinit(tarantool_lua_ibuf);
 	return tuple;
 }
@@ -136,9 +137,23 @@ luaT_tuple_new(struct lua_State *L, struct tuple_format *format)
 static int
 lbox_tuple_new(lua_State *L)
 {
-	struct tuple *tuple = luaT_tuple_new(L, box_tuple_format_default());
+	int argc = lua_gettop(L);
+	if (argc < 1) {
+		lua_newtable(L); /* create an empty tuple */
+		++argc;
+	}
+	/*
+	 * 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).
+	 */
+	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 (tuple == NULL)
 		return luaT_error(L);
+	/* box_tuple_new() doesn't leak on exception, see public API doc */
 	luaT_pushtuple(L, tuple);
 	return 1;
 }
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index 48b630969..dd3aa0f08 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -42,6 +42,8 @@ typedef struct tuple box_tuple_t;
 struct lua_State;
 struct mpstream;
 struct luaL_serializer;
+struct tuple_format;
+typedef struct tuple_format box_tuple_format_t;
 
 /** \cond public */
 
@@ -67,8 +69,17 @@ 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.
+ *
+ * In case of an error set a diag and return NULL.
+ */
 struct tuple *
-luaT_tuple_new(struct lua_State *L, struct tuple_format *format);
+luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format);
 
 static inline int
 luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index d0138e8fd..255d27083 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -148,6 +148,10 @@ add_executable(histogram.test histogram.c)
 target_link_libraries(histogram.test stat unit)
 add_executable(ratelimit.test ratelimit.c)
 target_link_libraries(ratelimit.test unit)
+add_executable(luaT_tuple_new.test luaT_tuple_new.c)
+target_link_libraries(luaT_tuple_new.test unit box server core misc
+    ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES}
+    ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES})
 
 add_executable(say.test say.c)
 target_link_libraries(say.test core unit)
diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c
new file mode 100644
index 000000000..522216ce1
--- /dev/null
+++ b/test/unit/luaT_tuple_new.c
@@ -0,0 +1,175 @@
+#include <string.h>           /* strncmp() */
+#include <lua.h>              /* lua_*() */
+#include <lauxlib.h>          /* luaL_*() */
+#include <lualib.h>           /* luaL_openlibs() */
+#include "unit.h"             /* plan, header, footer, is, ok */
+#include "memory.h"           /* memory_init() */
+#include "fiber.h"            /* fiber_init() */
+#include "small/ibuf.h"       /* struct ibuf */
+#include "box/box.h"          /* box_init() */
+#include "box/tuple.h"        /* box_tuple_format_default() */
+#include "lua/msgpack.h"      /* luaopen_msgpack() */
+#include "box/lua/tuple.h"    /* luaT_tuple_new() */
+#include "diag.h"             /* struct error, diag_*() */
+#include "exception.h"        /* type_IllegalParams */
+
+/*
+ * This test checks all usage cases of luaT_tuple_new():
+ *
+ * * Use with idx == 0 and idx != 0.
+ * * Use with default and non-default formats.
+ * * Use a table and a tuple as an input.
+ * * Use with an unexpected lua type as an input.
+ *
+ * The test does not vary an input table/tuple. This is done in
+ * box/tuple.test.lua.
+ */
+
+extern struct ibuf *tarantool_lua_ibuf;
+
+uint32_t
+min_u32(uint32_t a, uint32_t b)
+{
+	return a < b ? a : b;
+}
+
+void
+check_tuple(const struct tuple *tuple, box_tuple_format_t *format,
+	    int retvals, const char *case_name)
+{
+	uint32_t size;
+	const char *data = tuple_data_range(tuple, &size);
+
+	ok(tuple != NULL, "%s: tuple != NULL", case_name);
+	is(tuple->format_id, tuple_format_id(format),
+	   "%s: check tuple format id", case_name);
+	is(size, 4, "%s: check tuple size", case_name);
+	ok(!strncmp(data, "\x93\x01\x02\x03", min_u32(size, 4)),
+	   "%s: check tuple data", case_name);
+	is(retvals, 0, "%s: check retvals count", case_name);
+}
+
+void
+check_error(struct lua_State *L, const struct tuple *tuple, int retvals,
+	    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);
+	ok(!strcmp(e->errmsg, exp_err), "%s: check error message", case_name);
+}
+
+int
+test_basic(struct lua_State *L)
+{
+	plan(19);
+	header();
+
+	int top;
+	struct tuple *tuple;
+	box_tuple_format_t *default_format = box_tuple_format_default();
+
+	/*
+	 * 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 = luaT_tuple_new(L, -2, default_format);
+	check_tuple(tuple, default_format, lua_gettop(L) - top, "table");
+
+	/* Clean up. */
+	lua_pop(L, 2);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: a tuple on idx == -1 as an input.
+	 */
+
+	/* Prepare the Lua stack. */
+	luaT_pushtuple(L, tuple);
+
+	/* Create and check a tuple. */
+	top = lua_gettop(L);
+	tuple = luaT_tuple_new(L, -1, default_format);
+	check_tuple(tuple, default_format, lua_gettop(L) - top, "tuple");
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: elements on the stack (idx == 0) as an input and
+	 * a non-default format.
+	 */
+
+	/* Prepare the Lua stack. */
+	lua_pushinteger(L, 1);
+	lua_pushinteger(L, 2);
+	lua_pushinteger(L, 3);
+
+	/* Create a new format. */
+	struct key_part_def part;
+	part.fieldno = 0;
+	part.type = FIELD_TYPE_INTEGER;
+	part.coll_id = COLL_NONE;
+	part.is_nullable = false;
+	struct key_def *key_def = key_def_new(&part, 1);
+	box_tuple_format_t *another_format = box_tuple_format_new(&key_def, 1);
+	key_def_delete(key_def);
+
+	/* Create and check a tuple. */
+	top = lua_gettop(L);
+	tuple = luaT_tuple_new(L, 0, another_format);
+	check_tuple(tuple, another_format, lua_gettop(L) - top, "objects");
+
+	/* Clean up. */
+	tuple_format_delete(another_format);
+	lua_pop(L, 3);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: a lua object of an unexpected type.
+	 */
+
+	/* Prepare the Lua stack. */
+	lua_pushinteger(L, 42);
+
+	/* 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");
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	footer();
+	return check_plan();
+}
+
+int
+main()
+{
+	memory_init();
+	fiber_init(fiber_c_invoke);
+
+	ibuf_create(tarantool_lua_ibuf, &cord()->slabc, 16000);
+
+	struct lua_State *L = luaL_newstate();
+	luaL_openlibs(L);
+
+	box_init();
+	luaopen_msgpack(L);
+	box_lua_tuple_init(L);
+	lua_pop(L, 1);
+
+	return test_basic(L);
+}
diff --git a/test/unit/luaT_tuple_new.result b/test/unit/luaT_tuple_new.result
new file mode 100644
index 000000000..110aa68c2
--- /dev/null
+++ b/test/unit/luaT_tuple_new.result
@@ -0,0 +1,22 @@
+1..19
+	*** test_basic ***
+ok 1 - table: tuple != NULL
+ok 2 - table: check tuple format id
+ok 3 - table: check tuple size
+ok 4 - table: check tuple data
+ok 5 - table: check retvals count
+ok 6 - tuple: tuple != NULL
+ok 7 - tuple: check tuple format id
+ok 8 - tuple: check tuple size
+ok 9 - tuple: check tuple data
+ok 10 - tuple: check retvals count
+ok 11 - objects: tuple != NULL
+ok 12 - objects: check tuple format id
+ok 13 - objects: check tuple size
+ok 14 - objects: check tuple data
+ok 15 - objects: check retvals count
+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
+	*** test_basic: done ***
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new()

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

There is no 'refactoring:' subsystem. Lets use 'lua:'.

Re: [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new()

[Alexander Turenko]

On Mon, Sep 28, 2020 at 11:26:05PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> There is no 'refactoring:' subsystem. Lets use 'lua:'.

TL;RD: Changed (will be so in the next patchset version).

Hm.

I re-read the formal rule in our guidelines and it's origin (kernel's
guidelines): you're right, a prefix should be a subsystem. I also looked
over kernel's commits and, yep, actual commits are prefixed with a
subsystem.

I found it meaningful, when you have a lot of (at least several)
subsystems and there are persons, who look only at some of them: when a
project is relatively large.

When a project is quite small and subsystems are like 'code', 'test',
'build system', 'ci', the 'refactoring' prefix is useful to mark
changes, which do not change a user visible behaviour.

Anyway, tarantool is surely large enough to prefer a subsystem here. I
agree.

Aside of this, I reworded a bit the explanation why the original commit
message is strage:

 | The original message is a bit misleading, because the message was not
 | rewritten after introduction of the function with the same name in
 | 9e2a905c069e58cdd95d91868025391ecd9404e6 ('box: fix format of tuple
 | produced with frommap()'). Those patches were created in parallel.

(Changed 'it' -> 'the message'. Added the last sentence.)

[Tarantool-patches] [PATCH 1.10 03/16] 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

(cherry picked from commit 9d571850d22dea461b5029a50611722b697cdfa8)
---
 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 a8c515134..45a8f7733 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -197,9 +197,9 @@ set(api_headers
     ${CMAKE_SOURCE_DIR}/src/coio_task.h
     ${CMAKE_SOURCE_DIR}/src/lua/utils.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/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 0a1ac611e..ce2c37eb9 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 df853215c..36d0b86d4 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -167,10 +167,11 @@ key_def_swap(struct key_def *old_def, struct key_def *new_def);
 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 ee67cf533..f11ffacff 100644
--- a/test/app-tap/CMakeLists.txt
+++ b/test/app-tap/CMakeLists.txt
@@ -1 +1,7 @@
 build_module(module_api module_api.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 4abe1af48..69d22d44b 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 1.10 04/16] WIP: 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.

XXX: Write a test for the new module API calls.

[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

(backported from commit da6ca8df638d041ed558480a601f2cee42e41318)
---
 extra/exports |   4 ++
 src/fiber.c   |  24 ++++++++++++
 src/fiber.h   | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

diff --git a/extra/exports b/extra/exports
index 5f69e0730..52bb29d68 100644
--- a/extra/exports
+++ b/extra/exports
@@ -209,6 +209,10 @@ clock_realtime64
 clock_monotonic64
 clock_process64
 clock_thread64
+box_region_aligned_alloc
+box_region_alloc
+box_region_truncate
+box_region_used
 
 # Lua / LuaJIT
 
diff --git a/src/fiber.c b/src/fiber.c
index ecc59c783..03702556c 100644
--- a/src/fiber.c
+++ b/src/fiber.c
@@ -1119,6 +1119,30 @@ fiber_destroy_all(struct cord *cord)
 						      struct fiber, link));
 }
 
+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/fiber.h b/src/fiber.h
index c8f971b35..fb177d565 100644
--- a/src/fiber.h
+++ b/src/fiber.h
@@ -313,6 +313,107 @@ 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.
+ *
+ * Another useful property is that data allocated on this region
+ * are freed eventually, so a cold path may omit
+ * <box_region_truncate>() call: it will not lead to a memory
+ * leak. It is not recommended to exploit this property on a hot
+ * path, because of the risk to exhaust available memory for this
+ * region before it'll be freed.
+ *
+ * @sa <fiber_gc>() (internal function)
+ */
+
+/** How much memory is used by the box region. */
+API_EXPORT size_t
+box_region_used(void);
+
+/** Allocate size bytes from the box region. */
+API_EXPORT void *
+box_region_alloc(size_t size);
+
+/**
+ * Allocate size bytes from the box region with given alingment.
+ */
+API_EXPORT void *
+box_region_aligned_alloc(size_t size, size_t alignment);
+
+/*
+ * Allocate storage for an object of given type with respect to
+ * its alignment.
+ *
+ * @param T    type of an object
+ * @param size where to store size of an object (size_t *)
+ */
+#define box_region_alloc_object(T, size) ({					\
+	*(size) = sizeof(T);							\
+	(T *)box_region_aligned_alloc(sizeof(T), alignof(T));			\
+})
+
+/*
+ * Allocate storage for array of objects of given type.
+ *
+ * @param T     type of an object
+ * @param count amount of objects to allocate (size_t)
+ * @param size  where to store size of an object (size_t *)
+ */
+#define box_region_alloc_array(T, count, size) ({				\
+	int _tmp_ = sizeof(T) * (count);					\
+	*(size) = _tmp_;							\
+	(T *)box_region_aligned_alloc(_tmp_, alignof(T));			\
+})
+
+/**
+ * Truncate the box region to the given size.
+ */
+void
+box_region_truncate(size_t size);
+
 /** \endcond public */
 
 /**
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 05/16] WIP: 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.

XXX: Add a test to test/app-tap/module_api.{c,test.lua}.

Part of #5273

(cherry picked from commit 98551e8b90ebd2839dee57fee13f446aee515e42)
---
 extra/exports   |  1 +
 src/lua/utils.c |  6 ++++++
 src/lua/utils.h | 20 ++++++++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/extra/exports b/extra/exports
index 52bb29d68..a7d8ef81b 100644
--- a/extra/exports
+++ b/extra/exports
@@ -117,6 +117,7 @@ coio_close
 coio_call
 coio_getaddrinfo
 luaL_pushcdata
+luaL_iscdata
 luaL_checkcdata
 luaL_setcdatagc
 luaL_ctypeid
diff --git a/src/lua/utils.c b/src/lua/utils.c
index d88efc560..e555607cf 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -102,6 +102,12 @@ luaL_pushcdata(struct lua_State *L, uint32_t ctypeid)
 	return cdataptr(cd);
 }
 
+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 5164eb5bf..fe6728986 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -69,6 +69,26 @@ extern struct ibuf *tarantool_lua_ibuf;
 
 /** \cond public */
 
+/**
+ * @brief 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 stack index
+ *
+ * @return 1 if the value at the given acceptable index is a cdata
+ * and 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
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 06/16] WIP: 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).

XXX: Add a module API test. There is unit test already, so maybe just
verify some basic case in the module API test. Or, better, transform the
unit test into a module API test.

Part of #5273

(cherry picked from commit b525317d9056982ecfeff92e2e436554e0a605fd)
---
 extra/exports       |  1 +
 src/box/lua/tuple.c |  6 +++---
 src/box/lua/tuple.h | 22 ++++++++++++++++++----
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/extra/exports b/extra/exports
index a7d8ef81b..7f8a27ea2 100644
--- a/extra/exports
+++ b/extra/exports
@@ -130,6 +130,7 @@ luaL_touint64
 luaL_toint64
 luaT_pushtuple
 luaT_istuple
+luaT_tuple_new
 luaT_error
 luaT_call
 luaT_cpcall
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 8a8fd4d34..6732ac71a 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -97,7 +97,7 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-struct tuple *
+box_tuple_t *
 luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
 {
 	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
@@ -126,8 +126,8 @@ 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));
+	box_tuple_t *tuple = box_tuple_new(format, buf->buf,
+					   buf->buf + ibuf_used(buf));
 	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 dd3aa0f08..196a38c2d 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -67,8 +67,6 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
 box_tuple_t *
 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.
@@ -76,11 +74,27 @@ luaT_istuple(struct lua_State *L, int idx);
  * Set idx to zero to create the new tuple from objects on the lua
  * stack.
  *
- * In case of an error set a diag and return NULL.
+ * 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()
+ *
+ * If encoding fails, raise an error.
+ *
+ * In case of any other 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)
 {
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 07/16] 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 Lua shared ibuf (<tarantool_lua_ibuf>).

The reason to introduce 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).

XXX: Add a module API test.

Part of #5273

(cherry picked from commit 5cbb2e41bb32a3184d2663f678b705aef823dc85)
---
 extra/exports       |  1 +
 src/box/lua/tuple.c | 21 +++++++++++++++++----
 src/box/lua/tuple.h | 29 +++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/extra/exports b/extra/exports
index 7f8a27ea2..7feaaa05e 100644
--- a/extra/exports
+++ b/extra/exports
@@ -130,6 +130,7 @@ luaL_touint64
 luaL_toint64
 luaT_pushtuple
 luaT_istuple
+luaT_tuple_encode
 luaT_tuple_new
 luaT_error
 luaT_call
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 6732ac71a..ddc4f6087 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -97,8 +97,8 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-box_tuple_t *
-luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
+char *
+luaT_tuple_encode(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",
@@ -126,8 +126,21 @@ 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);
-	box_tuple_t *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;
+
+}
+
+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(L, idx, &tuple_len);
+	if (tuple_data == NULL)
+		return NULL;
+	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 196a38c2d..fcb7a9d87 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -67,6 +67,35 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
 box_tuple_t *
 luaT_istuple(struct lua_State *L, int idx);
 
+/**
+ * Encode a Lua table, a tuple or objects on a Lua stack as raw
+ * tuple data (MsgPack).
+ *
+ * @param L             Lua state
+ * @param idx           acceptable index on the Lua stack
+ *                      (or zero, see below)
+ * @param tuple_len_ptr where to store tuple data size in bytes
+ *                      (or NULL)
+ *
+ * Set idx to zero to create the new tuple from objects on the Lua
+ * stack.
+ *
+ * The data is encoded on the shared buffer: so called
+ * <tarantool_lua_ibuf> (it also available as <buffer.SHARED_IBUF>
+ * in Lua). The data is valid until next similar call. It is
+ * generally safe to pass the result to a box function (copy it if
+ * doubt). No need to release this buffer explicitly, it'll be
+ * reused by later calls.
+ *
+ * If encoding fails, raise an error.
+ *
+ * In case of any other 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);
+
 /**
  * Create a new tuple with specific format from a Lua table, a
  * tuple, or objects on the lua stack.
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 08/16] WIP: refactoring: add API_EXPORT to lua/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

(cherry picked from commit 4a95a64520b81ae7c61982801b7aab7f5724e964)
---
 src/box/lua/tuple.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index fcb7a9d87..2aed7a811 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" {
@@ -53,7 +54,7 @@ typedef struct tuple_format box_tuple_format_t;
  * @sa luaT_istuple
  * @throws on OOM
  */
-void
+API_EXPORT void
 luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
 
 /**
@@ -64,7 +65,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 1.10 09/16] WIP: refactoring: 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')

(cherry picked from commit 87223d32305b6c803c48f22a2319bffa90a64e4a)
---
 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 36d0b86d4..7623f650d 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -182,7 +182,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);
 
 /**
@@ -190,7 +190,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);
 
 /**
@@ -202,7 +202,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(const box_tuple_t *tuple_a, const box_tuple_t *tuple_b,
 		  box_key_def_t *key_def);
 
@@ -217,7 +217,7 @@ box_tuple_compare(const box_tuple_t *tuple_a, const box_tuple_t *tuple_b,
  * @retval >0 if key_fields(tuple) > parts(key)
  */
 
-int
+API_EXPORT int
 box_tuple_compare_with_key(const box_tuple_t *tuple_a, const char *key_b,
 			   box_key_def_t *key_def);
 
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 10/16] WIP: refactoring: extract key_def module API functions

[Alexander Turenko]

I plan to expand module API for key_def in further commits and mix of
public and private structures and functions becomes hard to read. So it
looks meaningful to extract module API wrappers into its own compilation
unit.

Added libtuple.a to the so called reexport libraries list, because
otherwise the functions from key_def_api compilation unit are not
exported on 1.10 (in the backported patch).

Part of #5273

(backported from commit cd326628673cc6f2cbd0262ace543e70ae11433e)
---
 src/CMakeLists.txt              |   4 +-
 src/box/CMakeLists.txt          |   1 +
 src/box/key_def.c               |  53 +++--------------
 src/box/key_def.h               |  86 ++++++++++-----------------
 src/box/key_def_api.c           |  75 ++++++++++++++++++++++++
 src/box/key_def_api.h           | 101 ++++++++++++++++++++++++++++++++
 test/unit/vy_iterators_helper.c |   1 +
 test/unit/vy_mem.c              |   1 +
 test/unit/vy_point_lookup.c     |   1 +
 test/unit/vy_write_iterator.c   |   1 +
 10 files changed, 221 insertions(+), 103 deletions(-)
 create mode 100644 src/box/key_def_api.c
 create mode 100644 src/box/key_def_api.h

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 45a8f7733..18a0f5172 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -198,7 +198,7 @@ set(api_headers
     ${CMAKE_SOURCE_DIR}/src/lua/utils.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/key_def_api.h
     ${CMAKE_SOURCE_DIR}/src/box/field_def.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple_format.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple_extract_key.h
@@ -230,7 +230,7 @@ target_link_libraries(server core bit uri uuid ${ICU_LIBRARIES})
 # Rule of thumb: if exporting a symbol from a static library, list the
 # library here.
 set (reexport_libraries server core misc bitset csv
-     ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} ${ICU_LIBRARIES})
+     tuple ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} ${ICU_LIBRARIES})
 
 set (common_libraries
     ${reexport_libraries}
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 52413d3cf..e1af1d9e4 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -41,6 +41,7 @@ add_library(tuple STATIC
     tuple_bloom.c
     tuple_dictionary.c
     key_def.c
+    key_def_api.c
     coll_id_def.c
     coll_id.c
     coll_id_cache.c
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 545b5dad2..1fdb681a2 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -121,7 +121,7 @@ key_def_delete(struct key_def *def)
 	free(def);
 }
 
-static void
+void
 key_def_set_cmp(struct key_def *def)
 {
 	def->tuple_compare = tuple_compare_create(def);
@@ -192,50 +192,6 @@ key_def_dump_parts(const struct key_def *def, struct key_part_def *parts)
 	}
 }
 
-box_key_def_t *
-box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
-{
-	size_t sz = key_def_sizeof(part_count);
-	struct key_def *key_def = calloc(1, sz);
-	if (key_def == NULL) {
-		diag_set(OutOfMemory, sz, "malloc", "struct key_def");
-		return NULL;
-	}
-
-	key_def->part_count = part_count;
-	key_def->unique_part_count = part_count;
-
-	for (uint32_t item = 0; item < part_count; ++item) {
-		key_def_set_part(key_def, item, fields[item],
-				 (enum field_type)types[item],
-				 false, NULL, COLL_NONE);
-	}
-	key_def_set_cmp(key_def);
-	return key_def;
-}
-
-void
-box_key_def_delete(box_key_def_t *key_def)
-{
-	key_def_delete(key_def);
-}
-
-int
-box_tuple_compare(const box_tuple_t *tuple_a, const box_tuple_t *tuple_b,
-		  box_key_def_t *key_def)
-{
-	return tuple_compare(tuple_a, tuple_b, key_def);
-}
-
-int
-box_tuple_compare_with_key(const box_tuple_t *tuple_a, const char *key_b,
-			   box_key_def_t *key_def)
-{
-	uint32_t part_count = mp_decode_array(&key_b);
-	return tuple_compare_with_key(tuple_a, key_b, part_count, key_def);
-
-}
-
 int
 key_part_cmp(const struct key_part *parts1, uint32_t part_count1,
 	     const struct key_part *parts2, uint32_t part_count2)
@@ -572,3 +528,10 @@ key_validate_parts(const struct key_def *key_def, const char *key,
 	}
 	return 0;
 }
+
+void
+key_def_set_part_175(struct key_def *def, uint32_t part_no, uint32_t fieldno,
+		     enum field_type type)
+{
+	key_def_set_part(def, part_no, fieldno, type, false, NULL, COLL_NONE);
+}
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 7623f650d..96993c0a4 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -167,62 +167,6 @@ key_def_swap(struct key_def *old_def, struct key_def *new_def);
 void
 key_def_delete(struct key_def *def);
 
-typedef struct tuple box_tuple_t;
-
-/** \cond public */
-
-typedef struct key_def box_key_def_t;
-
-/**
- * Create key definition with key fields with passed typed on passed positions.
- * May be used for tuple format creation and/or tuple comparison.
- *
- * \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
- * \returns a new key definition object
- */
-API_EXPORT box_key_def_t *
-box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
-
-/**
- * Delete key definition
- *
- * \param key_def key definition to delete
- */
-API_EXPORT void
-box_key_def_delete(box_key_def_t *key_def);
-
-/**
- * Compare tuples using the key definition.
- * @param tuple_a first tuple
- * @param tuple_b second tuple
- * @param key_def key definition
- * @retval 0  if key_fields(tuple_a) == key_fields(tuple_b)
- * @retval <0 if key_fields(tuple_a) < key_fields(tuple_b)
- * @retval >0 if key_fields(tuple_a) > key_fields(tuple_b)
- */
-API_EXPORT int
-box_tuple_compare(const box_tuple_t *tuple_a, const box_tuple_t *tuple_b,
-		  box_key_def_t *key_def);
-
-/**
- * @brief Compare tuple with key using the key definition.
- * @param tuple tuple
- * @param key key with MessagePack array header
- * @param key_def key definition
- *
- * @retval 0  if key_fields(tuple) == parts(key)
- * @retval <0 if key_fields(tuple) < parts(key)
- * @retval >0 if key_fields(tuple) > parts(key)
- */
-
-API_EXPORT int
-box_tuple_compare_with_key(const box_tuple_t *tuple_a, const char *key_b,
-			   box_key_def_t *key_def);
-
-/** \endcond public */
-
 static inline size_t
 key_def_sizeof(uint32_t part_count)
 {
@@ -507,6 +451,36 @@ tuple_compare_with_key(const struct tuple *tuple, const char *key,
 	return key_def->tuple_compare_with_key(tuple, key, part_count, key_def);
 }
 
+/* {{{ <box_key_def_new>() helpers */
+
+/**
+ * Set key part within @a key_def.
+ *
+ * The same as module private <key_def_set_part>(), but with less
+ * parameters. It is helper for <box_key_def_new>().
+ *
+ * 1.7.5 does not support collation, nullability and further
+ * key_def features.
+ */
+void
+key_def_set_part_175(struct key_def *def, uint32_t part_no, uint32_t fieldno,
+		     enum field_type type);
+
+/**
+ * Update compare, hash and extract functions.
+ *
+ * This function should be called after modification of
+ * @a key_def parts, because different compare, hash and extract
+ * functions should work depending of whether key parts are
+ * sequential, are nullable, whether a tuple may omit several
+ * fields at the end, whether a collation is used and so on.
+ * It is helper for <box_key_def_new>().
+ */
+void
+key_def_set_cmp(struct key_def *def);
+
+/* }}} <box_key_def_new>() helpers */
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
new file mode 100644
index 000000000..104ec5d5c
--- /dev/null
+++ b/src/box/key_def_api.c
@@ -0,0 +1,75 @@
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include "key_def_api.h"
+#include "key_def.h"
+
+box_key_def_t *
+box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
+{
+	size_t sz = key_def_sizeof(part_count);
+	struct key_def *key_def = calloc(1, sz);
+	if (key_def == NULL) {
+		diag_set(OutOfMemory, sz, "malloc", "struct key_def");
+		return NULL;
+	}
+
+	key_def->part_count = part_count;
+	key_def->unique_part_count = part_count;
+
+	for (uint32_t item = 0; item < part_count; ++item) {
+		key_def_set_part_175(key_def, item, fields[item],
+				     (enum field_type)types[item]);
+	}
+	key_def_set_cmp(key_def);
+	return key_def;
+}
+
+void
+box_key_def_delete(box_key_def_t *key_def)
+{
+	key_def_delete(key_def);
+}
+
+int
+box_tuple_compare(const box_tuple_t *tuple_a, const box_tuple_t *tuple_b,
+		  box_key_def_t *key_def)
+{
+	return tuple_compare(tuple_a, tuple_b, key_def);
+}
+
+int
+box_tuple_compare_with_key(const box_tuple_t *tuple_a, const char *key_b,
+			   box_key_def_t *key_def)
+{
+	uint32_t part_count = mp_decode_array(&key_b);
+	return tuple_compare_with_key(tuple_a, key_b, part_count, key_def);
+
+}
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
new file mode 100644
index 000000000..82899eed6
--- /dev/null
+++ b/src/box/key_def_api.h
@@ -0,0 +1,101 @@
+#ifndef TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
+#define TARANTOOL_BOX_KEY_DEF_API_H_INCLUDED
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+#include <stdint.h>
+#include "trivia/util.h"
+
+typedef struct tuple box_tuple_t;
+
+/** \cond public */
+
+typedef struct key_def box_key_def_t;
+
+/**
+ * Create key definition with given field numbers and field types.
+ *
+ * May be used for tuple format creation and/or tuple comparison.
+ *
+ * \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
+ * \returns a new key definition object
+ */
+API_EXPORT box_key_def_t *
+box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
+
+/**
+ * Delete key definition
+ *
+ * \param key_def key definition to delete
+ */
+API_EXPORT void
+box_key_def_delete(box_key_def_t *key_def);
+
+/**
+ * Compare tuples using the key definition.
+ * @param tuple_a first tuple
+ * @param tuple_b second tuple
+ * @param key_def key definition
+ * @retval 0  if key_fields(tuple_a) == key_fields(tuple_b)
+ * @retval <0 if key_fields(tuple_a) < key_fields(tuple_b)
+ * @retval >0 if key_fields(tuple_a) > key_fields(tuple_b)
+ */
+API_EXPORT int
+box_tuple_compare(const box_tuple_t *tuple_a, const box_tuple_t *tuple_b,
+		  box_key_def_t *key_def);
+
+/**
+ * @brief Compare tuple with key using the key definition.
+ * @param tuple tuple
+ * @param key key with MessagePack array header
+ * @param key_def key definition
+ *
+ * @retval 0  if key_fields(tuple) == parts(key)
+ * @retval <0 if key_fields(tuple) < parts(key)
+ * @retval >0 if key_fields(tuple) > parts(key)
+ */
+API_EXPORT int
+box_tuple_compare_with_key(const box_tuple_t *tuple_a, const char *key_b,
+			   box_key_def_t *key_def);
+
+/** \endcond public */
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_BOX_KEY_API_DEF_H_INCLUDED */
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index 7fad5600c..063cf1e6d 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -1,3 +1,4 @@
+#include "key_def_api.h"
 #include "vy_iterators_helper.h"
 #include "memory.h"
 #include "fiber.h"
diff --git a/test/unit/vy_mem.c b/test/unit/vy_mem.c
index ebf3fbc77..e82991bfd 100644
--- a/test/unit/vy_mem.c
+++ b/test/unit/vy_mem.c
@@ -1,6 +1,7 @@
 #include <trivia/config.h>
 #include "memory.h"
 #include "fiber.h"
+#include "key_def_api.h"
 #include "vy_history.h"
 #include "vy_iterators_helper.h"
 
diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c
index bec85fa02..546411dc5 100644
--- a/test/unit/vy_point_lookup.c
+++ b/test/unit/vy_point_lookup.c
@@ -1,5 +1,6 @@
 #include "trivia/util.h"
 #include "unit.h"
+#include "key_def_api.h"
 #include "vy_lsm.h"
 #include "vy_cache.h"
 #include "vy_run.h"
diff --git a/test/unit/vy_write_iterator.c b/test/unit/vy_write_iterator.c
index bb0eb8d39..1fe220e32 100644
--- a/test/unit/vy_write_iterator.c
+++ b/test/unit/vy_write_iterator.c
@@ -1,5 +1,6 @@
 #include "memory.h"
 #include "fiber.h"
+#include "key_def_api.h"
 #include "vy_write_iterator.h"
 #include "vy_iterators_helper.h"
 
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 11/16] WIP: module api: add box_key_def_new_ex()

[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 techinal 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.

- 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.

XXX: Add a module API test.

Part of #5273

(backported from commit ae27cccc2f967282e05fd76d4d5624ecb9bbb8b2)
---
 extra/exports         |   2 +
 src/box/key_def_api.c | 123 ++++++++++++++++++++++++++++++++++++++++++
 src/box/key_def_api.h | 103 +++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+)

diff --git a/extra/exports b/extra/exports
index 7feaaa05e..b70560db7 100644
--- a/extra/exports
+++ b/extra/exports
@@ -147,6 +147,8 @@ box_txn_alloc
 box_txn_id
 box_key_def_new
 box_key_def_delete
+box_key_def_new_ex
+box_key_part_def_create
 box_tuple_format_default
 box_tuple_new
 box_tuple_ref
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 104ec5d5c..f19f30950 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -30,6 +30,67 @@
  */
 #include "key_def_api.h"
 #include "key_def.h"
+#include "small/region.h"
+#include "coll_id_cache.h"
+#include "tuple_format.h"
+#include "field_def.h"
+#include "coll_id_cache.h"
+#include "fiber.h"
+
+/* {{{ Helpers */
+
+static int
+key_def_set_internal_part(struct key_part_def *internal_part,
+			  box_key_part_def_t *part)
+{
+	*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. */
+	internal_part->is_nullable =
+		(part->flags & BOX_KEY_PART_DEF_IS_NULLABLE_MASK) ==
+		BOX_KEY_PART_DEF_IS_NULLABLE_MASK;
+
+	/* 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;
+	}
+
+	return 0;
+}
+
+/* }}} Helpers */
+
+/* {{{ API functions implementations */
+
+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(uint32_t *fields, uint32_t *types, uint32_t part_count)
@@ -52,6 +113,66 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
 	return key_def;
 }
 
+box_key_def_t *
+box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count)
+{
+	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 (parts == NULL) {
+		diag_set(OutOfMemory, internal_parts_size, "region_alloc_array",
+			 "parts");
+		return NULL;
+	}
+	if (part_count == 0) {
+		diag_set(IllegalParams, "At least one key part is required");
+		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]) != 0) {
+			region_truncate(region, region_svp);
+			return NULL;
+		}
+		bool is_nullable =
+			(parts[i].flags & BOX_KEY_PART_DEF_IS_NULLABLE_MASK) ==
+			BOX_KEY_PART_DEF_IS_NULLABLE_MASK;
+		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);
+	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)
 {
@@ -73,3 +194,5 @@ box_tuple_compare_with_key(const box_tuple_t *tuple_a, const char *key_b,
 	return tuple_compare_with_key(tuple_a, key_b, part_count, key_def);
 
 }
+
+/* }}} API functions implementations */
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index 82899eed6..6025afe67 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -44,11 +44,84 @@ typedef struct tuple box_tuple_t;
 
 typedef struct key_def box_key_def_t;
 
+/** Key part definition flags. */
+enum {
+	BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT = 0,
+	BOX_KEY_PART_DEF_IS_NULLABLE_MASK =
+		1 << BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT,
+};
+
+/**
+ * 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_ex>() 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,
+};
+
+/**
+ * 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_ex>() 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_ex>().
+ *
+ * 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;
+	};
+	/**
+	 * 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_ex>().
+ *
  * \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
@@ -57,6 +130,28 @@ 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.
+ *
+ * All trailing padding bytes are set to zero.
+ *
+ * All unknown <flags> bits are set to zero.
+ */
+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_ex(box_key_part_def_t *parts, uint32_t part_count);
+
 /**
  * Delete key definition
  *
@@ -94,6 +189,14 @@ box_tuple_compare_with_key(const 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)");
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 12/16] WIP: module api: add box_key_def_dump_parts()

[Alexander Turenko]

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

XXX: Add a module API test.

Part of #5273

(backported from commit 94bccf8d2647debb1c1184af95713d48acf4c504)
---
 extra/exports         |  1 +
 src/box/key_def_api.c | 50 +++++++++++++++++++++++++++++++++++++++++++
 src/box/key_def_api.h |  9 ++++++++
 3 files changed, 60 insertions(+)

diff --git a/extra/exports b/extra/exports
index b70560db7..97d2a21b8 100644
--- a/extra/exports
+++ b/extra/exports
@@ -147,6 +147,7 @@ box_txn_alloc
 box_txn_id
 box_key_def_new
 box_key_def_delete
+box_key_def_dump_parts
 box_key_def_new_ex
 box_key_part_def_create
 box_tuple_format_default
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index f19f30950..30ddde9c8 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -179,6 +179,56 @@ 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 (part->is_nullable)
+			part_def->flags |= BOX_KEY_PART_DEF_IS_NULLABLE_MASK;
+		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;
+			}
+			part_def->collation = coll_id->name;
+		}
+	}
+
+	if (part_count_ptr != NULL)
+		*part_count_ptr = key_def->part_count;
+
+	return parts;
+}
+
 int
 box_tuple_compare(const box_tuple_t *tuple_a, const box_tuple_t *tuple_b,
 		  box_key_def_t *key_def)
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index 6025afe67..ee967c510 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -160,6 +160,15 @@ box_key_def_new_ex(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 on the box region.
+ * @sa <box_region_truncate>().
+ */
+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
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 13/16] WIP: module api: expose box_tuple_validate_key_parts()

[Alexander Turenko]

XXX: Add a module API test.

Part of #5273

(backported from commit 37b501a1982f903cef55e90e89fe3d42931e93d9)
---
 extra/exports         |  1 +
 src/box/key_def_api.c | 23 +++++++++++++++++++++++
 src/box/key_def_api.h | 12 ++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/extra/exports b/extra/exports
index 97d2a21b8..2afc36def 100644
--- a/extra/exports
+++ b/extra/exports
@@ -171,6 +171,7 @@ box_tuple_next
 box_tuple_update
 box_tuple_upsert
 box_tuple_extract_key
+box_tuple_validate_key_parts
 box_tuple_compare
 box_tuple_compare_with_key
 box_return_tuple
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 30ddde9c8..29203319c 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -36,6 +36,8 @@
 #include "field_def.h"
 #include "coll_id_cache.h"
 #include "fiber.h"
+#include "tuple.h"
+#include <msgpuck.h>
 
 /* {{{ Helpers */
 
@@ -229,6 +231,27 @@ box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr)
 	return parts;
 }
 
+int
+box_tuple_validate_key_parts(box_key_def_t *key_def, box_tuple_t *tuple)
+{
+	for (uint32_t idx = 0; idx < key_def->part_count; ++idx) {
+		struct key_part *part = &key_def->parts[idx];
+		const char *field = tuple_field_by_part(tuple, part);
+		if (field == NULL) {
+			if (part->is_nullable)
+				continue;
+			diag_set(ClientError, ER_NO_SUCH_FIELD,
+				 part->fieldno + TUPLE_INDEX_BASE);
+			return -1;
+		}
+		enum mp_type mp_type = mp_typeof(*field);
+		if (key_mp_type_validate(part->type, mp_type, ER_FIELD_TYPE,
+					 idx, part->is_nullable) != 0)
+			return -1;
+	}
+	return 0;
+}
+
 int
 box_tuple_compare(const box_tuple_t *tuple_a, const box_tuple_t *tuple_b,
 		  box_key_def_t *key_def)
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index ee967c510..bdc3ecebd 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -169,6 +169,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_tuple_validate_key_parts(box_key_def_t *key_def, box_tuple_t *tuple);
+
 /**
  * Compare tuples using the key definition.
  * @param tuple_a first tuple
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 14/16] WIP: module api: expose box_key_def_merge()

[Alexander Turenko]

Part of #5273

(cherry picked from commit de2594c6f3b2c45e07c53323c9201f5db9f1f67e)
---
 extra/exports         |  1 +
 src/box/key_def_api.c |  6 ++++++
 src/box/key_def_api.h | 14 ++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/extra/exports b/extra/exports
index 2afc36def..696ff2f30 100644
--- a/extra/exports
+++ b/extra/exports
@@ -149,6 +149,7 @@ box_key_def_new
 box_key_def_delete
 box_key_def_dump_parts
 box_key_def_new_ex
+box_key_def_merge
 box_key_part_def_create
 box_tuple_format_default
 box_tuple_new
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index 29203319c..a9cd95173 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -268,4 +268,10 @@ box_tuple_compare_with_key(const 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);
+}
+
 /* }}} API functions implementations */
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index bdc3ecebd..e3febe69e 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -208,6 +208,20 @@ API_EXPORT int
 box_tuple_compare_with_key(const 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 */
 
 /*
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 15/16] WIP: module api: expose box_tuple_extract_key_ex()

[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. Also there is no <MULTIKEY_NONE> constant in the
backport.

XXX: Add a module API test.

Part of #5273

(backported from commit c89c9a8c647943bff138f980fcd44ca7bc96bd25)
---
 extra/exports         |  1 +
 src/box/key_def_api.c |  8 ++++++++
 src/box/key_def_api.h | 20 ++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/extra/exports b/extra/exports
index 696ff2f30..1684da57d 100644
--- a/extra/exports
+++ b/extra/exports
@@ -172,6 +172,7 @@ box_tuple_next
 box_tuple_update
 box_tuple_upsert
 box_tuple_extract_key
+box_tuple_extract_key_ex
 box_tuple_validate_key_parts
 box_tuple_compare
 box_tuple_compare_with_key
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index a9cd95173..cce605e35 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -274,4 +274,12 @@ box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second)
 	return key_def_merge(first, second);
 }
 
+char *
+box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
+			 int ignored, uint32_t *key_size_ptr)
+{
+	(void) ignored;
+	return tuple_extract_key(tuple, key_def, key_size_ptr);
+}
+
 /* }}} API functions implementations */
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index e3febe69e..cf4f3c9b7 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -222,6 +222,26 @@ box_tuple_compare_with_key(const 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 tuple - tuple from which need to extract key
+ * @param key_def - definition of key that need to extract
+ * @param ignored - ignored on this version of tarantool
+ * @param key_size_ptr - here will be size of extracted key
+ *
+ * @retval not NULL Success
+ * @retval NULL     Memory allocation error
+ */
+API_EXPORT char *
+box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
+			 int ignored, uint32_t *key_size_ptr);
+
 /** \endcond public */
 
 /*
-- 
2.25.0

[Tarantool-patches] [PATCH 1.10 16/16] WIP: module api: add box_key_def_validate_key()

[Alexander Turenko]

XXX: Add a module API test.

Part of #5273

(cherry picked from commit 501646b13840c7f8166e47c7ce75385b76dc00b0)
---
 extra/exports         |  1 +
 src/box/key_def_api.c | 13 +++++++++++++
 src/box/key_def_api.h | 15 +++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/extra/exports b/extra/exports
index 1684da57d..a4ac03530 100644
--- a/extra/exports
+++ b/extra/exports
@@ -150,6 +150,7 @@ box_key_def_delete
 box_key_def_dump_parts
 box_key_def_new_ex
 box_key_def_merge
+box_key_def_validate_key
 box_key_part_def_create
 box_tuple_format_default
 box_tuple_new
diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
index cce605e35..55fbc53ce 100644
--- a/src/box/key_def_api.c
+++ b/src/box/key_def_api.c
@@ -282,4 +282,17 @@ box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
 	return tuple_extract_key(tuple, key_def, key_size_ptr);
 }
 
+int
+box_key_def_validate_key(const box_key_def_t *key_def, const char *key,
+			 bool allow_nullable)
+{
+	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;
+	}
+	return key_validate_parts(key_def, key, part_count, allow_nullable);
+}
+
 /* }}} API functions implementations */
diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
index cf4f3c9b7..b85ed7f3c 100644
--- a/src/box/key_def_api.h
+++ b/src/box/key_def_api.h
@@ -36,6 +36,7 @@ extern "C" {
 #endif /* defined(__cplusplus) */
 
 #include <stdint.h>
+#include <stdbool.h>
 #include "trivia/util.h"
 
 typedef struct tuple box_tuple_t;
@@ -242,6 +243,20 @@ API_EXPORT char *
 box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
 			 int ignored, 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.
+ * @param allow_nullable True if nullable parts are allowed.
+ *
+ * @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,
+			 bool allow_nullable);
+
 /** \endcond public */
 
 /*
-- 
2.25.0

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

[Alexander Turenko]

> What also worries me: I found that actual behaviour of luaT_tuple_new()
> is different from what I expected in the past: it may raise an error
> when serialization fails. I didn't expect this, so I'll look how much it
> may affect key_def and merger modules (built-in and external ones). For
> now I just described the actual contract in the API comments for
> luaT_tuple_new() and luaT_tuple_encode().

I looked at usages of the function and found leaks and inconsistences
caused by raising a Lua error from luaT_tuple_new(). Filed issue:

https://github.com/tarantool/tarantool/issues/5382

So it should be resolved before exposing the function to the module API.
I'll do.

WBR, Alexander Turenko.