From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 25 Oct 2018 18:31:02 +0300 From: Kirill Yukhin Subject: Re: [PATCH v2 1/4] schema: refactor space_cache API Message-ID: <20181025153102.gtbqkwx3til54g4k@tarantool.org> References: <1e8ea1c80fd917dc78aea785e1d95c76241cf830.1540388902.git.kyukhin@tarantool.org> <20181025144814.zwqflwpqsxxjlbd3@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20181025144814.zwqflwpqsxxjlbd3@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: 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);