* [PATCH 0/4] Reuse tuple formats for ephemeral spaces @ 2019-01-22 14:07 Kirill Yukhin 2019-01-22 14:07 ` Kirill Yukhin 0 siblings, 1 reply; 13+ messages in thread From: Kirill Yukhin @ 2019-01-22 14:07 UTC (permalink / raw) To: vdavydov.dev; +Cc: tarantool-patches, Kirill Yukhin This patchset makes it possible to re-use tuple_format around multiple ephemeral spaces. This is needed when many SQL requests are served at the same time. In this case for almost each non-trivial SELECT request ephemeral space is created with its own format. Since removal of ephemeral spaces doesn't mean removal of format, and formats count is limited, it is pretty much simple to run out of formats. This occurs on TPC-C br Issue https://github.com/tarantool/tarantool/issues/3924 Branch https://github.com/tarantool/tarantool/commits/kyukhin/gh-3924-reuse-eph-formats Kirill Yukhin (4): Pass necessary fields to tuple_format contructor Set is_temporary flag for formats of ephemeral spaces sql: set error type in case of epehemral space creation filure Allow to reuse tuple_formats for ephemeral spaces src/box/blackhole.c | 6 +- src/box/box.cc | 4 + src/box/memtx_engine.c | 6 ++ src/box/memtx_space.c | 9 +- src/box/space.c | 3 +- src/box/space_def.c | 16 ++++ src/box/space_def.h | 14 ++++ src/box/sql.c | 6 +- src/box/sql/vdbe.c | 4 +- src/box/tuple.c | 10 ++- src/box/tuple_format.c | 141 +++++++++++++++++++++++++++++--- src/box/tuple_format.h | 32 +++++++- src/box/vinyl.c | 12 +-- src/box/vy_lsm.c | 10 ++- src/errinj.h | 2 + test/box/errinj.result | 8 +- test/sql/errinj.result | 39 +++++++++ test/sql/errinj.test.lua | 14 ++++ test/unit/tuple_bigref.c | 1 + test/unit/vy_iterators_helper.c | 14 ++-- test/unit/vy_mem.c | 5 +- test/unit/vy_point_lookup.c | 5 +- 22 files changed, 305 insertions(+), 56 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/4] Reuse tuple formats for ephemeral spaces 2019-01-22 14:07 [PATCH 0/4] Reuse tuple formats for ephemeral spaces Kirill Yukhin @ 2019-01-22 14:07 ` Kirill Yukhin 2019-01-22 14:07 ` [PATCH 1/4] Pass necessary fields to tuple_format contructor Kirill Yukhin ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Kirill Yukhin @ 2019-01-22 14:07 UTC (permalink / raw) To: vdavydov.dev; +Cc: tarantool-patches, Kirill Yukhin This patchset makes it possible to re-use tuple_format around multiple ephemeral spaces. This is needed when many SQL requests are served at the same time. In this case for almost each non-trivial SELECT request ephemeral space is created with its own format. Since removal of ephemeral spaces doesn't mean removal of format, and formats count is limited, it is pretty much simple to run out of formats. This occurs on TPC-C benchmark, when number of clients > 32. Issue https://github.com/tarantool/tarantool/issues/3924 Branch https://github.com/tarantool/tarantool/commits/kyukhin/gh-3924-reuse-eph-formats Kirill Yukhin (4): Pass necessary fields to tuple_format contructor Set is_temporary flag for formats of ephemeral spaces sql: set error type in case of ephemral space creation failure Allow to reuse tuple_formats for ephemeral spaces src/box/blackhole.c | 6 +- src/box/box.cc | 4 + src/box/memtx_engine.c | 6 ++ src/box/memtx_space.c | 9 +- src/box/space.c | 3 +- src/box/space_def.c | 16 ++++ src/box/space_def.h | 14 ++++ src/box/sql.c | 6 +- src/box/sql/vdbe.c | 4 +- src/box/tuple.c | 10 ++- src/box/tuple_format.c | 141 +++++++++++++++++++++++++++++--- src/box/tuple_format.h | 32 +++++++- src/box/vinyl.c | 12 +-- src/box/vy_lsm.c | 10 ++- src/errinj.h | 2 + test/box/errinj.result | 8 +- test/sql/errinj.result | 39 +++++++++ test/sql/errinj.test.lua | 14 ++++ test/unit/tuple_bigref.c | 1 + test/unit/vy_iterators_helper.c | 14 ++-- test/unit/vy_mem.c | 5 +- test/unit/vy_point_lookup.c | 5 +- 22 files changed, 305 insertions(+), 56 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] Pass necessary fields to tuple_format contructor 2019-01-22 14:07 ` Kirill Yukhin @ 2019-01-22 14:07 ` Kirill Yukhin 2019-01-23 7:53 ` Vladimir Davydov 2019-01-22 14:07 ` [PATCH 2/4] Set is_temporary flag for formats of ephemeral spaces Kirill Yukhin ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Kirill Yukhin @ 2019-01-22 14:07 UTC (permalink / raw) To: vdavydov.dev; +Cc: tarantool-patches, Kirill Yukhin There were three extra fields of tuple_format which were setup after it was created. Fix that by extending tuple_format contstructor w/ three new arguments: engine, is_temporary, exact_field_count. --- src/box/blackhole.c | 6 +++--- src/box/memtx_space.c | 8 +++----- src/box/tuple.c | 8 ++++---- src/box/tuple_format.c | 13 ++++++++----- src/box/tuple_format.h | 11 ++++++++--- src/box/vinyl.c | 12 +++++++----- src/box/vy_lsm.c | 9 +++++---- test/unit/vy_iterators_helper.c | 12 ++++++------ test/unit/vy_mem.c | 4 ++-- test/unit/vy_point_lookup.c | 4 ++-- 10 files changed, 48 insertions(+), 39 deletions(-) diff --git a/src/box/blackhole.c b/src/box/blackhole.c index 0412ffe..64e24cd 100644 --- a/src/box/blackhole.c +++ b/src/box/blackhole.c @@ -155,13 +155,13 @@ blackhole_engine_create_space(struct engine *engine, struct space_def *def, /* Allocate tuples on runtime arena, but check space format. */ struct tuple_format *format; - format = tuple_format_new(&tuple_format_runtime->vtab, NULL, 0, - def->fields, def->field_count, def->dict); + format = tuple_format_new(&tuple_format_runtime->vtab, NULL, NULL, 0, + def->fields, def->field_count, def->dict, + false, def->exact_field_count); if (format == NULL) { free(space); return NULL; } - format->exact_field_count = def->exact_field_count; tuple_format_ref(format); if (space_create(space, engine, &blackhole_space_vtab, diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index eb790a6..49d09af 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -990,15 +990,13 @@ memtx_space_new(struct memtx_engine *memtx, keys[key_count++] = index_def->key_def; struct tuple_format *format = - tuple_format_new(&memtx_tuple_format_vtab, keys, key_count, - def->fields, def->field_count, def->dict); + tuple_format_new(&memtx_tuple_format_vtab, memtx, keys, key_count, + def->fields, def->field_count, def->dict, + def->opts.is_temporary, def->exact_field_count); if (format == NULL) { free(memtx_space); return NULL; } - format->engine = memtx; - format->is_temporary = def->opts.is_temporary; - format->exact_field_count = def->exact_field_count; tuple_format_ref(format); if (space_create((struct space *)memtx_space, (struct engine *)memtx, diff --git a/src/box/tuple.c b/src/box/tuple.c index 2343f0e..5295800 100644 --- a/src/box/tuple.c +++ b/src/box/tuple.c @@ -207,8 +207,8 @@ tuple_init(field_name_hash_f hash) /* * Create a format for runtime tuples */ - tuple_format_runtime = tuple_format_new(&tuple_format_runtime_vtab, - NULL, 0, NULL, 0, NULL); + tuple_format_runtime = tuple_format_new(&tuple_format_runtime_vtab, NULL, + NULL, 0, NULL, 0, NULL, false, 0); if (tuple_format_runtime == NULL) return -1; @@ -379,8 +379,8 @@ box_tuple_format_t * box_tuple_format_new(struct key_def **keys, uint16_t key_count) { box_tuple_format_t *format = - tuple_format_new(&tuple_format_runtime_vtab, - keys, key_count, NULL, 0, NULL); + tuple_format_new(&tuple_format_runtime_vtab, NULL, + keys, key_count, NULL, 0, NULL, false, 0); if (format != NULL) tuple_format_ref(format); return format; diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index e11b4e6..6edc0fb 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -398,17 +398,20 @@ tuple_format_delete(struct tuple_format *format) } struct tuple_format * -tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys, - uint16_t key_count, const struct field_def *space_fields, - uint32_t space_field_count, struct tuple_dictionary *dict) +tuple_format_new(struct tuple_format_vtab *vtab, void *engine, + struct key_def * const *keys, uint16_t key_count, + const struct field_def *space_fields, + uint32_t space_field_count, struct tuple_dictionary *dict, + bool is_temporary, uint32_t exact_field_count) { struct tuple_format *format = tuple_format_alloc(keys, key_count, space_field_count, dict); if (format == NULL) return NULL; format->vtab = *vtab; - format->engine = NULL; - format->is_temporary = false; + format->engine = engine; + format->is_temporary = is_temporary; + format->exact_field_count = exact_field_count; if (tuple_format_register(format) < 0) { tuple_format_destroy(format); free(format); diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h index 30b93b6..9778fda 100644 --- a/src/box/tuple_format.h +++ b/src/box/tuple_format.h @@ -258,18 +258,23 @@ tuple_format_unref(struct tuple_format *format) /** * Allocate, construct and register a new in-memory tuple format. * @param vtab Virtual function table for specific engines. + * @param engine Pointer to storage engine. * @param keys Array of key_defs of a space. * @param key_count The number of keys in @a keys array. * @param space_fields Array of fields, defined in a space format. * @param space_field_count Length of @a space_fields. + * @param is_temporary Set if format is temporary. + * @param exact_field_count Exact field count for format. * * @retval not NULL Tuple format. * @retval NULL Memory error. */ struct tuple_format * -tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys, - uint16_t key_count, const struct field_def *space_fields, - uint32_t space_field_count, struct tuple_dictionary *dict); +tuple_format_new(struct tuple_format_vtab *vtab, void *engine, + struct key_def * const *keys, uint16_t key_count, + const struct field_def *space_fields, + uint32_t space_field_count, struct tuple_dictionary *dict, + bool is_temporary, uint32_t exact_field_count); /** * Check, if @a format1 can store any tuples of @a format2. For diff --git a/src/box/vinyl.c b/src/box/vinyl.c index d6117f4..abae076 100644 --- a/src/box/vinyl.c +++ b/src/box/vinyl.c @@ -619,13 +619,14 @@ vinyl_engine_create_space(struct engine *engine, struct space_def *def, keys[key_count++] = index_def->key_def; struct tuple_format *format = - tuple_format_new(&vy_tuple_format_vtab, keys, key_count, - def->fields, def->field_count, def->dict); + tuple_format_new(&vy_tuple_format_vtab, NULL, keys, + key_count, + def->fields, def->field_count, def->dict, + false, def->exact_field_count); if (format == NULL) { free(space); return NULL; } - format->exact_field_count = def->exact_field_count; tuple_format_ref(format); if (space_create(space, engine, &vinyl_space_vtab, @@ -3043,8 +3044,9 @@ vy_send_lsm(struct vy_join_ctx *ctx, struct vy_lsm_recovery_info *lsm_info) lsm_info->key_part_count); if (ctx->key_def == NULL) goto out; - ctx->format = tuple_format_new(&vy_tuple_format_vtab, &ctx->key_def, - 1, NULL, 0, NULL); + ctx->format = tuple_format_new(&vy_tuple_format_vtab, NULL, + &ctx->key_def, 1, NULL, 0, NULL, false, + 0); if (ctx->format == NULL) goto out_free_key_def; tuple_format_ref(ctx->format); diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c index 5eb3fd7..983a655 100644 --- a/src/box/vy_lsm.c +++ b/src/box/vy_lsm.c @@ -60,8 +60,8 @@ vy_lsm_env_create(struct vy_lsm_env *env, const char *path, vy_upsert_thresh_cb upsert_thresh_cb, void *upsert_thresh_arg) { - env->key_format = tuple_format_new(&vy_tuple_format_vtab, - NULL, 0, NULL, 0, NULL); + env->key_format = tuple_format_new(&vy_tuple_format_vtab, NULL, + NULL, 0, NULL, 0, NULL, false, 0); if (env->key_format == NULL) return -1; tuple_format_ref(env->key_format); @@ -153,8 +153,9 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env, */ lsm->disk_format = format; } else { - lsm->disk_format = tuple_format_new(&vy_tuple_format_vtab, - &cmp_def, 1, NULL, 0, NULL); + lsm->disk_format = tuple_format_new(&vy_tuple_format_vtab, NULL, + &cmp_def, 1, NULL, 0, NULL, + false, 0); if (lsm->disk_format == NULL) goto fail_format; } diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c index 7fad560..7031097 100644 --- a/test/unit/vy_iterators_helper.c +++ b/test/unit/vy_iterators_helper.c @@ -21,8 +21,8 @@ vy_iterator_C_test_init(size_t cache_size) tuple_init(NULL); vy_cache_env_create(&cache_env, cord_slab_cache()); vy_cache_env_set_quota(&cache_env, cache_size); - vy_key_format = tuple_format_new(&vy_tuple_format_vtab, NULL, 0, - NULL, 0, NULL); + vy_key_format = tuple_format_new(&vy_tuple_format_vtab, NULL, NULL, 0, + NULL, 0, NULL, false, 0); tuple_format_ref(vy_key_format); size_t mem_size = 64 * 1024 * 1024; @@ -201,8 +201,8 @@ create_test_mem(struct key_def *def) /* Create format */ struct key_def * const defs[] = { def }; struct tuple_format *format = - tuple_format_new(&vy_tuple_format_vtab, defs, def->part_count, - NULL, 0, NULL); + tuple_format_new(&vy_tuple_format_vtab, NULL, defs, + def->part_count, NULL, 0, NULL, false, 0); fail_if(format == NULL); /* Create mem */ @@ -219,8 +219,8 @@ create_test_cache(uint32_t *fields, uint32_t *types, *def = box_key_def_new(fields, types, key_cnt); assert(*def != NULL); vy_cache_create(cache, &cache_env, *def, true); - *format = tuple_format_new(&vy_tuple_format_vtab, def, 1, NULL, 0, - NULL); + *format = tuple_format_new(&vy_tuple_format_vtab, NULL, def, 1, NULL, 0, + NULL, false, 0); tuple_format_ref(*format); } diff --git a/test/unit/vy_mem.c b/test/unit/vy_mem.c index ebf3fbc..911f082 100644 --- a/test/unit/vy_mem.c +++ b/test/unit/vy_mem.c @@ -77,8 +77,8 @@ test_iterator_restore_after_insertion() /* Create format */ struct tuple_format *format = tuple_format_new(&vy_tuple_format_vtab, - &key_def, 1, NULL, 0, - NULL); + NULL, &key_def, 1, NULL, + 0, NULL, false, 0); assert(format != NULL); tuple_format_ref(format); diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c index 65dafcb..634fffa 100644 --- a/test/unit/vy_point_lookup.c +++ b/test/unit/vy_point_lookup.c @@ -84,8 +84,8 @@ test_basic() vy_cache_create(&cache, &cache_env, key_def, true); struct tuple_format *format = tuple_format_new(&vy_tuple_format_vtab, - &key_def, 1, NULL, 0, - NULL); + NULL, &key_def, 1, NULL, + 0, NULL, false, 0); isnt(format, NULL, "tuple_format_new is not NULL"); tuple_format_ref(format); -- 2.19.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] Pass necessary fields to tuple_format contructor 2019-01-22 14:07 ` [PATCH 1/4] Pass necessary fields to tuple_format contructor Kirill Yukhin @ 2019-01-23 7:53 ` Vladimir Davydov 2019-01-24 8:13 ` Kirill Yukhin 0 siblings, 1 reply; 13+ messages in thread From: Vladimir Davydov @ 2019-01-23 7:53 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches On Tue, Jan 22, 2019 at 05:07:17PM +0300, Kirill Yukhin wrote: > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index e11b4e6..6edc0fb 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c > @@ -398,17 +398,20 @@ tuple_format_delete(struct tuple_format *format) > } > > struct tuple_format * > -tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys, > - uint16_t key_count, const struct field_def *space_fields, > - uint32_t space_field_count, struct tuple_dictionary *dict) > +tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > + struct key_def * const *keys, uint16_t key_count, > + const struct field_def *space_fields, > + uint32_t space_field_count, struct tuple_dictionary *dict, > + bool is_temporary, uint32_t exact_field_count) Nit: I think we should rearrange the arguments a little: vtab, engine, keys, key_count, fields, field_count, exact_field_count, dict, is_temporary, is_ephemeral > diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h > index 30b93b6..9778fda 100644 > --- a/src/box/tuple_format.h > +++ b/src/box/tuple_format.h > @@ -258,18 +258,23 @@ tuple_format_unref(struct tuple_format *format) > /** > * Allocate, construct and register a new in-memory tuple format. > * @param vtab Virtual function table for specific engines. > + * @param engine Pointer to storage engine. > * @param keys Array of key_defs of a space. > * @param key_count The number of keys in @a keys array. > * @param space_fields Array of fields, defined in a space format. > * @param space_field_count Length of @a space_fields. > + * @param is_temporary Set if format is temporary. Nit: The format isn't temporary. The space that this format is created for is. > diff --git a/src/box/vinyl.c b/src/box/vinyl.c > index d6117f4..abae076 100644 > --- a/src/box/vinyl.c > +++ b/src/box/vinyl.c > @@ -619,13 +619,14 @@ vinyl_engine_create_space(struct engine *engine, struct space_def *def, > keys[key_count++] = index_def->key_def; > > struct tuple_format *format = > - tuple_format_new(&vy_tuple_format_vtab, keys, key_count, > - def->fields, def->field_count, def->dict); > + tuple_format_new(&vy_tuple_format_vtab, NULL, keys, > + key_count, Nit: This short line surrounded by long ones doesn't look good. > + def->fields, def->field_count, def->dict, > + false, def->exact_field_count); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] Pass necessary fields to tuple_format contructor 2019-01-23 7:53 ` Vladimir Davydov @ 2019-01-24 8:13 ` Kirill Yukhin 0 siblings, 0 replies; 13+ messages in thread From: Kirill Yukhin @ 2019-01-24 8:13 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tarantool-patches Hello Vladimir, Thanks for review. My answers are inlined. I'll send updated patch as v2. On 23 Jan 10:53, Vladimir Davydov wrote: > On Tue, Jan 22, 2019 at 05:07:17PM +0300, Kirill Yukhin wrote: > > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > > index e11b4e6..6edc0fb 100644 > > --- a/src/box/tuple_format.c > > +++ b/src/box/tuple_format.c > > @@ -398,17 +398,20 @@ tuple_format_delete(struct tuple_format *format) > > } > > > > struct tuple_format * > > -tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys, > > - uint16_t key_count, const struct field_def *space_fields, > > - uint32_t space_field_count, struct tuple_dictionary *dict) > > +tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > > + struct key_def * const *keys, uint16_t key_count, > > + const struct field_def *space_fields, > > + uint32_t space_field_count, struct tuple_dictionary *dict, > > + bool is_temporary, uint32_t exact_field_count) > > Nit: I think we should rearrange the arguments a little: > > vtab, engine, keys, key_count, > fields, field_count, exact_field_count, > dict, is_temporary, is_ephemeral Done. > > diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h > > index 30b93b6..9778fda 100644 > > --- a/src/box/tuple_format.h > > +++ b/src/box/tuple_format.h > > @@ -258,18 +258,23 @@ tuple_format_unref(struct tuple_format *format) > > /** > > * Allocate, construct and register a new in-memory tuple format. > > * @param vtab Virtual function table for specific engines. > > + * @param engine Pointer to storage engine. > > * @param keys Array of key_defs of a space. > > * @param key_count The number of keys in @a keys array. > > * @param space_fields Array of fields, defined in a space format. > > * @param space_field_count Length of @a space_fields. > > + * @param is_temporary Set if format is temporary. > > Nit: The format isn't temporary. The space that this format > is created for is. Re-worded. > > diff --git a/src/box/vinyl.c b/src/box/vinyl.c > > index d6117f4..abae076 100644 > > --- a/src/box/vinyl.c > > +++ b/src/box/vinyl.c > > @@ -619,13 +619,14 @@ vinyl_engine_create_space(struct engine *engine, struct space_def *def, > > keys[key_count++] = index_def->key_def; > > > > struct tuple_format *format = > > - tuple_format_new(&vy_tuple_format_vtab, keys, key_count, > > - def->fields, def->field_count, def->dict); > > + tuple_format_new(&vy_tuple_format_vtab, NULL, keys, > > + key_count, > > Nit: This short line surrounded by long ones doesn't look good. Fixed. > > + def->fields, def->field_count, def->dict, > > + false, def->exact_field_count); -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] Set is_temporary flag for formats of ephemeral spaces 2019-01-22 14:07 ` Kirill Yukhin 2019-01-22 14:07 ` [PATCH 1/4] Pass necessary fields to tuple_format contructor Kirill Yukhin @ 2019-01-22 14:07 ` Kirill Yukhin 2019-01-23 8:00 ` Vladimir Davydov 2019-01-22 14:07 ` [PATCH 3/4] sql: set error type in case of ephemral space creation failure Kirill Yukhin 2019-01-22 14:07 ` [PATCH 4/4] Allow to reuse tuple_formats for ephemeral spaces Kirill Yukhin 3 siblings, 1 reply; 13+ messages in thread From: Kirill Yukhin @ 2019-01-22 14:07 UTC (permalink / raw) To: vdavydov.dev; +Cc: tarantool-patches, Kirill Yukhin Before the patch, when ephemeral space was created flag is_temporary was set after space was actually created. Which in turn lead to corresponding flag of tuple_format being set to `false`. So, having heavy load using ephemeral spaces (almost any SQL query) and snapshotting at the same time might lead to OOM, since tuples of ephemeral spaces were not marked as temporary and were not gc-ed. Patch sets the flag in space definition. --- src/box/space_def.c | 13 +++++++++++++ src/box/space_def.h | 9 +++++++++ src/box/sql.c | 6 +----- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/box/space_def.c b/src/box/space_def.c index 3516bdd..1d2bfcb 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -257,6 +257,19 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, return def; } +struct space_def* +space_def_new_ephemeral(uint32_t field_count) +{ + struct space_def *space_def = space_def_new(0, 0 , field_count, + "ephemeral", + strlen("ephemeral"), + "memtx", strlen("memtx"), + &space_opts_default, + &field_def_default, 0); + space_def->opts.is_temporary = true; + return space_def; +} + /** Free a default value's syntax trees of @a defs. */ void space_def_destroy_fields(struct field_def *fields, uint32_t field_count, diff --git a/src/box/space_def.h b/src/box/space_def.h index 8044f88..e8d46c5 100644 --- a/src/box/space_def.h +++ b/src/box/space_def.h @@ -169,6 +169,15 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, const struct space_opts *opts, const struct field_def *fields, uint32_t field_count); +/** + * Create a new ephemeral space. + * @param field_count Number of fields in the space. + * + * @retval Space definition. + */ +struct space_def * +space_def_new_ephemeral(uint32_t field_count); + /** * Size of the space_def. * @param name_len Length of the space name. diff --git a/src/box/sql.c b/src/box/sql.c index 081a038..387da7b 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -408,11 +408,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) rlist_add_entry(&key_list, ephemer_index_def, link); struct space_def *ephemer_space_def = - space_def_new(0 /* space id */, 0 /* user id */, field_count, - "ephemeral", strlen("ephemeral"), - "memtx", strlen("memtx"), - &space_opts_default, &field_def_default, - 0 /* length of field_def */); + space_def_new_ephemeral(field_count); if (ephemer_space_def == NULL) { index_def_delete(ephemer_index_def); return NULL; -- 2.19.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] Set is_temporary flag for formats of ephemeral spaces 2019-01-22 14:07 ` [PATCH 2/4] Set is_temporary flag for formats of ephemeral spaces Kirill Yukhin @ 2019-01-23 8:00 ` Vladimir Davydov 2019-01-24 8:53 ` Kirill Yukhin 0 siblings, 1 reply; 13+ messages in thread From: Vladimir Davydov @ 2019-01-23 8:00 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches On Tue, Jan 22, 2019 at 05:07:18PM +0300, Kirill Yukhin wrote: > diff --git a/src/box/space_def.c b/src/box/space_def.c > index 3516bdd..1d2bfcb 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > @@ -257,6 +257,19 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > return def; > } > > +struct space_def* > +space_def_new_ephemeral(uint32_t field_count) > +{ > + struct space_def *space_def = space_def_new(0, 0 , field_count, > + "ephemeral", > + strlen("ephemeral"), > + "memtx", strlen("memtx"), > + &space_opts_default, > + &field_def_default, 0); > + space_def->opts.is_temporary = true; Nit: I'd rather you didn't override is_temporary of a newly created space: struct space_opts opts = space_opts_default; opts.is_temporary = true; space_def_new(opts); > + return space_def; > +} > + > /** Free a default value's syntax trees of @a defs. */ > void > space_def_destroy_fields(struct field_def *fields, uint32_t field_count, > diff --git a/src/box/space_def.h b/src/box/space_def.h > index 8044f88..e8d46c5 100644 > --- a/src/box/space_def.h > +++ b/src/box/space_def.h > @@ -169,6 +169,15 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > const struct space_opts *opts, const struct field_def *fields, > uint32_t field_count); > > +/** > + * Create a new ephemeral space. Nit: space definition, not space. > + * @param field_count Number of fields in the space. > + * > + * @retval Space definition. > + */ > +struct space_def * > +space_def_new_ephemeral(uint32_t field_count); > + > /** > * Size of the space_def. > * @param name_len Length of the space name. > diff --git a/src/box/sql.c b/src/box/sql.c > index 081a038..387da7b 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -408,11 +408,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > rlist_add_entry(&key_list, ephemer_index_def, link); > > struct space_def *ephemer_space_def = > - space_def_new(0 /* space id */, 0 /* user id */, field_count, > - "ephemeral", strlen("ephemeral"), > - "memtx", strlen("memtx"), > - &space_opts_default, &field_def_default, > - 0 /* length of field_def */); > + space_def_new_ephemeral(field_count); > if (ephemer_space_def == NULL) { > index_def_delete(ephemer_index_def); > return NULL; This patch should also replace space->def->opts.is_temporary = true; with the corresponding assertion in space_new_ephemeral(). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] Set is_temporary flag for formats of ephemeral spaces 2019-01-23 8:00 ` Vladimir Davydov @ 2019-01-24 8:53 ` Kirill Yukhin 0 siblings, 0 replies; 13+ messages in thread From: Kirill Yukhin @ 2019-01-24 8:53 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tarantool-patches Hello Vladimir, Thanks for review. My answers are inlined. On 23 Jan 11:00, Vladimir Davydov wrote: > On Tue, Jan 22, 2019 at 05:07:18PM +0300, Kirill Yukhin wrote: > > diff --git a/src/box/space_def.c b/src/box/space_def.c > > index 3516bdd..1d2bfcb 100644 > > --- a/src/box/space_def.c > > +++ b/src/box/space_def.c > > @@ -257,6 +257,19 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > > return def; > > } > > > > +struct space_def* > > +space_def_new_ephemeral(uint32_t field_count) > > +{ > > + struct space_def *space_def = space_def_new(0, 0 , field_count, > > + "ephemeral", > > + strlen("ephemeral"), > > + "memtx", strlen("memtx"), > > + &space_opts_default, > > + &field_def_default, 0); > > + space_def->opts.is_temporary = true; > > Nit: I'd rather you didn't override is_temporary of a newly created > space: > > struct space_opts opts = space_opts_default; > opts.is_temporary = true; > space_def_new(opts); > > > + return space_def; > > +} Fixed. > > /** Free a default value's syntax trees of @a defs. */ > > void > > space_def_destroy_fields(struct field_def *fields, uint32_t field_count, > > diff --git a/src/box/space_def.h b/src/box/space_def.h > > index 8044f88..e8d46c5 100644 > > --- a/src/box/space_def.h > > +++ b/src/box/space_def.h > > @@ -169,6 +169,15 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > > const struct space_opts *opts, const struct field_def *fields, > > uint32_t field_count); > > > > +/** > > + * Create a new ephemeral space. > > Nit: space definition, not space. Added. > > diff --git a/src/box/sql.c b/src/box/sql.c > > index 081a038..387da7b 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -408,11 +408,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > > rlist_add_entry(&key_list, ephemer_index_def, link); > > > > struct space_def *ephemer_space_def = > > - space_def_new(0 /* space id */, 0 /* user id */, field_count, > > - "ephemeral", strlen("ephemeral"), > > - "memtx", strlen("memtx"), > > - &space_opts_default, &field_def_default, > > - 0 /* length of field_def */); > > + space_def_new_ephemeral(field_count); > > if (ephemer_space_def == NULL) { > > index_def_delete(ephemer_index_def); > > return NULL; > > This patch should also replace > > space->def->opts.is_temporary = true; > > with the corresponding assertion in space_new_ephemeral(). Done. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] sql: set error type in case of ephemral space creation failure 2019-01-22 14:07 ` Kirill Yukhin 2019-01-22 14:07 ` [PATCH 1/4] Pass necessary fields to tuple_format contructor Kirill Yukhin 2019-01-22 14:07 ` [PATCH 2/4] Set is_temporary flag for formats of ephemeral spaces Kirill Yukhin @ 2019-01-22 14:07 ` Kirill Yukhin 2019-01-23 8:00 ` Vladimir Davydov 2019-01-22 14:07 ` [PATCH 4/4] Allow to reuse tuple_formats for ephemeral spaces Kirill Yukhin 3 siblings, 1 reply; 13+ messages in thread From: Kirill Yukhin @ 2019-01-22 14:07 UTC (permalink / raw) To: vdavydov.dev; +Cc: tarantool-patches, Kirill Yukhin This is trivial patch which sets error kind if epehemeral spaces cannot be created due to Tarantool's backend (e.g. there's no more memory or formats available). --- src/box/sql/vdbe.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9f9d0ea..9fc362f 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -3272,8 +3272,10 @@ case OP_OpenTEphemeral: { struct space *space = sql_ephemeral_space_create(pOp->p2, pOp->p4.key_info); - if (space == NULL) + if (space == NULL) { + rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; + } aMem[pOp->p1].u.p = space; aMem[pOp->p1].flags = MEM_Ptr; break; -- 2.19.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] sql: set error type in case of ephemral space creation failure 2019-01-22 14:07 ` [PATCH 3/4] sql: set error type in case of ephemral space creation failure Kirill Yukhin @ 2019-01-23 8:00 ` Vladimir Davydov 0 siblings, 0 replies; 13+ messages in thread From: Vladimir Davydov @ 2019-01-23 8:00 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches On Tue, Jan 22, 2019 at 05:07:19PM +0300, Kirill Yukhin wrote: > This is trivial patch which sets error kind if epehemeral > spaces cannot be created due to Tarantool's backend (e.g. there's > no more memory or formats available). > --- > src/box/sql/vdbe.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 9f9d0ea..9fc362f 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3272,8 +3272,10 @@ case OP_OpenTEphemeral: { > struct space *space = sql_ephemeral_space_create(pOp->p2, > pOp->p4.key_info); > > - if (space == NULL) > + if (space == NULL) { > + rc = SQL_TARANTOOL_ERROR; > goto abort_due_to_error; > + } > aMem[pOp->p1].u.p = space; > aMem[pOp->p1].flags = MEM_Ptr; > break; This one is apparently OK to push. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] Allow to reuse tuple_formats for ephemeral spaces 2019-01-22 14:07 ` Kirill Yukhin ` (2 preceding siblings ...) 2019-01-22 14:07 ` [PATCH 3/4] sql: set error type in case of ephemral space creation failure Kirill Yukhin @ 2019-01-22 14:07 ` Kirill Yukhin 2019-01-23 8:38 ` Vladimir Davydov 3 siblings, 1 reply; 13+ messages in thread From: Kirill Yukhin @ 2019-01-22 14:07 UTC (permalink / raw) To: vdavydov.dev; +Cc: tarantool-patches, Kirill Yukhin Since under heavy load with SQL queries ephemeral spaces might be extensively used it is possible to run out of tuple_formats for such spaces. This occurs because tuple_format is not immediately deleted when ephemeral space is dropped. Its removel is postponed instead and triggered only when tuple memory is exhausted. As far as there's no way to alter ephemeral space's format, let's re-use them for multiple epehemral spaces in case they're identical. Closes #3924 --- src/box/blackhole.c | 2 +- src/box/box.cc | 4 + src/box/memtx_engine.c | 6 ++ src/box/memtx_space.c | 3 +- src/box/space.c | 3 +- src/box/space_def.c | 3 + src/box/space_def.h | 5 ++ src/box/tuple.c | 6 +- src/box/tuple_format.c | 130 ++++++++++++++++++++++++++++++-- src/box/tuple_format.h | 23 +++++- src/box/vinyl.c | 4 +- src/box/vy_lsm.c | 5 +- src/errinj.h | 2 + test/box/errinj.result | 8 +- test/sql/errinj.result | 39 ++++++++++ test/sql/errinj.test.lua | 14 ++++ test/unit/tuple_bigref.c | 1 + test/unit/vy_iterators_helper.c | 8 +- test/unit/vy_mem.c | 3 +- test/unit/vy_point_lookup.c | 3 +- 20 files changed, 246 insertions(+), 26 deletions(-) diff --git a/src/box/blackhole.c b/src/box/blackhole.c index 64e24cd..16958a2 100644 --- a/src/box/blackhole.c +++ b/src/box/blackhole.c @@ -157,7 +157,7 @@ blackhole_engine_create_space(struct engine *engine, struct space_def *def, struct tuple_format *format; format = tuple_format_new(&tuple_format_runtime->vtab, NULL, NULL, 0, def->fields, def->field_count, def->dict, - false, def->exact_field_count); + false, false, def->exact_field_count); if (format == NULL) { free(space); return NULL; diff --git a/src/box/box.cc b/src/box/box.cc index 9f2fd6d..3799544 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -46,6 +46,7 @@ #include <rmean.h> #include "main.h" #include "tuple.h" +#include "tuple_format.h" #include "session.h" #include "schema.h" #include "engine.h" @@ -2050,6 +2051,9 @@ box_init(void) */ session_init(); + if (tuple_format_init() != 0) + diag_raise(); + if (tuple_init(lua_hash) != 0) diag_raise(); diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c index 5cf70ab..254ef24 100644 --- a/src/box/memtx_engine.c +++ b/src/box/memtx_engine.c @@ -994,6 +994,12 @@ memtx_engine_gc_f(va_list va) struct memtx_engine *memtx = va_arg(va, struct memtx_engine *); while (!fiber_is_cancelled()) { bool stop; + struct errinj *delay = errinj(ERRINJ_MEMTX_DELAY_GC, + ERRINJ_BOOL); + if (delay != NULL && delay->bparam) { + while (delay->bparam) + fiber_sleep(0.001); + } memtx_engine_run_gc(memtx, &stop); if (stop) { fiber_yield_timeout(TIMEOUT_INFINITY); diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index 49d09af..e6909f9 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -992,7 +992,8 @@ memtx_space_new(struct memtx_engine *memtx, struct tuple_format *format = tuple_format_new(&memtx_tuple_format_vtab, memtx, keys, key_count, def->fields, def->field_count, def->dict, - def->opts.is_temporary, def->exact_field_count); + def->opts.is_temporary, def->opts.is_ephemeral, + def->exact_field_count); if (format == NULL) { free(memtx_space); return NULL; diff --git a/src/box/space.c b/src/box/space.c index 4d174f7..316b34b 100644 --- a/src/box/space.c +++ b/src/box/space.c @@ -194,10 +194,11 @@ space_new(struct space_def *def, struct rlist *key_list) struct space * space_new_ephemeral(struct space_def *def, struct rlist *key_list) { + assert(def->opts.is_temporary); + assert(def->opts.is_ephemeral); struct space *space = space_new(def, key_list); if (space == NULL) return NULL; - space->def->opts.is_temporary = true; space->vtab->init_ephemeral_space(space); return space; } diff --git a/src/box/space_def.c b/src/box/space_def.c index 1d2bfcb..c37df24 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -53,6 +53,7 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode, const struct space_opts space_opts_default = { /* .group_id = */ 0, /* .is_temporary = */ false, + /* .is_ephemeral = */ false, /* .view = */ false, /* .sql = */ NULL, /* .checks = */ NULL, @@ -61,6 +62,7 @@ const struct space_opts space_opts_default = { const struct opt_def space_opts_reg[] = { OPT_DEF("group_id", OPT_UINT32, struct space_opts, group_id), OPT_DEF("temporary", OPT_BOOL, struct space_opts, is_temporary), + OPT_DEF("ephemeral", OPT_BOOL, struct space_opts, is_ephemeral), OPT_DEF("view", OPT_BOOL, struct space_opts, is_view), OPT_DEF("sql", OPT_STRPTR, struct space_opts, sql), OPT_DEF_ARRAY("checks", struct space_opts, checks, @@ -267,6 +269,7 @@ space_def_new_ephemeral(uint32_t field_count) &space_opts_default, &field_def_default, 0); space_def->opts.is_temporary = true; + space_def->opts.is_ephemeral = true; return space_def; } diff --git a/src/box/space_def.h b/src/box/space_def.h index e8d46c5..ea20b1d 100644 --- a/src/box/space_def.h +++ b/src/box/space_def.h @@ -58,6 +58,11 @@ struct space_opts { * does not require manual release. */ bool is_temporary; + /** + * This flag is set if space is ephemeral and hence + * its format might be re-used. + */ + bool is_ephemeral; /** * If the space is a view, then it can't feature any * indexes, and must have SQL statement. Moreover, diff --git a/src/box/tuple.c b/src/box/tuple.c index 5295800..765e40b 100644 --- a/src/box/tuple.c +++ b/src/box/tuple.c @@ -208,7 +208,8 @@ tuple_init(field_name_hash_f hash) * Create a format for runtime tuples */ tuple_format_runtime = tuple_format_new(&tuple_format_runtime_vtab, NULL, - NULL, 0, NULL, 0, NULL, false, 0); + NULL, 0, NULL, 0, NULL, false, + false, 0); if (tuple_format_runtime == NULL) return -1; @@ -380,7 +381,8 @@ box_tuple_format_new(struct key_def **keys, uint16_t key_count) { box_tuple_format_t *format = tuple_format_new(&tuple_format_runtime_vtab, NULL, - keys, key_count, NULL, 0, NULL, false, 0); + keys, key_count, NULL, 0, NULL, false, false, + 0); if (format != NULL) tuple_format_ref(format); return format; diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 6edc0fb..5d54e94 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -34,6 +34,8 @@ #include "tuple_format.h" #include "coll_id_cache.h" +#include "third_party/PMurHash.h" + /** Global table of tuple formats */ struct tuple_format **tuple_formats; static intptr_t recycled_format_ids = FORMAT_ID_NIL; @@ -276,7 +278,11 @@ tuple_format_register(struct tuple_format *format) formats_capacity = new_capacity; tuple_formats = formats; } - if (formats_size == FORMAT_ID_MAX + 1) { + struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, + ERRINJ_INT); + if ((inj != NULL && inj->iparam > 0 + && formats_size >= inj->iparam + 1) + || formats_size == FORMAT_ID_MAX + 1) { diag_set(ClientError, ER_TUPLE_FORMAT_LIMIT, (unsigned) formats_capacity); return -1; @@ -380,10 +386,77 @@ error: return NULL; } +static int +tuple_format_cmp(const struct tuple_format *a, const struct tuple_format *b) +{ + if (a->exact_field_count != b->exact_field_count) + return a->exact_field_count - b->exact_field_count; + if (tuple_format_field_count(a) != tuple_format_field_count(b)) + return tuple_format_field_count(a) - tuple_format_field_count(b); + + for (uint32_t i = 0; i < tuple_format_field_count(a); ++i) { + struct tuple_field *field_a = tuple_format_field( + (struct tuple_format *)a, i); + struct tuple_field *field_b = tuple_format_field( + (struct tuple_format *)b, i); + if (field_a->type != field_b->type) + return (int)field_a->type - (int)field_b->type; + if (field_a->coll_id != field_b->coll_id) + return (int)field_a->coll_id - (int)field_b->coll_id; + if (field_a->nullable_action != field_b->nullable_action) + return (int)field_a->nullable_action - + (int)field_b->nullable_action; + if (field_a->is_key_part != field_b->is_key_part) + return (int)field_a->is_key_part - + (int)field_b->is_key_part; + } + + return 0; +} + +static uint32_t +tuple_format_hash(struct tuple_format *format) +{ + uint32_t h = 0; + uint32_t carry = 0; + uint32_t size = 0; + for (uint32_t i = 0; i < tuple_format_field_count(format); ++i) { + struct tuple_field *f = tuple_format_field(format, i); + PMurHash32_Process(&h, &carry, &f->type, + sizeof(enum field_type)); + size += sizeof(enum field_type); + PMurHash32_Process(&h, &carry, &f->coll_id, + sizeof(uint32_t)); + size += sizeof(uint32_t); + PMurHash32_Process(&h, &carry, &f->nullable_action, + sizeof(enum on_conflict_action)); + size += sizeof(enum on_conflict_action); + PMurHash32_Process(&h, &carry, &f->is_key_part, sizeof(bool)); + size += sizeof(bool); + } + return PMurHash32_Result(h, carry, size); +} + +#define MH_SOURCE 1 +#define mh_name _tuple_format +#define mh_key_t struct tuple_format * +#define mh_node_t struct tuple_format * +#define mh_arg_t void * +#define mh_hash(a, arg) ((*(a))->hash) +#define mh_hash_key(a, arg) ((a)->hash) +#define mh_cmp(a, b, arg) (tuple_format_cmp(*(a), *(b))) +#define mh_cmp_key(a, b, arg) (tuple_format_cmp((a), *(b))) +#include "salad/mhash.h" + +struct mh_tuple_format_t *tuple_formats_hash = NULL; + /** Free tuple format resources, doesn't unregister. */ static inline void tuple_format_destroy(struct tuple_format *format) { + mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); + if (key != mh_end(tuple_formats_hash)) + mh_tuple_format_del(tuple_formats_hash, key, NULL); free(format->required_fields); tuple_format_destroy_fields(format); tuple_dictionary_unref(format->dict); @@ -402,7 +475,8 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, struct key_def * const *keys, uint16_t key_count, const struct field_def *space_fields, uint32_t space_field_count, struct tuple_dictionary *dict, - bool is_temporary, uint32_t exact_field_count) + bool is_temporary, bool is_ephemeral, + uint32_t exact_field_count) { struct tuple_format *format = tuple_format_alloc(keys, key_count, space_field_count, dict); @@ -411,18 +485,46 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, format->vtab = *vtab; format->engine = engine; format->is_temporary = is_temporary; + format->is_ephemeral = is_ephemeral; format->exact_field_count = exact_field_count; - if (tuple_format_register(format) < 0) { - tuple_format_destroy(format); - free(format); - return NULL; - } if (tuple_format_create(format, keys, key_count, space_fields, space_field_count) < 0) { tuple_format_delete(format); return NULL; } - return format; + format->hash = tuple_format_hash(format); + struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, ERRINJ_INT); + if (inj != NULL && inj->iparam == 200) { + inj = NULL; + } + if (format->is_ephemeral) { + mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, + NULL); + if (key != mh_end(tuple_formats_hash)) { + struct tuple_format **entry = mh_tuple_format_node(tuple_formats_hash, key); + tuple_format_destroy(format); + free(format); + return *entry; + } else { + if (tuple_format_register(format) < 0) { + tuple_format_destroy(format); + free(format); + return NULL; + } + mh_tuple_format_put(tuple_formats_hash, + (const + struct tuple_format **)&format, + NULL, NULL); + return format; + } + } else { + if (tuple_format_register(format) < 0) { + tuple_format_destroy(format); + free(format); + return NULL; + } + return format; + } } bool @@ -823,3 +925,15 @@ error: tt_sprintf("error in path on position %d", rc)); return -1; } + +int +tuple_format_init() +{ + tuple_formats_hash = mh_tuple_format_new(); + if (tuple_formats_hash == NULL) { + diag_set(OutOfMemory, sizeof(struct mh_tuple_format_t), "malloc", + "tuple format hash"); + return -1; + } + return 0; +} diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h index 9778fda..d347eb7 100644 --- a/src/box/tuple_format.h +++ b/src/box/tuple_format.h @@ -151,6 +151,11 @@ struct tuple_format { * in progress. */ bool is_temporary; + /** + * This format belongs to ephemeral space and thus might + * be shared with other ephemeral spaces. + */ + bool is_ephemeral; /** * Size of field map of tuple in bytes. * \sa struct tuple @@ -193,6 +198,11 @@ struct tuple_format { * tuple_field::token. */ struct json_tree fields; + + /** + * Hash key for shared formats of epeheral spaces. + */ + uint32_t hash; }; /** @@ -200,7 +210,7 @@ struct tuple_format { * a given format. */ static inline uint32_t -tuple_format_field_count(struct tuple_format *format) +tuple_format_field_count(const struct tuple_format *format) { const struct json_token *root = &format->fields.root; return root->children != NULL ? root->max_child_idx + 1 : 0; @@ -263,6 +273,7 @@ tuple_format_unref(struct tuple_format *format) * @param key_count The number of keys in @a keys array. * @param space_fields Array of fields, defined in a space format. * @param space_field_count Length of @a space_fields. + * @param is_ephemeral Set if format belongs to ephemeral space. * @param is_temporary Set if format is temporary. * @param exact_field_count Exact field count for format. * @@ -274,7 +285,8 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, struct key_def * const *keys, uint16_t key_count, const struct field_def *space_fields, uint32_t space_field_count, struct tuple_dictionary *dict, - bool is_temporary, uint32_t exact_field_count); + bool is_temporary, bool is_ephemeral, + uint32_t exact_field_count); /** * Check, if @a format1 can store any tuples of @a format2. For @@ -459,6 +471,13 @@ tuple_field_by_part_raw(struct tuple_format *format, const char *data, return tuple_field_raw(format, data, field_map, part->fieldno); } +/** + * Initialize tuple formats. + * @retval 0 on success, -1 otherwise. + */ +int +tuple_format_init(); + #if defined(__cplusplus) } /* extern "C" */ #endif /* defined(__cplusplus) */ diff --git a/src/box/vinyl.c b/src/box/vinyl.c index abae076..dab72a6 100644 --- a/src/box/vinyl.c +++ b/src/box/vinyl.c @@ -622,7 +622,7 @@ vinyl_engine_create_space(struct engine *engine, struct space_def *def, tuple_format_new(&vy_tuple_format_vtab, NULL, keys, key_count, def->fields, def->field_count, def->dict, - false, def->exact_field_count); + false, false, def->exact_field_count); if (format == NULL) { free(space); return NULL; @@ -3046,7 +3046,7 @@ vy_send_lsm(struct vy_join_ctx *ctx, struct vy_lsm_recovery_info *lsm_info) goto out; ctx->format = tuple_format_new(&vy_tuple_format_vtab, NULL, &ctx->key_def, 1, NULL, 0, NULL, false, - 0); + false, 0); if (ctx->format == NULL) goto out_free_key_def; tuple_format_ref(ctx->format); diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c index 983a655..a9fb9a0 100644 --- a/src/box/vy_lsm.c +++ b/src/box/vy_lsm.c @@ -61,7 +61,8 @@ vy_lsm_env_create(struct vy_lsm_env *env, const char *path, void *upsert_thresh_arg) { env->key_format = tuple_format_new(&vy_tuple_format_vtab, NULL, - NULL, 0, NULL, 0, NULL, false, 0); + NULL, 0, NULL, 0, NULL, false, false, + 0); if (env->key_format == NULL) return -1; tuple_format_ref(env->key_format); @@ -155,7 +156,7 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env, } else { lsm->disk_format = tuple_format_new(&vy_tuple_format_vtab, NULL, &cmp_def, 1, NULL, 0, NULL, - false, 0); + false, false, 0); if (lsm->disk_format == NULL) goto fail_format; } diff --git a/src/errinj.h b/src/errinj.h index 39de63d..31e4dfb 100644 --- a/src/errinj.h +++ b/src/errinj.h @@ -123,6 +123,8 @@ struct errinj { _(ERRINJ_RELAY_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \ _(ERRINJ_WAL_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \ _(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \ + _(ERRINJ_TUPLE_FORMAT_COUNT, ERRINJ_INT, {.iparam = -1}) \ + _(ERRINJ_MEMTX_DELAY_GC, ERRINJ_BOOL, {.bparam = false}) \ ENUM0(errinj_id, ERRINJ_LIST); extern struct errinj errinjs[]; diff --git a/test/box/errinj.result b/test/box/errinj.result index 1230367..ccb9aff 100644 --- a/test/box/errinj.result +++ b/test/box/errinj.result @@ -46,8 +46,12 @@ errinj.info() state: false ERRINJ_WAL_FALLOCATE: state: 0 + ERRINJ_RELAY_EXIT_DELAY: + state: 0 ERRINJ_SNAP_COMMIT_DELAY: state: false + ERRINJ_TUPLE_FORMAT_COUNT: + state: -1 ERRINJ_TUPLE_ALLOC: state: false ERRINJ_VY_RUN_WRITE_DELAY: @@ -92,8 +96,8 @@ errinj.info() state: false ERRINJ_VY_POINT_ITER_WAIT: state: false - ERRINJ_RELAY_EXIT_DELAY: - state: 0 + ERRINJ_MEMTX_DELAY_GC: + state: false ERRINJ_IPROTO_TX_DELAY: state: false ERRINJ_BUILD_INDEX: diff --git a/test/sql/errinj.result b/test/sql/errinj.result index cb993f8..9a6cf6e 100644 --- a/test/sql/errinj.result +++ b/test/sql/errinj.result @@ -16,6 +16,45 @@ errinj = box.error.injection fiber = require('fiber') --- ... +-- gh-3924 Check that tuple_formats of ephemeral spaces are +-- reused. +format = {} +--- +... +box.sql.execute("CREATE TABLE t4 (id INTEGER PRIMARY KEY, a INTEGER);") +--- +... +box.sql.execute("INSERT INTO t4 VALUES (1,1)") +--- +... +box.sql.execute("INSERT INTO t4 VALUES (2,1)") +--- +... +box.sql.execute("INSERT INTO t4 VALUES (3,2)") +--- +... +errinj.set('ERRINJ_TUPLE_FORMAT_COUNT', 200) +--- +- ok +... +errinj.set('ERRINJ_MEMTX_DELAY_GC', true) +--- +- ok +... +for i = 1, 201 do box.sql.execute("SELECT DISTINCT a FROM t4") end +--- +... +errinj.set('ERRINJ_MEMTX_DELAY_GC', false) +--- +- ok +... +errinj.set('ERRINJ_TUPLE_FORMAT_COUNT', -1) +--- +- ok +... +box.sql.execute('DROP TABLE t4') +--- +... box.sql.execute('create table test (id int primary key, a float, b text)') --- ... diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua index fa7f9f2..0b50864 100644 --- a/test/sql/errinj.test.lua +++ b/test/sql/errinj.test.lua @@ -5,6 +5,20 @@ box.sql.execute('pragma sql_default_engine=\''..engine..'\'') errinj = box.error.injection fiber = require('fiber') +-- gh-3924 Check that tuple_formats of ephemeral spaces are +-- reused. +format = {} +box.sql.execute("CREATE TABLE t4 (id INTEGER PRIMARY KEY, a INTEGER);") +box.sql.execute("INSERT INTO t4 VALUES (1,1)") +box.sql.execute("INSERT INTO t4 VALUES (2,1)") +box.sql.execute("INSERT INTO t4 VALUES (3,2)") +errinj.set('ERRINJ_TUPLE_FORMAT_COUNT', 200) +errinj.set('ERRINJ_MEMTX_DELAY_GC', true) +for i = 1, 201 do box.sql.execute("SELECT DISTINCT a FROM t4") end +errinj.set('ERRINJ_MEMTX_DELAY_GC', false) +errinj.set('ERRINJ_TUPLE_FORMAT_COUNT', -1) +box.sql.execute('DROP TABLE t4') + box.sql.execute('create table test (id int primary key, a float, b text)') box.schema.user.grant('guest','read,write,execute', 'universe') cn = remote.connect(box.cfg.listen) diff --git a/test/unit/tuple_bigref.c b/test/unit/tuple_bigref.c index 20eab61..76a2e98 100644 --- a/test/unit/tuple_bigref.c +++ b/test/unit/tuple_bigref.c @@ -143,6 +143,7 @@ main() memory_init(); fiber_init(fiber_c_invoke); + tuple_format_init(); tuple_init(NULL); tuple_end = mp_encode_array(tuple_end, 1); diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c index 7031097..aa613d1 100644 --- a/test/unit/vy_iterators_helper.c +++ b/test/unit/vy_iterators_helper.c @@ -17,12 +17,13 @@ vy_iterator_C_test_init(size_t cache_size) say_set_log_level(S_WARN); memory_init(); + tuple_format_init(); fiber_init(fiber_c_invoke); tuple_init(NULL); vy_cache_env_create(&cache_env, cord_slab_cache()); vy_cache_env_set_quota(&cache_env, cache_size); vy_key_format = tuple_format_new(&vy_tuple_format_vtab, NULL, NULL, 0, - NULL, 0, NULL, false, 0); + NULL, 0, NULL, false, false, 0); tuple_format_ref(vy_key_format); size_t mem_size = 64 * 1024 * 1024; @@ -202,7 +203,8 @@ create_test_mem(struct key_def *def) struct key_def * const defs[] = { def }; struct tuple_format *format = tuple_format_new(&vy_tuple_format_vtab, NULL, defs, - def->part_count, NULL, 0, NULL, false, 0); + def->part_count, NULL, 0, NULL, false, false, + 0); fail_if(format == NULL); /* Create mem */ @@ -220,7 +222,7 @@ create_test_cache(uint32_t *fields, uint32_t *types, assert(*def != NULL); vy_cache_create(cache, &cache_env, *def, true); *format = tuple_format_new(&vy_tuple_format_vtab, NULL, def, 1, NULL, 0, - NULL, false, 0); + NULL, false, false, 0); tuple_format_ref(*format); } diff --git a/test/unit/vy_mem.c b/test/unit/vy_mem.c index 911f082..dc4665a 100644 --- a/test/unit/vy_mem.c +++ b/test/unit/vy_mem.c @@ -78,7 +78,8 @@ test_iterator_restore_after_insertion() /* Create format */ struct tuple_format *format = tuple_format_new(&vy_tuple_format_vtab, NULL, &key_def, 1, NULL, - 0, NULL, false, 0); + 0, NULL, false, false, + 0); assert(format != NULL); tuple_format_ref(format); diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c index 634fffa..5cc27eb 100644 --- a/test/unit/vy_point_lookup.c +++ b/test/unit/vy_point_lookup.c @@ -85,7 +85,8 @@ test_basic() vy_cache_create(&cache, &cache_env, key_def, true); struct tuple_format *format = tuple_format_new(&vy_tuple_format_vtab, NULL, &key_def, 1, NULL, - 0, NULL, false, 0); + 0, NULL, false, false, + 0); isnt(format, NULL, "tuple_format_new is not NULL"); tuple_format_ref(format); -- 2.19.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] Allow to reuse tuple_formats for ephemeral spaces 2019-01-22 14:07 ` [PATCH 4/4] Allow to reuse tuple_formats for ephemeral spaces Kirill Yukhin @ 2019-01-23 8:38 ` Vladimir Davydov 2019-01-24 12:39 ` Kirill Yukhin 0 siblings, 1 reply; 13+ messages in thread From: Vladimir Davydov @ 2019-01-23 8:38 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches On Tue, Jan 22, 2019 at 05:07:20PM +0300, Kirill Yukhin wrote: > diff --git a/src/box/blackhole.c b/src/box/blackhole.c > index 64e24cd..16958a2 100644 > --- a/src/box/blackhole.c > +++ b/src/box/blackhole.c > @@ -157,7 +157,7 @@ blackhole_engine_create_space(struct engine *engine, struct space_def *def, > struct tuple_format *format; > format = tuple_format_new(&tuple_format_runtime->vtab, NULL, NULL, 0, > def->fields, def->field_count, def->dict, > - false, def->exact_field_count); > + false, false, def->exact_field_count); > if (format == NULL) { > free(space); > return NULL; > diff --git a/src/box/box.cc b/src/box/box.cc > index 9f2fd6d..3799544 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -46,6 +46,7 @@ > #include <rmean.h> > #include "main.h" > #include "tuple.h" > +#include "tuple_format.h" > #include "session.h" > #include "schema.h" > #include "engine.h" > @@ -2050,6 +2051,9 @@ box_init(void) > */ > session_init(); > > + if (tuple_format_init() != 0) > + diag_raise(); > + Please also implement tuple_format_free(). Call it in unit tests. Add it to the commented block in box_free() so that we won't forget it when we fix box shutdown. > if (tuple_init(lua_hash) != 0) > diag_raise(); > > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index 5cf70ab..254ef24 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -994,6 +994,12 @@ memtx_engine_gc_f(va_list va) > struct memtx_engine *memtx = va_arg(va, struct memtx_engine *); > while (!fiber_is_cancelled()) { > bool stop; > + struct errinj *delay = errinj(ERRINJ_MEMTX_DELAY_GC, > + ERRINJ_BOOL); > + if (delay != NULL && delay->bparam) { > + while (delay->bparam) > + fiber_sleep(0.001); > + } > memtx_engine_run_gc(memtx, &stop); > if (stop) { > fiber_yield_timeout(TIMEOUT_INFINITY); > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index 49d09af..e6909f9 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -992,7 +992,8 @@ memtx_space_new(struct memtx_engine *memtx, > struct tuple_format *format = > tuple_format_new(&memtx_tuple_format_vtab, memtx, keys, key_count, > def->fields, def->field_count, def->dict, > - def->opts.is_temporary, def->exact_field_count); > + def->opts.is_temporary, def->opts.is_ephemeral, > + def->exact_field_count); > if (format == NULL) { > free(memtx_space); > return NULL; > diff --git a/src/box/space.c b/src/box/space.c > index 4d174f7..316b34b 100644 > --- a/src/box/space.c > +++ b/src/box/space.c > @@ -194,10 +194,11 @@ space_new(struct space_def *def, struct rlist *key_list) > struct space * > space_new_ephemeral(struct space_def *def, struct rlist *key_list) > { > + assert(def->opts.is_temporary); > + assert(def->opts.is_ephemeral); > struct space *space = space_new(def, key_list); > if (space == NULL) > return NULL; > - space->def->opts.is_temporary = true; Should have been done in patch 2. > space->vtab->init_ephemeral_space(space); > return space; > } > diff --git a/src/box/space_def.c b/src/box/space_def.c > index 1d2bfcb..c37df24 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > @@ -53,6 +53,7 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode, > const struct space_opts space_opts_default = { > /* .group_id = */ 0, > /* .is_temporary = */ false, > + /* .is_ephemeral = */ false, > /* .view = */ false, > /* .sql = */ NULL, > /* .checks = */ NULL, > @@ -61,6 +62,7 @@ const struct space_opts space_opts_default = { > const struct opt_def space_opts_reg[] = { > OPT_DEF("group_id", OPT_UINT32, struct space_opts, group_id), > OPT_DEF("temporary", OPT_BOOL, struct space_opts, is_temporary), > + OPT_DEF("ephemeral", OPT_BOOL, struct space_opts, is_ephemeral), This isn't needed as "ephemeral" space option is purely ephemeral, meaning it should never be persisted. > OPT_DEF("view", OPT_BOOL, struct space_opts, is_view), > OPT_DEF("sql", OPT_STRPTR, struct space_opts, sql), > OPT_DEF_ARRAY("checks", struct space_opts, checks, > @@ -267,6 +269,7 @@ space_def_new_ephemeral(uint32_t field_count) > &space_opts_default, > &field_def_default, 0); > space_def->opts.is_temporary = true; > + space_def->opts.is_ephemeral = true; > return space_def; > } > > diff --git a/src/box/space_def.h b/src/box/space_def.h > index e8d46c5..ea20b1d 100644 > --- a/src/box/space_def.h > +++ b/src/box/space_def.h > @@ -58,6 +58,11 @@ struct space_opts { > * does not require manual release. > */ > bool is_temporary; > + /** > + * This flag is set if space is ephemeral and hence > + * its format might be re-used. > + */ > + bool is_ephemeral; > /** > * If the space is a view, then it can't feature any > * indexes, and must have SQL statement. Moreover, > diff --git a/src/box/tuple.c b/src/box/tuple.c > index 5295800..765e40b 100644 > --- a/src/box/tuple.c > +++ b/src/box/tuple.c > @@ -208,7 +208,8 @@ tuple_init(field_name_hash_f hash) > * Create a format for runtime tuples > */ > tuple_format_runtime = tuple_format_new(&tuple_format_runtime_vtab, NULL, > - NULL, 0, NULL, 0, NULL, false, 0); > + NULL, 0, NULL, 0, NULL, false, > + false, 0); > if (tuple_format_runtime == NULL) > return -1; > > @@ -380,7 +381,8 @@ box_tuple_format_new(struct key_def **keys, uint16_t key_count) > { > box_tuple_format_t *format = > tuple_format_new(&tuple_format_runtime_vtab, NULL, > - keys, key_count, NULL, 0, NULL, false, 0); > + keys, key_count, NULL, 0, NULL, false, false, > + 0); > if (format != NULL) > tuple_format_ref(format); > return format; > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index 6edc0fb..5d54e94 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c > @@ -34,6 +34,8 @@ > #include "tuple_format.h" > #include "coll_id_cache.h" > > +#include "third_party/PMurHash.h" > + > /** Global table of tuple formats */ > struct tuple_format **tuple_formats; > static intptr_t recycled_format_ids = FORMAT_ID_NIL; > @@ -276,7 +278,11 @@ tuple_format_register(struct tuple_format *format) > formats_capacity = new_capacity; > tuple_formats = formats; > } > - if (formats_size == FORMAT_ID_MAX + 1) { > + struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, > + ERRINJ_INT); > + if ((inj != NULL && inj->iparam > 0 > + && formats_size >= inj->iparam + 1) > + || formats_size == FORMAT_ID_MAX + 1) { IMO the code would be easier to follow if you rewrote it as follows: uint32_t formats_size_max = FORMAT_ID_MAX + 1; struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, ERRINJ_INT); if (inf != NULL && inj->iparam > 0) formats_size = inj->iparam; if (formats_size >= fromats_size_max) { > diag_set(ClientError, ER_TUPLE_FORMAT_LIMIT, > (unsigned) formats_capacity); > return -1; > @@ -380,10 +386,77 @@ error: > return NULL; > } > > +static int > +tuple_format_cmp(const struct tuple_format *a, const struct tuple_format *b) > +{ > + if (a->exact_field_count != b->exact_field_count) > + return a->exact_field_count - b->exact_field_count; > + if (tuple_format_field_count(a) != tuple_format_field_count(b)) > + return tuple_format_field_count(a) - tuple_format_field_count(b); > + > + for (uint32_t i = 0; i < tuple_format_field_count(a); ++i) { > + struct tuple_field *field_a = tuple_format_field( > + (struct tuple_format *)a, i); Please don't use const for struct tuple_format to avoid these type conversions. > + struct tuple_field *field_b = tuple_format_field( > + (struct tuple_format *)b, i); > + if (field_a->type != field_b->type) > + return (int)field_a->type - (int)field_b->type; > + if (field_a->coll_id != field_b->coll_id) > + return (int)field_a->coll_id - (int)field_b->coll_id; > + if (field_a->nullable_action != field_b->nullable_action) > + return (int)field_a->nullable_action - > + (int)field_b->nullable_action; > + if (field_a->is_key_part != field_b->is_key_part) > + return (int)field_a->is_key_part - > + (int)field_b->is_key_part; > + } > + > + return 0; > +} > + > +static uint32_t > +tuple_format_hash(struct tuple_format *format) > +{ > + uint32_t h = 0; 'h' should be initially set to a prime number, say 13. Take a look at HASH_SEED in tuple_hash.cc or json.c. > + uint32_t carry = 0; > + uint32_t size = 0; > + for (uint32_t i = 0; i < tuple_format_field_count(format); ++i) { > + struct tuple_field *f = tuple_format_field(format, i); > + PMurHash32_Process(&h, &carry, &f->type, > + sizeof(enum field_type)); > + size += sizeof(enum field_type); > + PMurHash32_Process(&h, &carry, &f->coll_id, > + sizeof(uint32_t)); If we change coll_id type from uint32_t to uint16_t, we'll almost certainly overlook this place. Please use sizeof(f->coll_id). Also, I think we should define a macro here that would compute a hash over a tuple_field struct member and increment the size: tuple_format_hash(...) { #define TUPLE_FIELD_MEMBER_HASH(field, member, h, carry, size) ... for each field TUPLE_FIELD_MEMBER_HASH(f, coll_id, h, carry, size); ... #undef TUPLE_FIELD_MEMBER_HASH } > + size += sizeof(uint32_t); > + PMurHash32_Process(&h, &carry, &f->nullable_action, > + sizeof(enum on_conflict_action)); > + size += sizeof(enum on_conflict_action); > + PMurHash32_Process(&h, &carry, &f->is_key_part, sizeof(bool)); > + size += sizeof(bool); > + } > + return PMurHash32_Result(h, carry, size); > +} > + > +#define MH_SOURCE 1 > +#define mh_name _tuple_format > +#define mh_key_t struct tuple_format * > +#define mh_node_t struct tuple_format * > +#define mh_arg_t void * > +#define mh_hash(a, arg) ((*(a))->hash) > +#define mh_hash_key(a, arg) ((a)->hash) > +#define mh_cmp(a, b, arg) (tuple_format_cmp(*(a), *(b))) > +#define mh_cmp_key(a, b, arg) (tuple_format_cmp((a), *(b))) > +#include "salad/mhash.h" > + > +struct mh_tuple_format_t *tuple_formats_hash = NULL; > + > /** Free tuple format resources, doesn't unregister. */ > static inline void > tuple_format_destroy(struct tuple_format *format) > { > + mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); > + if (key != mh_end(tuple_formats_hash)) > + mh_tuple_format_del(tuple_formats_hash, key, NULL); Since you add a format to the hash in tuple_format_new(), I guess you should remove it from the hash in tuple_format_delete(), not in tuple_format_destroy(). > free(format->required_fields); > tuple_format_destroy_fields(format); > tuple_dictionary_unref(format->dict); > @@ -402,7 +475,8 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > struct key_def * const *keys, uint16_t key_count, > const struct field_def *space_fields, > uint32_t space_field_count, struct tuple_dictionary *dict, > - bool is_temporary, uint32_t exact_field_count) > + bool is_temporary, bool is_ephemeral, > + uint32_t exact_field_count) > { > struct tuple_format *format = > tuple_format_alloc(keys, key_count, space_field_count, dict); > @@ -411,18 +485,46 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > format->vtab = *vtab; > format->engine = engine; > format->is_temporary = is_temporary; > + format->is_ephemeral = is_ephemeral; > format->exact_field_count = exact_field_count; > - if (tuple_format_register(format) < 0) { > - tuple_format_destroy(format); > - free(format); > - return NULL; > - } > if (tuple_format_create(format, keys, key_count, space_fields, > space_field_count) < 0) { > tuple_format_delete(format); > return NULL; > } > - return format; > + format->hash = tuple_format_hash(format); I don't think we should compute the hash if we aren't going to reuse the format: the hash function won't work for all formats as it doesn't check dict, engine, temporary flag. I'd also add assertions making sure those fields are unset if the space is going to be reused. > + struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, ERRINJ_INT); > + if (inj != NULL && inj->iparam == 200) { > + inj = NULL; > + } Looks like this piece left from some preliminary version of the patch. Please remove. > + if (format->is_ephemeral) { > + mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, > + NULL); > + if (key != mh_end(tuple_formats_hash)) { > + struct tuple_format **entry = mh_tuple_format_node(tuple_formats_hash, key); > + tuple_format_destroy(format); > + free(format); > + return *entry; This piece of code should be factored out to a separate function, say tuple_format_reuse(), with a comment explaining why we only reuse ephemeral formats. > + } else { > + if (tuple_format_register(format) < 0) { > + tuple_format_destroy(format); > + free(format); > + return NULL; > + } > + mh_tuple_format_put(tuple_formats_hash, > + (const > + struct tuple_format **)&format, Splitting type conversion looks ugly. Please don't do that. Better not align arguments by parenthesis in such a case: mh_tuple_format_put(tuple_formats_hash, (const struct tuple_format **)&format, NULL, NULL); Also, you forgot to check ENOMEM error here (say hi to #3534 :) > + NULL, NULL); > + return format; > + } > + } else { > + if (tuple_format_register(format) < 0) { Please try to rearrange the code somehow so that there's only one place in the code where you call tuple_format_register(). > + tuple_format_destroy(format); > + free(format); > + return NULL; > + } > + return format; > + } > } > > bool > @@ -823,3 +925,15 @@ error: > tt_sprintf("error in path on position %d", rc)); > return -1; > } > + > +int > +tuple_format_init() > +{ > + tuple_formats_hash = mh_tuple_format_new(); > + if (tuple_formats_hash == NULL) { > + diag_set(OutOfMemory, sizeof(struct mh_tuple_format_t), "malloc", > + "tuple format hash"); > + return -1; > + } > + return 0; > +} > diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h > index 9778fda..d347eb7 100644 > --- a/src/box/tuple_format.h > +++ b/src/box/tuple_format.h > @@ -151,6 +151,11 @@ struct tuple_format { > * in progress. > */ > bool is_temporary; > + /** > + * This format belongs to ephemeral space and thus might > + * be shared with other ephemeral spaces. > + */ > + bool is_ephemeral; > /** > * Size of field map of tuple in bytes. > * \sa struct tuple > @@ -193,6 +198,11 @@ struct tuple_format { > * tuple_field::token. > */ > struct json_tree fields; > + Extra new line. > + /** > + * Hash key for shared formats of epeheral spaces. > + */ > + uint32_t hash; I'd move this definition higher, after 'id'. > }; > > /** > @@ -200,7 +210,7 @@ struct tuple_format { > * a given format. > */ > static inline uint32_t > -tuple_format_field_count(struct tuple_format *format) > +tuple_format_field_count(const struct tuple_format *format) Please don't do that. I deliberately avoid using const for tuple_format as it uses reference counting and json tree, which don't play nicely with const. > { > const struct json_token *root = &format->fields.root; > return root->children != NULL ? root->max_child_idx + 1 : 0; > @@ -263,6 +273,7 @@ tuple_format_unref(struct tuple_format *format) > * @param key_count The number of keys in @a keys array. > * @param space_fields Array of fields, defined in a space format. > * @param space_field_count Length of @a space_fields. > + * @param is_ephemeral Set if format belongs to ephemeral space. is_ephemeral goes after is_temporary in the argument list. > * @param is_temporary Set if format is temporary. > * @param exact_field_count Exact field count for format. > * > @@ -274,7 +285,8 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > struct key_def * const *keys, uint16_t key_count, > const struct field_def *space_fields, > uint32_t space_field_count, struct tuple_dictionary *dict, > - bool is_temporary, uint32_t exact_field_count); > + bool is_temporary, bool is_ephemeral, > + uint32_t exact_field_count); > > /** > * Check, if @a format1 can store any tuples of @a format2. For > @@ -459,6 +471,13 @@ tuple_field_by_part_raw(struct tuple_format *format, const char *data, > return tuple_field_raw(format, data, field_map, part->fieldno); > } > > +/** > + * Initialize tuple formats. I'd rather say 'Initialize tuple format subsystem' or 'Initialize tuple format library'. > + * @retval 0 on success, -1 otherwise. > + */ > +int > +tuple_format_init(); > + > #if defined(__cplusplus) > } /* extern "C" */ > #endif /* defined(__cplusplus) */ > diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua > index fa7f9f2..0b50864 100644 > --- a/test/sql/errinj.test.lua > +++ b/test/sql/errinj.test.lua > @@ -5,6 +5,20 @@ box.sql.execute('pragma sql_default_engine=\''..engine..'\'') > errinj = box.error.injection > fiber = require('fiber') > > +-- gh-3924 Check that tuple_formats of ephemeral spaces are > +-- reused. > +format = {} This assignment isn't needed. Please remove. > +box.sql.execute("CREATE TABLE t4 (id INTEGER PRIMARY KEY, a INTEGER);") > +box.sql.execute("INSERT INTO t4 VALUES (1,1)") > +box.sql.execute("INSERT INTO t4 VALUES (2,1)") > +box.sql.execute("INSERT INTO t4 VALUES (3,2)") > +errinj.set('ERRINJ_TUPLE_FORMAT_COUNT', 200) > +errinj.set('ERRINJ_MEMTX_DELAY_GC', true) > +for i = 1, 201 do box.sql.execute("SELECT DISTINCT a FROM t4") end > +errinj.set('ERRINJ_MEMTX_DELAY_GC', false) > +errinj.set('ERRINJ_TUPLE_FORMAT_COUNT', -1) > +box.sql.execute('DROP TABLE t4') > + > box.sql.execute('create table test (id int primary key, a float, b text)') > box.schema.user.grant('guest','read,write,execute', 'universe') > cn = remote.connect(box.cfg.listen) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] Allow to reuse tuple_formats for ephemeral spaces 2019-01-23 8:38 ` Vladimir Davydov @ 2019-01-24 12:39 ` Kirill Yukhin 0 siblings, 0 replies; 13+ messages in thread From: Kirill Yukhin @ 2019-01-24 12:39 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tarantool-patches Hello Vladimir, Thanks fore review! My answers are inlinded. On 23 Jan 11:38, Vladimir Davydov wrote: > On Tue, Jan 22, 2019 at 05:07:20PM +0300, Kirill Yukhin wrote: > > diff --git a/src/box/box.cc b/src/box/box.cc > > index 9f2fd6d..3799544 100644 > > --- a/src/box/box.cc > > +++ b/src/box/box.cc > > @@ -2050,6 +2051,9 @@ box_init(void) > > */ > > session_init(); > > > > + if (tuple_format_init() != 0) > > + diag_raise(); > > + > > Please also implement tuple_format_free(). Call it in unit tests. > Add it to the commented block in box_free() so that we won't forget > it when we fix box shutdown. Looks like tuple_format_free() already exists and is invoked by tuple_free(). Moved tuple_format_init() to tuple_init() and added call to mhash destructor to tuple_format_free(). > > diff --git a/src/box/space.c b/src/box/space.c > > index 4d174f7..316b34b 100644 > > --- a/src/box/space.c > > +++ b/src/box/space.c > > @@ -194,10 +194,11 @@ space_new(struct space_def *def, struct rlist *key_list) > > struct space * > > space_new_ephemeral(struct space_def *def, struct rlist *key_list) > > { > > + assert(def->opts.is_temporary); > > + assert(def->opts.is_ephemeral); > > struct space *space = space_new(def, key_list); > > if (space == NULL) > > return NULL; > > - space->def->opts.is_temporary = true; > > Should have been done in patch 2. Done. > > diff --git a/src/box/space_def.c b/src/box/space_def.c > > index 1d2bfcb..c37df24 100644 > > --- a/src/box/space_def.c > > +++ b/src/box/space_def.c > > @@ -61,6 +62,7 @@ const struct space_opts space_opts_default = { > > const struct opt_def space_opts_reg[] = { > > OPT_DEF("group_id", OPT_UINT32, struct space_opts, group_id), > > OPT_DEF("temporary", OPT_BOOL, struct space_opts, is_temporary), > > + OPT_DEF("ephemeral", OPT_BOOL, struct space_opts, is_ephemeral), > > This isn't needed as "ephemeral" space option is purely ephemeral, > meaning it should never be persisted. Removed. > > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > > index 6edc0fb..5d54e94 100644 > > --- a/src/box/tuple_format.c > > +++ b/src/box/tuple_format.c > > @@ -276,7 +278,11 @@ tuple_format_register(struct tuple_format *format) > > formats_capacity = new_capacity; > > tuple_formats = formats; > > } > > - if (formats_size == FORMAT_ID_MAX + 1) { > > + struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, > > + ERRINJ_INT); > > + if ((inj != NULL && inj->iparam > 0 > > + && formats_size >= inj->iparam + 1) > > + || formats_size == FORMAT_ID_MAX + 1) { > > IMO the code would be easier to follow if you rewrote it as follows: > > uint32_t formats_size_max = FORMAT_ID_MAX + 1; > struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, > ERRINJ_INT); > if (inf != NULL && inj->iparam > 0) > formats_size = inj->iparam; > if (formats_size >= fromats_size_max) { Applied, thanks. > > @@ -380,10 +386,77 @@ error: > > return NULL; > > } > > > > +static int > > +tuple_format_cmp(const struct tuple_format *a, const struct tuple_format *b) > > +{ > > + if (a->exact_field_count != b->exact_field_count) > > + return a->exact_field_count - b->exact_field_count; > > + if (tuple_format_field_count(a) != tuple_format_field_count(b)) > > + return tuple_format_field_count(a) - tuple_format_field_count(b); > > + > > + for (uint32_t i = 0; i < tuple_format_field_count(a); ++i) { > > + struct tuple_field *field_a = tuple_format_field( > > + (struct tuple_format *)a, i); > > Please don't use const for struct tuple_format to avoid these type > conversions. Mhash comparator declaration won't match in that case. So, let's keep it. > > + struct tuple_field *field_b = tuple_format_field( > > + (struct tuple_format *)b, i); > > + if (field_a->type != field_b->type) > > + return (int)field_a->type - (int)field_b->type; > > + if (field_a->coll_id != field_b->coll_id) > > + return (int)field_a->coll_id - (int)field_b->coll_id; > > + if (field_a->nullable_action != field_b->nullable_action) > > + return (int)field_a->nullable_action - > > + (int)field_b->nullable_action; > > + if (field_a->is_key_part != field_b->is_key_part) > > + return (int)field_a->is_key_part - > > + (int)field_b->is_key_part; > > + } > > + > > + return 0; > > +} > > + > > +static uint32_t > > +tuple_format_hash(struct tuple_format *format) > > +{ > > + uint32_t h = 0; > > 'h' should be initially set to a prime number, say 13. > Take a look at HASH_SEED in tuple_hash.cc or json.c. Okay, but I'd prefer 727. > > + uint32_t carry = 0; > > + uint32_t size = 0; > > + for (uint32_t i = 0; i < tuple_format_field_count(format); ++i) { > > + struct tuple_field *f = tuple_format_field(format, i); > > + PMurHash32_Process(&h, &carry, &f->type, > > + sizeof(enum field_type)); > > + size += sizeof(enum field_type); > > + PMurHash32_Process(&h, &carry, &f->coll_id, > > + sizeof(uint32_t)); > > If we change coll_id type from uint32_t to uint16_t, we'll almost > certainly overlook this place. Please use sizeof(f->coll_id). Done. > Also, I think we should define a macro here that would compute a hash > over a tuple_field struct member and increment the size: > > tuple_format_hash(...) > { > #define TUPLE_FIELD_MEMBER_HASH(field, member, h, carry, size) ... > > for each field > TUPLE_FIELD_MEMBER_HASH(f, coll_id, h, carry, size); > ... > > #undef TUPLE_FIELD_MEMBER_HASH > } Done. > > + size += sizeof(uint32_t); > > + PMurHash32_Process(&h, &carry, &f->nullable_action, > > + sizeof(enum on_conflict_action)); > > + size += sizeof(enum on_conflict_action); > > + PMurHash32_Process(&h, &carry, &f->is_key_part, sizeof(bool)); > > + size += sizeof(bool); > > + } > > + return PMurHash32_Result(h, carry, size); > > +} > > + > > +#define MH_SOURCE 1 > > +#define mh_name _tuple_format > > +#define mh_key_t struct tuple_format * > > +#define mh_node_t struct tuple_format * > > +#define mh_arg_t void * > > +#define mh_hash(a, arg) ((*(a))->hash) > > +#define mh_hash_key(a, arg) ((a)->hash) > > +#define mh_cmp(a, b, arg) (tuple_format_cmp(*(a), *(b))) > > +#define mh_cmp_key(a, b, arg) (tuple_format_cmp((a), *(b))) > > +#include "salad/mhash.h" > > + > > +struct mh_tuple_format_t *tuple_formats_hash = NULL; > > + > > /** Free tuple format resources, doesn't unregister. */ > > static inline void > > tuple_format_destroy(struct tuple_format *format) > > { > > + mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); > > + if (key != mh_end(tuple_formats_hash)) > > + mh_tuple_format_del(tuple_formats_hash, key, NULL); > > Since you add a format to the hash in tuple_format_new(), I guess > you should remove it from the hash in tuple_format_delete(), not in > tuple_format_destroy(). Moved. > > @@ -411,18 +485,46 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > > format->vtab = *vtab; > > format->engine = engine; > > format->is_temporary = is_temporary; > > + format->is_ephemeral = is_ephemeral; > > format->exact_field_count = exact_field_count; > > - if (tuple_format_register(format) < 0) { > > - tuple_format_destroy(format); > > - free(format); > > - return NULL; > > - } > > if (tuple_format_create(format, keys, key_count, space_fields, > > space_field_count) < 0) { > > tuple_format_delete(format); > > return NULL; > > } > > - return format; > > + format->hash = tuple_format_hash(format); > > I don't think we should compute the hash if we aren't going to reuse the > format: the hash function won't work for all formats as it doesn't check > dict, engine, temporary flag. I'd also add assertions making sure those > fields are unset if the space is going to be reused. Done. > > + struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, ERRINJ_INT); > > + if (inj != NULL && inj->iparam == 200) { > > + inj = NULL; > > + } > > Looks like this piece left from some preliminary version of the patch. > Please remove. Whoops, removed. > > + if (format->is_ephemeral) { > > + mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, > > + NULL); > > + if (key != mh_end(tuple_formats_hash)) { > > + struct tuple_format **entry = mh_tuple_format_node(tuple_formats_hash, key); > > + tuple_format_destroy(format); > > + free(format); > > + return *entry; > > This piece of code should be factored out to a separate function, say > tuple_format_reuse(), with a comment explaining why we only reuse > ephemeral formats. Done. > > + } else { > > + if (tuple_format_register(format) < 0) { > > + tuple_format_destroy(format); > > + free(format); > > + return NULL; > > + } > > + mh_tuple_format_put(tuple_formats_hash, > > + (const > > + struct tuple_format **)&format, > > Splitting type conversion looks ugly. Please don't do that. Better not > align arguments by parenthesis in such a case: Re-indented. > mh_tuple_format_put(tuple_formats_hash, > (const struct tuple_format **)&format, > NULL, NULL); > > Also, you forgot to check ENOMEM error here (say hi to #3534 :) Check added. > > + NULL, NULL); > > + return format; > > + } > > + } else { > > + if (tuple_format_register(format) < 0) { > > Please try to rearrange the code somehow so that there's only one place > in the code where you call tuple_format_register(). Extracted into separate routine. > > diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h > > index 9778fda..d347eb7 100644 > > --- a/src/box/tuple_format.h > > +++ b/src/box/tuple_format.h > > @@ -151,6 +151,11 @@ struct tuple_format { > > * in progress. > > */ > > bool is_temporary; > > + /** > > + * This format belongs to ephemeral space and thus might > > + * be shared with other ephemeral spaces. > > + */ > > + bool is_ephemeral; > > /** > > * Size of field map of tuple in bytes. > > * \sa struct tuple > > @@ -193,6 +198,11 @@ struct tuple_format { > > * tuple_field::token. > > */ > > struct json_tree fields; > > + > > Extra new line. Removed. > > + /** > > + * Hash key for shared formats of epeheral spaces. > > + */ > > + uint32_t hash; > > I'd move this definition higher, after 'id'. Moved. > > @@ -200,7 +210,7 @@ struct tuple_format { > > * a given format. > > */ > > static inline uint32_t > > -tuple_format_field_count(struct tuple_format *format) > > +tuple_format_field_count(const struct tuple_format *format) > > Please don't do that. I deliberately avoid using const for tuple_format > as it uses reference counting and json tree, which don't play nicely > with const. Mhash requires it for comparator which in turn uses the routine. Let's keep it. > > { > > const struct json_token *root = &format->fields.root; > > return root->children != NULL ? root->max_child_idx + 1 : 0; > > @@ -263,6 +273,7 @@ tuple_format_unref(struct tuple_format *format) > > * @param key_count The number of keys in @a keys array. > > * @param space_fields Array of fields, defined in a space format. > > * @param space_field_count Length of @a space_fields. > > + * @param is_ephemeral Set if format belongs to ephemeral space. > > is_ephemeral goes after is_temporary in the argument list. Moved. > > @@ -459,6 +471,13 @@ tuple_field_by_part_raw(struct tuple_format *format, const char *data, > > return tuple_field_raw(format, data, field_map, part->fieldno); > > } > > > > +/** > > + * Initialize tuple formats. > > I'd rather say 'Initialize tuple format subsystem' or 'Initialize tuple > format library'. Pasted. > > diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua > > index fa7f9f2..0b50864 100644 > > --- a/test/sql/errinj.test.lua > > +++ b/test/sql/errinj.test.lua > > @@ -5,6 +5,20 @@ box.sql.execute('pragma sql_default_engine=\''..engine..'\'') > > errinj = box.error.injection > > fiber = require('fiber') > > > > +-- gh-3924 Check that tuple_formats of ephemeral spaces are > > +-- reused. > > +format = {} > > This assignment isn't needed. Please remove. Removed. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-01-24 12:39 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-22 14:07 [PATCH 0/4] Reuse tuple formats for ephemeral spaces Kirill Yukhin 2019-01-22 14:07 ` Kirill Yukhin 2019-01-22 14:07 ` [PATCH 1/4] Pass necessary fields to tuple_format contructor Kirill Yukhin 2019-01-23 7:53 ` Vladimir Davydov 2019-01-24 8:13 ` Kirill Yukhin 2019-01-22 14:07 ` [PATCH 2/4] Set is_temporary flag for formats of ephemeral spaces Kirill Yukhin 2019-01-23 8:00 ` Vladimir Davydov 2019-01-24 8:53 ` Kirill Yukhin 2019-01-22 14:07 ` [PATCH 3/4] sql: set error type in case of ephemral space creation failure Kirill Yukhin 2019-01-23 8:00 ` Vladimir Davydov 2019-01-22 14:07 ` [PATCH 4/4] Allow to reuse tuple_formats for ephemeral spaces Kirill Yukhin 2019-01-23 8:38 ` Vladimir Davydov 2019-01-24 12:39 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox