[tarantool-patches] Re: [PATCH 1/5] Create new methods for ephemeral spaces

[Vladislav Shpilevoy <v.shpilevoy@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,