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

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx, imeevma@xxxxxxxxxxxxx
  • Date: Fri, 13 Jul 2018 19:32:17 +0300

Hello. Thanks for the patch! See 11 comments below.

On 12/07/2018 14:16, imeevma@xxxxxxxxxxxxx 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,

Other related posts: