Re: [PATCH v2 2/4] schema: add space names cache

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Thu, Oct 25, 2018 at 11:17:10AM +0300, Kirill Yukhin wrote:
> Since SQL is heavily using name -> space mapping,
> introduce (instead of scanning _space space) dedicated
> cache, which maps space name to space pointer.
> ---
>  src/box/schema.cc | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/box/schema.h  |  8 ++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 08457a46..b931fdf 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -55,6 +55,7 @@
>  
>  /** All existing spaces. */
>  static struct mh_i32ptr_t *spaces;
> +static struct mh_strnptr_t *spaces_by_name;
>  static struct mh_i32ptr_t *funcs;
>  static struct mh_strnptr_t *funcs_by_name;
>  static struct mh_i32ptr_t *sequences;
> @@ -95,6 +96,17 @@ space_by_id(uint32_t id)
>  	return (struct space *) mh_i32ptr_node(spaces, space)->val;
>  }
>  
> +/** Return space by its name */
> +struct space *
> +space_by_name(const char *name)
> +{
> +	mh_int_t space = mh_strnptr_find_inp(spaces_by_name, name,
> +					     strlen(name));
> +	if (space == mh_end(spaces_by_name))
> +		return NULL;
> +	return (struct space *) mh_strnptr_node(spaces_by_name, space)->val;
> +}
> +
>  /** Return current schema version */
>  uint32_t
>  box_schema_version()
> @@ -165,7 +177,25 @@ void
>  space_cache_replace(struct space *old_space, struct space *new_space)
>  {
>  	assert(new_space != NULL || old_space != NULL);
> +	struct space *old_space_by_n = NULL;
>  	if (new_space != NULL) {
> +		/*
> +		 * If replacing is performed and space name was
> +		 * changed, then need to explicitly remove old
> +		 * entry from spaces cache.
> +		 * NB: Since space_id never changed - no need
> +		 * to do so for space_id cache.
> +		 */
> +		if (old_space != NULL && strcmp(space_name(old_space),
> +						new_space->def->name) != 0) {
> +			const char *name = space_name(old_space);
> +			mh_int_t k = mh_strnptr_find_inp(spaces_by_name, name,
> +							 strlen(name));
> +			assert(k != mh_end(spaces_by_name));
> +			mh_strnptr_del(spaces_by_name, k, NULL);
> +			old_space_by_n = (struct space *)mh_strnptr_node(spaces_by_name,
> +									 k)->val;
> +		}
>  		const struct mh_i32ptr_node_t node_p = { space_id(new_space),
>  							 new_space };
>  		struct mh_i32ptr_node_t old, *p_old = &old;
> @@ -174,7 +204,23 @@ space_cache_replace(struct space *old_space, struct space *new_space)
>  			panic_syserror("Out of memory for the data "
>  				       "dictionary cache.");
>  		}
> -		assert((struct space *)p_old->val == old_space);
> +		const char *name = space_name(new_space);
> +		uint32_t name_len = strlen(name);
> +		uint32_t hash = mh_strn_hash(name, name_len);
> +		const struct mh_strnptr_node_t node_s = { name, name_len, hash,
> +							  new_space };
> +		struct mh_strnptr_node_t old_s, *p_old_s = &old_s;
> +		k = mh_strnptr_put(spaces_by_name, &node_s, &p_old_s, NULL);
> +		if (k == mh_end(spaces_by_name)) {
> +			panic_syserror("Out of memory for the data "
> +				       "dictionary cache.");
> +		}
> +		assert(old_space_by_n == NULL || p_old_s == NULL);
> +		if(old_space_by_n == NULL && p_old != NULL) {
                                             ^^^^^
Apparently, it should be p_old_s here (with _s), but when I tried to fix
that, sql/view started to fail on the assertion checking old_space_by_n
below. Turned out there was a bug in on_replace_dd_space (see the patch
below for more details). I fixed it and then pushed this patch (after
some code cleanup) to 2.1. I wonder if it really was a typo or you
simply didn't want to dig deeper...

> +			assert(p_old->val == p_old_s->val);
> +			old_space_by_n = (struct space *)p_old->val;
> +		}
> +		assert((struct space*)old_space_by_n == old_space);
>  	} else {
>  		mh_int_t k = mh_i32ptr_find(spaces, space_id(old_space), NULL);
>  		assert(k != mh_end(spaces));

From d792fd2664cdbf87237253f96c96d94e833e822c Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Fri, 26 Oct 2018 23:10:50 +0300
Subject: [PATCH] alter: install space commit/rollback triggers before
 preparing sql view

sql_compile_view() may fail, in which case the space will never be
deleted from (in case of space creation) or inserted back into (in case
of space drop) the space cache, because commit/rollback triggers, which
are supposed to do the job, are only installed after preparing a view.
Fix this by installing triggers before sql_compile_view().

No need to write a test as without this commit sql/view test will
crash after applying the next commit (the one that introduces space
name cache).

Fixes commit dc358cb01428 ("sql: rework VIEW internals").

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 986d4daf..a6bb5a0f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1702,6 +1702,12 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		 * so it's safe to simply drop the space on
 		 * rollback.
 		 */
+		struct trigger *on_commit =
+			txn_alter_trigger_new(on_create_space_commit, space);
+		txn_on_commit(txn, on_commit);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_create_space_rollback, space);
+		txn_on_rollback(txn, on_rollback);
 		if (def->opts.is_view) {
 			struct Select *select = sql_view_compile(sql_get(),
 								 def->opts.sql);
@@ -1732,12 +1738,6 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 			txn_on_rollback(txn, on_rollback_view);
 			select_guard.is_active = false;
 		}
-		struct trigger *on_commit =
-			txn_alter_trigger_new(on_create_space_commit, space);
-		txn_on_commit(txn, on_commit);
-		struct trigger *on_rollback =
-			txn_alter_trigger_new(on_create_space_rollback, space);
-		txn_on_rollback(txn, on_rollback);
 	} else if (new_tuple == NULL) { /* DELETE */
 		access_check_ddl(old_space->def->name, old_space->def->id,
 				 old_space->def->uid, SC_SPACE, PRIV_D, true);
@@ -1788,6 +1788,9 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		struct trigger *on_commit =
 			txn_alter_trigger_new(on_drop_space_commit, old_space);
 		txn_on_commit(txn, on_commit);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_drop_space_rollback, old_space);
+		txn_on_rollback(txn, on_rollback);
 		if (old_space->def->opts.is_view) {
 			struct Select *select =
 				sql_view_compile(sql_get(),
@@ -1807,10 +1810,6 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 			txn_on_rollback(txn, on_rollback_view);
 			select_guard.is_active = false;
 		}
-		struct trigger *on_rollback =
-			txn_alter_trigger_new(on_drop_space_rollback,
-					      old_space);
-		txn_on_rollback(txn, on_rollback);
 	} else { /* UPDATE, REPLACE */
 		assert(old_space != NULL && new_tuple != NULL);
 		struct space_def *def =