Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Kirill Yukhin <kyukhin@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH v2 1/4] schema: refactor space_cache API
Date: Thu, 25 Oct 2018 17:48:14 +0300	[thread overview]
Message-ID: <20181025144814.zwqflwpqsxxjlbd3@esperanza> (raw)
In-Reply-To: <1e8ea1c80fd917dc78aea785e1d95c76241cf830.1540388902.git.kyukhin@tarantool.org>

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?

> ---
>  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.

>   *
>   * - 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.

>  		/*
>  		 * 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.

>  	}
>  	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.

>   */
> -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();

  reply	other threads:[~2018-10-25 14:48 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-25  8:17 [PATCH v2 0/4] Check read access while executing SQL query Kirill Yukhin
2018-10-25  8:17 ` [PATCH v2 1/4] schema: refactor space_cache API Kirill Yukhin
2018-10-25 14:48   ` Vladimir Davydov [this message]
2018-10-25 15:31     ` Kirill Yukhin
2018-10-25 16:09       ` Vladimir Davydov
2018-10-25  8:17 ` [PATCH v2 2/4] schema: add space names cache Kirill Yukhin
2018-10-26 20:55   ` Vladimir Davydov
2018-11-01 11:34     ` [tarantool-patches] " Konstantin Osipov
2018-10-25  8:17 ` [PATCH v2 3/4] sql: use space_by_name in SQL Kirill Yukhin
2018-10-26 21:00   ` Vladimir Davydov
2018-10-25  8:17 ` [PATCH v2 4/4] sql: check read access while executing SQL query Kirill Yukhin
2018-10-26 21:04   ` Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181025144814.zwqflwpqsxxjlbd3@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=kyukhin@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [PATCH v2 1/4] schema: refactor space_cache API' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox