* [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module @ 2020-09-23 1:14 Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 01/14] module api: get rid of typedef redefinitions Alexander Turenko ` (15 more replies) 0 siblings, 16 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 01/14] module api: get rid of typedef redefinitions 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region Alexander Turenko ` (14 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 01/14] module api: get rid of typedef redefinitions Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-24 22:31 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko ` (13 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region Alexander Turenko @ 2020-09-24 22:31 ` Vladislav Shpilevoy 2020-10-08 19:21 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-24 22:31 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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 */ > > /** > ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region 2020-09-24 22:31 ` Vladislav Shpilevoy @ 2020-10-08 19:21 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-08 19:21 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches 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); ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 01/14] module api: get rid of typedef redefinitions Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-24 22:32 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 04/14] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko ` (12 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko @ 2020-09-24 22:32 ` Vladislav Shpilevoy 2020-10-08 21:46 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-24 22:32 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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 > ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function 2020-09-24 22:32 ` Vladislav Shpilevoy @ 2020-10-08 21:46 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-08 21:46 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > > 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 04/14] WIP: module api/lua: expose luaT_tuple_new() 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (2 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko ` (11 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (3 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 04/14] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-24 22:32 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 06/14] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko ` (10 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko @ 2020-09-24 22:32 ` Vladislav Shpilevoy 2020-10-12 19:06 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-24 22:32 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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); ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() 2020-09-24 22:32 ` Vladislav Shpilevoy @ 2020-10-12 19:06 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-12 19:06 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > > +/** > > + * 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 06/14] WIP: refactoring: add API_EXPORT to lua/tuple functions 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (4 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 07/14] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko ` (9 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 07/14] WIP: refactoring: add API_EXPORT to key_def functions 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (5 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 06/14] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions Alexander Turenko ` (8 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (6 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 07/14] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() Alexander Turenko ` (7 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions Alexander Turenko @ 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-07 11:30 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-25 22:58 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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. > + */ ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions 2020-09-25 22:58 ` Vladislav Shpilevoy @ 2020-10-07 11:30 ` Alexander Turenko 2020-10-07 22:12 ` Vladislav Shpilevoy 0 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-10-07 11:30 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions 2020-10-07 11:30 ` Alexander Turenko @ 2020-10-07 22:12 ` Vladislav Shpilevoy 0 siblings, 0 replies; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-10-07 22:12 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (7 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() Alexander Turenko ` (6 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() Alexander Turenko @ 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-09 21:54 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-25 22:58 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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); ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() 2020-09-25 22:58 ` Vladislav Shpilevoy @ 2020-10-09 21:54 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-09 21:54 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > 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); ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (8 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 11/14] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko ` (5 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() Alexander Turenko @ 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-09 9:33 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-25 22:58 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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; > +} ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() 2020-09-25 22:58 ` Vladislav Shpilevoy @ 2020-10-09 9:33 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-09 9:33 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > > 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 11/14] WIP: module api: expose box_tuple_validate_key_parts() 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (9 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() Alexander Turenko ` (4 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (10 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 11/14] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko ` (3 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() Alexander Turenko @ 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-09 1:46 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-25 22:58 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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? ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() 2020-09-25 22:58 ` Vladislav Shpilevoy @ 2020-10-09 1:46 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-09 1:46 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches 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). ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (11 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() Alexander Turenko ` (2 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko @ 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-09 1:14 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-25 22:58 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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 */ ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() 2020-09-25 22:58 ` Vladislav Shpilevoy @ 2020-10-09 1:14 ` Alexander Turenko 2020-10-10 1:21 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-10-09 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() 2020-10-09 1:14 ` Alexander Turenko @ 2020-10-10 1:21 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-10 1:21 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (12 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko @ 2020-09-23 1:14 ` Alexander Turenko 2020-09-25 22:59 ` Vladislav Shpilevoy 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko 2020-10-05 7:26 ` [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:14 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() Alexander Turenko @ 2020-09-25 22:59 ` Vladislav Shpilevoy 2020-10-09 1:22 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-25 22:59 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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); > +} ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() 2020-09-25 22:59 ` Vladislav Shpilevoy @ 2020-10-09 1:22 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-09 1:22 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > > +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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (13 preceding siblings ...) 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 01/16] collation: allow to find a collation by a name Alexander Turenko ` (15 more replies) 2020-10-05 7:26 ` [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko 15 siblings, 16 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 01/16] collation: allow to find a collation by a name 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() Alexander Turenko ` (14 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 01/16] collation: allow to find a collation by a name Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-28 21:26 ` Vladislav Shpilevoy 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 03/16] module api: get rid of typedef redefinitions Alexander Turenko ` (13 subsequent siblings) 15 siblings, 1 reply; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() Alexander Turenko @ 2020-09-28 21:26 ` Vladislav Shpilevoy 2020-10-05 11:58 ` Alexander Turenko 0 siblings, 1 reply; 55+ messages in thread From: Vladislav Shpilevoy @ 2020-09-28 21:26 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches Hi! Thanks for the patch! There is no 'refactoring:' subsystem. Lets use 'lua:'. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() 2020-09-28 21:26 ` Vladislav Shpilevoy @ 2020-10-05 11:58 ` Alexander Turenko 0 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-05 11:58 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches 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.) ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 03/16] module api: get rid of typedef redefinitions 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 01/16] collation: allow to find a collation by a name Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 04/16] WIP: module api: expose box region Alexander Turenko ` (12 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 04/16] WIP: module api: expose box region 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (2 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 03/16] module api: get rid of typedef redefinitions Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 05/16] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko ` (11 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 05/16] WIP: module api/lua: add luaL_iscdata() function 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (3 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 04/16] WIP: module api: expose box region Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 06/16] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko ` (10 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 06/16] WIP: module api/lua: expose luaT_tuple_new() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (4 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 05/16] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 07/16] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko ` (9 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 07/16] WIP: module api/lua: add luaT_tuple_encode() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (5 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 06/16] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 08/16] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko ` (8 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 08/16] WIP: refactoring: add API_EXPORT to lua/tuple functions 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (6 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 07/16] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 09/16] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko ` (7 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 09/16] WIP: refactoring: add API_EXPORT to key_def functions 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (7 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 08/16] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 10/16] WIP: refactoring: extract key_def module API functions Alexander Turenko ` (6 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 10/16] WIP: refactoring: extract key_def module API functions 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (8 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 09/16] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 11/16] WIP: module api: add box_key_def_new_ex() Alexander Turenko ` (5 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 11/16] WIP: module api: add box_key_def_new_ex() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (9 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 10/16] WIP: refactoring: extract key_def module API functions Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 12/16] WIP: module api: add box_key_def_dump_parts() Alexander Turenko ` (4 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 12/16] WIP: module api: add box_key_def_dump_parts() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (10 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 11/16] WIP: module api: add box_key_def_new_ex() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 13/16] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko ` (3 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 13/16] WIP: module api: expose box_tuple_validate_key_parts() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (11 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 12/16] WIP: module api: add box_key_def_dump_parts() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 14/16] WIP: module api: expose box_key_def_merge() Alexander Turenko ` (2 subsequent siblings) 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 14/16] WIP: module api: expose box_key_def_merge() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (12 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 13/16] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 15/16] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 16/16] WIP: module api: add box_key_def_validate_key() Alexander Turenko 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 15/16] WIP: module api: expose box_tuple_extract_key_ex() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (13 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 14/16] WIP: module api: expose box_key_def_merge() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 16/16] WIP: module api: add box_key_def_validate_key() Alexander Turenko 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [Tarantool-patches] [PATCH 1.10 16/16] WIP: module api: add box_key_def_validate_key() 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (14 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 15/16] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko @ 2020-09-23 1:40 ` Alexander Turenko 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-09-23 1:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko ` (14 preceding siblings ...) 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko @ 2020-10-05 7:26 ` Alexander Turenko 15 siblings, 0 replies; 55+ messages in thread From: Alexander Turenko @ 2020-10-05 7:26 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
end of thread, other threads:[~2020-10-12 19:06 UTC | newest] Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-23 1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 01/14] module api: get rid of typedef redefinitions Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region Alexander Turenko 2020-09-24 22:31 ` Vladislav Shpilevoy 2020-10-08 19:21 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko 2020-09-24 22:32 ` Vladislav Shpilevoy 2020-10-08 21:46 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 04/14] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko 2020-09-24 22:32 ` Vladislav Shpilevoy 2020-10-12 19:06 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 06/14] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 07/14] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-07 11:30 ` Alexander Turenko 2020-10-07 22:12 ` Vladislav Shpilevoy 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-09 21:54 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-09 9:33 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 11/14] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-09 1:46 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko 2020-09-25 22:58 ` Vladislav Shpilevoy 2020-10-09 1:14 ` Alexander Turenko 2020-10-10 1:21 ` Alexander Turenko 2020-09-23 1:14 ` [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() Alexander Turenko 2020-09-25 22:59 ` Vladislav Shpilevoy 2020-10-09 1:22 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 01/16] collation: allow to find a collation by a name Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() Alexander Turenko 2020-09-28 21:26 ` Vladislav Shpilevoy 2020-10-05 11:58 ` Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 03/16] module api: get rid of typedef redefinitions Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 04/16] WIP: module api: expose box region Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 05/16] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 06/16] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 07/16] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 08/16] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 09/16] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 10/16] WIP: refactoring: extract key_def module API functions Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 11/16] WIP: module api: add box_key_def_new_ex() Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 12/16] WIP: module api: add box_key_def_dump_parts() Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 13/16] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 14/16] WIP: module api: expose box_key_def_merge() Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 15/16] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko 2020-09-23 1:40 ` [Tarantool-patches] [PATCH 1.10 16/16] WIP: module api: add box_key_def_validate_key() Alexander Turenko 2020-10-05 7:26 ` [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox