[tarantool-patches] Re: [PATCH v2 1/4] schema: refactor space_cache API

  • From: Kirill Yukhin <kyukhin@xxxxxxxxxxxxx>
  • To: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
  • Date: Thu, 25 Oct 2018 18:31:02 +0300

Hello,

Incremental patch in the bottom. My answers are inlines.

On 25 Oct 17:48, Vladimir Davydov wrote:

On Thu, Oct 25, 2018 at 11:17:09AM +0300, Kirill Yukhin wrote:
Remove function which deletes from cache, making replace
more general: it might be used for both insertions,
deletions and replaces. Also, put assert on equality
of space pointer found in cache to old one into
replace routine.

I guess this refactoring should be pushed to 1.10-features, right?

It's up to you.

---
 src/box/alter.cc  | 26 ++++++++++----------------
 src/box/schema.cc | 52 +++++++++++++++++++++++++---------------------------
 src/box/schema.h  |  9 ++-------
 3 files changed, 37 insertions(+), 50 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index de3943c..79ff589 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1610,8 +1604,7 @@ on_drop_view_rollback(struct trigger *trigger, void 
*event)
  *
  * - space cache should be updated, and changes in the space
  *   cache should be reflected in Lua bindings
- *   (this is done in space_cache_replace() and
- *   space_cache_delete())
+ *   (this is done in space_cache_replace())

This comment is obsolete. Lua bindings are updated by an on_alter_space
trigger callback. Please fix.

Removed Lua mention.

  *
  * - the space which is changed should be rebuilt according
  *   to the nature of the modification, i.e. indexes added/dropped,
@@ -1695,7 +1688,7 @@ on_replace_dd_space(struct trigger * /* trigger */, 
void *event)
             * cache right away to achieve linearisable
             * execution on a replica.
             */
-           (void) space_cache_replace(space);
+           (void) space_cache_replace(NULL, space);

(void) is not needed here anymore.

Done.

            /*
             * Do not forget to update schema_version right after
             * inserting the space to the space_cache, since no
@@ -447,8 +446,7 @@ schema_free(void)
 
            struct space *space = (struct space *)
                            mh_i32ptr_node(spaces, i)->val;
-           space_cache_delete(space_id(space));
-           space_delete(space);
+           space_cache_replace(space, NULL);

space_cache_replace(), just like its predecessor space_cache_delete(),
doesn't delete the space so space_delete() must remain.

Reverted.

    }
    mh_i32ptr_delete(spaces);
    while (mh_size(funcs) > 0) {
diff --git a/src/box/schema.h b/src/box/schema.h
index 264c16b..05f32de 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -134,14 +134,9 @@ space_cache_find_xc(uint32_t id)
 /**
  * Update contents of the space cache.  Typically the new space is
  * an altered version of the original space.
- * Returns the old space, if any.

What's the old and new spaces are for? Please improve the comment.
Done.

  */
-struct space *
-space_cache_replace(struct space *space);
-
-/** Delete a space from the space cache. */
-struct space *
-space_cache_delete(uint32_t id);
+void
+space_cache_replace(struct space *old_space, struct space *new_space);
 
 void
 schema_init();

--
Regards, Kirill Yukhin

index 79ff589..986d4da 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1602,9 +1602,7 @@ on_drop_view_rollback(struct trigger *trigger, void 
*event)
  * Generally, whenever a data dictionary change occurs
  * 2 things should be done:
  *
- * - space cache should be updated, and changes in the space
- *   cache should be reflected in Lua bindings
- *   (this is done in space_cache_replace())
+ * - space cache should be updated
  *
  * - the space which is changed should be rebuilt according
  *   to the nature of the modification, i.e. indexes added/dropped,
@@ -1688,7 +1686,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void 
*event)
                 * cache right away to achieve linearisable
                 * execution on a replica.
                 */
-          (void) space_cache_replace(NULL, space);
+               space_cache_replace(NULL, space);
                /*
                 * Do not forget to update schema_version right after
                 * inserting the space to the space_cache, since no
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 08457a4..87a5c41 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -446,6 +446,7 @@ schema_free(void)

                struct space *space = (struct space *)
                                mh_i32ptr_node(spaces, i)->val;
+               space_delete(space);
                space_cache_replace(space, NULL);
        }
        mh_i32ptr_delete(spaces);
diff --git a/src/box/schema.h b/src/box/schema.h
index 05f32de..66ab3bc 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -132,8 +132,8 @@ space_cache_find_xc(uint32_t id)
 }

 /**
- * Update contents of the space cache.  Typically the new space is
- * an altered version of the original space.
+ * Update contents of the space caches with new space pointer.
+ * Old space pointer must exist in the caches.
  */
 void
 space_cache_replace(struct space *old_space, struct space *new_space);

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 10-2018