From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Mon, 24 Jun 2019 13:22:50 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v4 1/6] box: rework func cache update machinery
Message-ID: <20190624102250.auiu556wtnri6fj3@esperanza>
References: <cover.1561298197.git.kshcherbatov@tarantool.org>
 <b9061489ce02424eef1a0660e48a9446ead036fb.1561298197.git.kshcherbatov@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b9061489ce02424eef1a0660e48a9446ead036fb.1561298197.git.kshcherbatov@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Sun, Jun 23, 2019 at 04:57:52PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 6d3153815..d63add535 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -541,39 +542,27 @@ schema_free(void)
>  }
>  
>  void
> -func_cache_replace(struct func_def *def)
> +func_cache_insert(struct func *func)
>  {
> -	struct func *old = func_by_id(def->fid);
> -	if (old) {
> -		func_update(old, def);
> -		return;
> -	}
> -	if (mh_size(funcs) >= BOX_FUNCTION_MAX)
> -		tnt_raise(ClientError, ER_FUNCTION_MAX, BOX_FUNCTION_MAX);
> -	struct func *func = func_new(def);
> -	if (func == NULL) {
> -error:
> -		panic_syserror("Out of memory for the data "
> -			       "dictionary cache (stored function).");
> -	}
> -	const struct mh_i32ptr_node_t node = { def->fid, func };
> +	assert(func_by_id(func->def->fid) == NULL);
> +	assert(func_by_name(func->def->name, strlen(func->def->name)) == NULL);
> +	const struct mh_i32ptr_node_t node = { func->def->fid, func };
>  	mh_int_t k1 = mh_i32ptr_put(funcs, &node, NULL, NULL);
>  	if (k1 == mh_end(funcs)) {
> -		func->def = NULL;
>  		func_delete(func);
> -		goto error;
> +		panic_syserror("Out of memory for the data dictionary cache "
> +			       "(stored function).");
>  	}
>  	size_t def_name_len = strlen(func->def->name);
>  	uint32_t name_hash = mh_strn_hash(func->def->name, def_name_len);
>  	const struct mh_strnptr_node_t strnode = {
>  		func->def->name, def_name_len, name_hash, func };
> -
>  	mh_int_t k2 = mh_strnptr_put(funcs_by_name, &strnode, NULL, NULL);
>  	if (k2 == mh_end(funcs_by_name)) {
>  		mh_i32ptr_del(funcs, k1, NULL);
> -		func->def = NULL;
>  		func_delete(func);

No need to delete the function before panic. Moreover, we shouldn't do
it even if we didn't panic, because the function was created by the
caller.

> -		goto error;
> +		panic_syserror("Out of memory for the data dictionary cache "
> +			       "(stored function).");

Code duplication.

>  	}
>  }
>  

Pushed to master with the following changes:

diff --git a/src/box/schema.cc b/src/box/schema.cc
index d63add53..87fe18f3 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -549,9 +549,9 @@ func_cache_insert(struct func *func)
 	const struct mh_i32ptr_node_t node = { func->def->fid, func };
 	mh_int_t k1 = mh_i32ptr_put(funcs, &node, NULL, NULL);
 	if (k1 == mh_end(funcs)) {
-		func_delete(func);
-		panic_syserror("Out of memory for the data dictionary cache "
-			       "(stored function).");
+error:
+		panic_syserror("Out of memory for the data "
+			       "dictionary cache (stored function).");
 	}
 	size_t def_name_len = strlen(func->def->name);
 	uint32_t name_hash = mh_strn_hash(func->def->name, def_name_len);
@@ -560,9 +560,7 @@ func_cache_insert(struct func *func)
 	mh_int_t k2 = mh_strnptr_put(funcs_by_name, &strnode, NULL, NULL);
 	if (k2 == mh_end(funcs_by_name)) {
 		mh_i32ptr_del(funcs, k1, NULL);
-		func_delete(func);
-		panic_syserror("Out of memory for the data dictionary cache "
-			       "(stored function).");
+		goto error;
 	}
 }