[PATCH v2 1/4] schema: refactor space_cache API
Kirill Yukhin
kyukhin at tarantool.org
Thu Oct 25 18:31:02 MSK 2018
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);
More information about the Tarantool-patches
mailing list