From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B77B7233BA for ; Wed, 18 Jul 2018 04:06:18 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AFbwp9GOhIjo for ; Wed, 18 Jul 2018 04:06:18 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 3BEE122972 for ; Wed, 18 Jul 2018 04:06:18 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/7] box: add space address to index_replace References: <20a3eff418ed30830a6adeeb6e315c5433133c3b.1531832794.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Wed, 18 Jul 2018 11:06:15 +0300 MIME-Version: 1.0 In-Reply-To: <20a3eff418ed30830a6adeeb6e315c5433133c3b.1531832794.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, imeevma@tarantool.org, Kirill Shcherbatov Kirill, please, do the second review. On 17/07/2018 16:08, imeevma@tarantool.org wrote: > On error in some cases index_replace > looking for space using space_id that > is saved in index. It is wrong as > function that calls index_replace > already have that space. Also in case > of ephemeral spaces space_cache_find > doesn't work right as all ephemeral > space have id == 0. > > Part of #3375. > --- > Branch: https://github.com/tarantool/tarantool/compare/imeevma/gh-3375-lua-expose-ephemeral-spaces > Issue: https://github.com/tarantool/tarantool/issues/3375 > > src/box/index.cc | 16 +++++++++------- > src/box/index.h | 30 +++++++++++++++++------------- > src/box/memtx_bitset.c | 7 ++++--- > src/box/memtx_engine.c | 6 +++--- > src/box/memtx_hash.c | 13 +++++-------- > src/box/memtx_rtree.c | 7 ++++--- > src/box/memtx_space.c | 13 +++++++------ > src/box/memtx_tree.c | 17 ++++++++--------- > 8 files changed, 57 insertions(+), 52 deletions(-) > > diff --git a/src/box/index.cc b/src/box/index.cc > index 188995e..94dcf4b 100644 > --- a/src/box/index.cc > +++ b/src/box/index.cc > @@ -510,7 +510,7 @@ index_delete(struct index *index) > } > > int > -index_build(struct index *index, struct index *pk) > +index_build(struct index *index, struct space *space, struct index *pk) > { > ssize_t n_tuples = index_size(pk); > if (n_tuples < 0) > @@ -539,7 +539,7 @@ index_build(struct index *index, struct index *pk) > break; > if (tuple == NULL) > break; > - rc = index_build_next(index, tuple); > + rc = index_build_next(index, space, tuple); > if (rc != 0) > break; > } > @@ -658,10 +658,11 @@ generic_index_get(struct index *index, const char *key, > } > > int > -generic_index_replace(struct index *index, struct tuple *old_tuple, > - struct tuple *new_tuple, enum dup_replace_mode mode, > - struct tuple **result) > +generic_index_replace(struct index *index, struct space *space, > + struct tuple *old_tuple, struct tuple *new_tuple, > + enum dup_replace_mode mode, struct tuple **result) > { > + (void)space; > (void)old_tuple; > (void)new_tuple; > (void)mode; > @@ -709,10 +710,11 @@ generic_index_reserve(struct index *, uint32_t) > } > > int > -generic_index_build_next(struct index *index, struct tuple *tuple) > +generic_index_build_next(struct index *index, struct space *space, > + struct tuple *tuple) > { > struct tuple *unused; > - return index_replace(index, NULL, tuple, DUP_INSERT, &unused); > + return index_replace(index, space, NULL, tuple, DUP_INSERT, &unused); > } > > void > diff --git a/src/box/index.h b/src/box/index.h > index 686e7a1..2b5357a 100644 > --- a/src/box/index.h > +++ b/src/box/index.h > @@ -41,6 +41,7 @@ extern "C" { > > struct tuple; > struct engine; > +struct space; > struct index; > struct index_def; > struct key_def; > @@ -411,9 +412,9 @@ struct index_vtab { > const char *key, uint32_t part_count); > int (*get)(struct index *index, const char *key, > uint32_t part_count, struct tuple **result); > - int (*replace)(struct index *index, struct tuple *old_tuple, > - struct tuple *new_tuple, enum dup_replace_mode mode, > - struct tuple **result); > + int (*replace)(struct index *index, struct space *space, > + struct tuple *old_tuple, struct tuple *new_tuple, > + enum dup_replace_mode mode, struct tuple **result); > /** Create an index iterator. */ > struct iterator *(*create_iterator)(struct index *index, > enum iterator_type type, > @@ -443,7 +444,8 @@ struct index_vtab { > * begin_build(). > */ > int (*reserve)(struct index *index, uint32_t size_hint); > - int (*build_next)(struct index *index, struct tuple *tuple); > + int (*build_next)(struct index *index, struct space *space, > + struct tuple *tuple); > void (*end_build)(struct index *index); > }; > > @@ -504,7 +506,7 @@ index_delete(struct index *index); > > /** Build this index based on the contents of another index. */ > int > -index_build(struct index *index, struct index *pk); > +index_build(struct index *index, struct space *space, struct index *pk); > > static inline void > index_commit_create(struct index *index, int64_t signature) > @@ -596,11 +598,12 @@ index_get(struct index *index, const char *key, > } > > static inline int > -index_replace(struct index *index, struct tuple *old_tuple, > - struct tuple *new_tuple, enum dup_replace_mode mode, > - struct tuple **result) > +index_replace(struct index *index, struct space *space, > + struct tuple *old_tuple, struct tuple *new_tuple, > + enum dup_replace_mode mode, struct tuple **result) > { > - return index->vtab->replace(index, old_tuple, new_tuple, mode, result); > + return index->vtab->replace(index, space, old_tuple, new_tuple, mode, > + result); > } > > static inline struct iterator * > @@ -647,9 +650,9 @@ index_reserve(struct index *index, uint32_t size_hint) > } > > static inline int > -index_build_next(struct index *index, struct tuple *tuple) > +index_build_next(struct index *index, struct space *space, struct tuple *tuple) > { > - return index->vtab->build_next(index, tuple); > + return index->vtab->build_next(index, space, tuple); > } > > static inline void > @@ -674,7 +677,8 @@ int generic_index_random(struct index *, uint32_t, struct tuple **); > ssize_t generic_index_count(struct index *, enum iterator_type, > const char *, uint32_t); > int generic_index_get(struct index *, const char *, uint32_t, struct tuple **); > -int generic_index_replace(struct index *, struct tuple *, struct tuple *, > +int generic_index_replace(struct index *, struct space *, > + struct tuple *, struct tuple *, > enum dup_replace_mode, struct tuple **); > struct snapshot_iterator *generic_index_create_snapshot_iterator(struct index *); > void generic_index_stat(struct index *, struct info_handler *); > @@ -682,7 +686,7 @@ void generic_index_compact(struct index *); > void generic_index_reset_stat(struct index *); > void generic_index_begin_build(struct index *); > int generic_index_reserve(struct index *, uint32_t); > -int generic_index_build_next(struct index *, struct tuple *); > +int generic_index_build_next(struct index *, struct space *, struct tuple *); > void generic_index_end_build(struct index *); > > #if defined(__cplusplus) > diff --git a/src/box/memtx_bitset.c b/src/box/memtx_bitset.c > index a665f1a..ada6fa8 100644 > --- a/src/box/memtx_bitset.c > +++ b/src/box/memtx_bitset.c > @@ -252,10 +252,11 @@ make_key(const char *field, uint32_t *key_len) > } > > static int > -memtx_bitset_index_replace(struct index *base, struct tuple *old_tuple, > - struct tuple *new_tuple, enum dup_replace_mode mode, > - struct tuple **result) > +memtx_bitset_index_replace(struct index *base, struct space *space, > + struct tuple *old_tuple, struct tuple *new_tuple, > + enum dup_replace_mode mode, struct tuple **result) > { > + (void)space; > struct memtx_bitset_index *index = (struct memtx_bitset_index *)base; > > assert(!base->def->opts.is_unique); > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index c210afb..addc477 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -160,7 +160,7 @@ memtx_build_secondary_keys(struct space *space, void *param) > } > > for (uint32_t j = 1; j < space->index_count; j++) { > - if (index_build(space->index[j], pk) < 0) > + if (index_build(space->index[j], space, pk) < 0) > return -1; > } > > @@ -448,8 +448,8 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn, > struct tuple *unused; > struct index *index = space->index[i]; > /* Rollback must not fail. */ > - if (index_replace(index, stmt->new_tuple, stmt->old_tuple, > - DUP_INSERT, &unused) != 0) { > + if (index_replace(index, space, stmt->new_tuple, > + stmt->old_tuple, DUP_INSERT, &unused) != 0) { > diag_log(); > unreachable(); > panic("failed to rollback change"); > diff --git a/src/box/memtx_hash.c b/src/box/memtx_hash.c > index eae07e8..0f70229 100644 > --- a/src/box/memtx_hash.c > +++ b/src/box/memtx_hash.c > @@ -35,7 +35,6 @@ > #include "tuple_hash.h" > #include "memtx_engine.h" > #include "space.h" > -#include "schema.h" /* space_cache_find() */ > #include "errinj.h" > > #include > @@ -244,9 +243,9 @@ memtx_hash_index_get(struct index *base, const char *key, > } > > static int > -memtx_hash_index_replace(struct index *base, struct tuple *old_tuple, > - struct tuple *new_tuple, enum dup_replace_mode mode, > - struct tuple **result) > +memtx_hash_index_replace(struct index *base, struct space *space, > + struct tuple *old_tuple, struct tuple *new_tuple, > + enum dup_replace_mode mode, struct tuple **result) > { > struct memtx_hash_index *index = (struct memtx_hash_index *)base; > struct light_index_core *hash_table = &index->hash_table; > @@ -281,10 +280,8 @@ memtx_hash_index_replace(struct index *base, struct tuple *old_tuple, > "recover of int hash_table"); > } > } > - struct space *sp = space_cache_find(base->def->space_id); > - if (sp != NULL) > - diag_set(ClientError, errcode, base->def->name, > - space_name(sp)); > + diag_set(ClientError, errcode, base->def->name, > + space_name(space)); > return -1; > } > > diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c > index 0b12cda..333d191 100644 > --- a/src/box/memtx_rtree.c > +++ b/src/box/memtx_rtree.c > @@ -210,10 +210,11 @@ memtx_rtree_index_get(struct index *base, const char *key, > } > > static int > -memtx_rtree_index_replace(struct index *base, struct tuple *old_tuple, > - struct tuple *new_tuple, enum dup_replace_mode mode, > - struct tuple **result) > +memtx_rtree_index_replace(struct index *base, struct space *space, > + struct tuple *old_tuple, struct tuple *new_tuple, > + enum dup_replace_mode mode, struct tuple **result) > { > + (void)space; > (void)mode; > struct memtx_rtree_index *index = (struct memtx_rtree_index *)base; > struct rtree_rect rect; > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index b94c4ab..185be92 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -124,7 +124,7 @@ memtx_space_replace_build_next(struct space *space, struct tuple *old_tuple, > panic("Failed to commit transaction when loading " > "from snapshot"); > } > - if (index_build_next(space->index[0], new_tuple) != 0) > + if (index_build_next(space->index[0], space, new_tuple) != 0) > return -1; > memtx_space_update_bsize(space, NULL, new_tuple); > return 0; > @@ -140,7 +140,7 @@ memtx_space_replace_primary_key(struct space *space, struct tuple *old_tuple, > enum dup_replace_mode mode, > struct tuple **result) > { > - if (index_replace(space->index[0], old_tuple, > + if (index_replace(space->index[0], space, old_tuple, > new_tuple, mode, &old_tuple) != 0) > return -1; > memtx_space_update_bsize(space, old_tuple, new_tuple); > @@ -259,7 +259,8 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple, > * If old_tuple is not NULL, the index has to > * find and delete it, or return an error. > */ > - if (index_replace(pk, old_tuple, new_tuple, mode, &old_tuple) != 0) > + if (index_replace(pk, space, old_tuple, > + new_tuple, mode, &old_tuple) != 0) > return -1; > assert(old_tuple || new_tuple); > > @@ -267,7 +268,7 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple, > for (i++; i < space->index_count; i++) { > struct tuple *unused; > struct index *index = space->index[i]; > - if (index_replace(index, old_tuple, new_tuple, > + if (index_replace(index, space, old_tuple, new_tuple, > DUP_INSERT, &unused) != 0) > goto rollback; > } > @@ -281,7 +282,7 @@ rollback: > struct tuple *unused; > struct index *index = space->index[i - 1]; > /* Rollback must not fail. */ > - if (index_replace(index, new_tuple, old_tuple, > + if (index_replace(index, space, new_tuple, old_tuple, > DUP_INSERT, &unused) != 0) { > diag_log(); > unreachable(); > @@ -898,7 +899,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index, > * @todo: better message if there is a duplicate. > */ > struct tuple *old_tuple; > - rc = index_replace(new_index, NULL, tuple, > + rc = index_replace(new_index, src_space, NULL, tuple, > DUP_INSERT, &old_tuple); > if (rc != 0) > break; > diff --git a/src/box/memtx_tree.c b/src/box/memtx_tree.c > index f851fb8..cf5d14f 100644 > --- a/src/box/memtx_tree.c > +++ b/src/box/memtx_tree.c > @@ -31,7 +31,6 @@ > #include "memtx_tree.h" > #include "memtx_engine.h" > #include "space.h" > -#include "schema.h" /* space_cache_find() */ > #include "errinj.h" > #include "memory.h" > #include "fiber.h" > @@ -438,9 +437,9 @@ memtx_tree_index_get(struct index *base, const char *key, > } > > static int > -memtx_tree_index_replace(struct index *base, struct tuple *old_tuple, > - struct tuple *new_tuple, enum dup_replace_mode mode, > - struct tuple **result) > +memtx_tree_index_replace(struct index *base, struct space *space, > + struct tuple *old_tuple, struct tuple *new_tuple, > + enum dup_replace_mode mode, struct tuple **result) > { > struct memtx_tree_index *index = (struct memtx_tree_index *)base; > if (new_tuple) { > @@ -461,10 +460,8 @@ memtx_tree_index_replace(struct index *base, struct tuple *old_tuple, > memtx_tree_delete(&index->tree, new_tuple); > if (dup_tuple) > memtx_tree_insert(&index->tree, dup_tuple, 0); > - struct space *sp = space_cache_find(base->def->space_id); > - if (sp != NULL) > - diag_set(ClientError, errcode, base->def->name, > - space_name(sp)); > + diag_set(ClientError, errcode, base->def->name, > + space_name(space)); > return -1; > } > if (dup_tuple) { > @@ -549,8 +546,10 @@ memtx_tree_index_reserve(struct index *base, uint32_t size_hint) > } > > static int > -memtx_tree_index_build_next(struct index *base, struct tuple *tuple) > +memtx_tree_index_build_next(struct index *base, struct space *space, > + struct tuple *tuple) > { > + (void)space; > struct memtx_tree_index *index = (struct memtx_tree_index *)base; > if (index->build_array == NULL) { > index->build_array = (struct tuple **)malloc(MEMTX_EXTENT_SIZE); >