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 2882427484 for ; Fri, 13 Jul 2018 12:32:21 -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 H-VezsaAycLE for ; Fri, 13 Jul 2018 12:32:21 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 B50DF2744D for ; Fri, 13 Jul 2018 12:32:20 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/5] Create new methods for ephemeral spaces References: <83b9283a8e61ece6bd66834d403bc18b1acee32c.1531393821.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Fri, 13 Jul 2018 19:32:17 +0300 MIME-Version: 1.0 In-Reply-To: <83b9283a8e61ece6bd66834d403bc18b1acee32c.1531393821.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 Hello. Thanks for the patch! See 11 comments below. On 12/07/2018 14:16, imeevma@tarantool.org wrote: > Up to this patch any space had two additional methods > that were methods of ephemeral spaces. In this patch > these methods deleted from vtab and from now on > ephemeral spaces are spaces with special vtab. > > Part of #3375. > --- > src/box/box.cc | 108 ++++++++++++++++++++++++ > src/box/box.h | 42 ++++++++++ > src/box/memtx_space.c | 210 ++++++++++++++++++++++++++++++++++++++++------- > src/box/memtx_tree.c | 5 ++ > src/box/space.h | 17 ---- > src/box/sql.c | 10 +-- > src/box/sysview_engine.c | 22 ----- > src/box/vinyl.c | 22 ----- > 8 files changed, 339 insertions(+), 97 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 481c2dd..795e3ee 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1170,6 +1170,114 @@ box_upsert(uint32_t space_id, uint32_t index_id, const char *tuple, > return box_process1(&request, result); > } > > +int > +box_ephemeral_insert(struct space *space, const char *tuple, > + const char *tuple_end, box_tuple_t **result) > +{ > + mp_tuple_assert(tuple, tuple_end); > + struct request request; > + memset(&request, 0, sizeof(request)); > + request.type = IPROTO_INSERT; > + request.space_id = space->def->id; 1. Do you really need this space_id filling? As I understand, ephemeral space id is always 0, so it was nullified already in memset above. 2. Please, extract request initialization into separate functions, that will be called from box_insert and box_ephemeral_insert. Same for other functions. > + request.tuple = tuple; > + request.tuple_end = tuple_end; > + if(result != NULL) > + return space_execute_dml(space, NULL, &request, result); > + struct tuple *temp_result; > + return space_execute_dml(space, NULL, &request, &temp_result); 3. Insert returns the tuple too, so here temp_result leaks, it is not? I am not sure, just a question. 4. Lets make this API similar to other box methods: result is mandatory non-NULL argument. All the same comments for other functions. > diff --git a/src/box/box.h b/src/box/box.h > index d879847..a00a842 100644 > --- a/src/box/box.h > +++ b/src/box/box.h > @@ -402,6 +402,48 @@ int > box_process1(struct request *request, box_tuple_t **result); > > /** > + * Used to prepare request for inserting tuple into 5. Lets use imperative for function comment to respect the code style. > + * ephemeral space and call box_process_rw(). 6. But actually it does not call box_process_rw()... Same about functions below. > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index b94c4ab..e4781e3 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -543,56 +543,51 @@ memtx_space_execute_upsert(struct space *space, struct txn *txn, > } > > /** > - * This function simply creates new memtx tuple, refs it and calls space's > - * replace function. In constrast to original memtx_space_execute_replace(), it > - * doesn't handle any transaction routine. > - * Ephemeral spaces shouldn't be involved in transaction routine, since > - * they are used only for internal purposes. Moreover, ephemeral spaces > - * can be created and destroyed within one transaction and rollback of already > - * destroyed space may lead to undefined behaviour. For this reason it > - * doesn't take txn as an argument. > + * This function creates new memtx tuple, refs it and calls > + * ephemeral space's replace function. > + * > + * This function isn't involved in any transaction routine. > */ > static int > -memtx_space_ephemeral_replace(struct space *space, const char *tuple, > - const char *tuple_end) > +memtx_space_ephemeral_replace(struct space *space, struct txn *txn, > + struct request *request, struct tuple **result) > { > + (void)txn; 7. Please, add an assertion that txn == NULL. Same for other functions. > struct memtx_space *memtx_space = (struct memtx_space *)space; > - struct tuple *new_tuple = memtx_tuple_new(space->format, tuple, > - tuple_end); > + enum dup_replace_mode mode = dup_replace_mode(request->type); > + struct tuple *new_tuple = memtx_tuple_new(space->format, request->tuple, > + request->tuple_end); > if (new_tuple == NULL) > return -1; > tuple_ref(new_tuple); > struct tuple *old_tuple = NULL; > if (memtx_space->replace(space, old_tuple, new_tuple, > - DUP_REPLACE_OR_INSERT, &old_tuple) != 0) { > + mode, &old_tuple) != 0) { > tuple_unref(new_tuple); > return -1; > } > + *result = new_tuple; > if (old_tuple != NULL) > tuple_unref(old_tuple); > return 0; > } > @@ -601,7 +596,138 @@ memtx_space_ephemeral_delete(struct space *space, const char *key) > memtx_space->replace(space, old_tuple, NULL, > DUP_REPLACE, &old_tuple) != 0) > return -1; > - tuple_unref(old_tuple); > + *result = old_tuple; > + return 0; > +} > + > +/** > + * Replace tuple with given key from primary index of > + * ephemeral space with new tuple. > + * > + * This function isn't involved in any transaction routine. > + */ > +static int > +memtx_space_ephemeral_update(struct space *space, struct txn *txn, > + struct request *request, struct tuple **result) > +{ > + (void)txn; > + struct memtx_space *memtx_space = (struct memtx_space *)space; > + struct index *pk = space_index(space, 0 /* primary index*/); 8. Lets better get the pk the same way as in non-ephemeral space: via index_find_unique. When we will add alter, there are non-unique indexes too. 9. Lets extract all these DML routines into _impl() functions and call them from ephemeral and real DML. There are too many code duplication. > @@ -943,8 +1066,6 @@ static const struct space_vtab memtx_space_vtab = { > /* .execute_delete = */ memtx_space_execute_delete, > /* .execute_update = */ memtx_space_execute_update, > /* .execute_upsert = */ memtx_space_execute_upsert, > - /* .ephemeral_replace = */ memtx_space_ephemeral_replace, > - /* .ephemeral_delete = */ memtx_space_ephemeral_delete, > /* .init_system_space = */ memtx_init_system_space, > /* .init_ephemeral_space = */ memtx_init_ephemeral_space, > /* .check_index_def = */ memtx_space_check_index_def, > @@ -957,6 +1078,33 @@ static const struct space_vtab memtx_space_vtab = { > /* .prepare_alter = */ memtx_space_prepare_alter, > }; > > +static const struct space_vtab memtx_space_ephemeral_vtab = { > + /* .destroy = */ memtx_space_destroy, > + /* .bsize = */ memtx_space_bsize, > + /* .apply_initial_join_row = */ memtx_space_apply_initial_join_row, > + /* .execute_replace = */ memtx_space_ephemeral_replace, > + /* .execute_delete = */ memtx_space_ephemeral_delete, > + /* .execute_update = */ memtx_space_ephemeral_update, > + /* .execute_upsert = */ memtx_space_ephemeral_upsert, > + /* .init_system_space = */ memtx_init_system_space, > + /* .init_ephemeral_space = */ memtx_init_ephemeral_space, 10. I do not think that we should allow to call ephemeral space initialization from ephemeral space. It should be NULL or implemented as diag_set(ER_UNSUPPORTED). > + /* .check_index_def = */ memtx_space_check_index_def, > + /* .create_index = */ memtx_space_create_index, > + /* .add_primary_key = */ memtx_space_add_primary_key, > + /* .drop_primary_key = */ memtx_space_drop_primary_key, > + /* .check_format = */ memtx_space_check_format, > + /* .build_index = */ memtx_space_build_index, > + /* .swap_index = */ generic_space_swap_index, > + /* .prepare_alter = */ memtx_space_prepare_alter, > +}; > diff --git a/src/box/memtx_tree.c b/src/box/memtx_tree.c > index f851fb8..b0634f9 100644 > --- a/src/box/memtx_tree.c > +++ b/src/box/memtx_tree.c > @@ -461,6 +461,11 @@ 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); > + if (base->def->space_id == 0) { > + diag_set(ClientError, errcode, base->def->name, > + "ephemeral"); > + return -1; > + } 11. Why? Can not understand why do you check here for ephemerality, and why is it bad. > struct space *sp = space_cache_find(base->def->space_id); > if (sp != NULL) > diag_set(ClientError, errcode, base->def->name,