[tarantool-patches] Re: [PATCH 1/5] Create new methods for ephemeral spaces
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jul 13 19:32:17 MSK 2018
Hello. Thanks for the patch! See 11 comments below.
On 12/07/2018 14:16, imeevma at 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,
More information about the Tarantool-patches
mailing list