Re: [PATCH v3 1/6] box: rework func cache update machinery

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

On Thu, Jun 13, 2019 at 05:08:21PM +0300, Kirill Shcherbatov wrote:
> Tarantool used to assume that func_new call must not fail and
> it used to build a new func object by given definition just on
> func cache replace operation. We need to fix it to perform
> user-dependent risky actions like Lua function assemble in
> further patches.
> 
> The replace method is disallowed for _func space because it is
> redundant and difficult to maintain in case of functions that
> have pre-compiled runtime.
> 
> Needed for #4182, #1260
> ---
>  src/box/alter.cc            | 57 +++++++++++++++++++------------------
>  src/box/func.c              |  8 ------
>  src/box/func.h              |  3 --
>  src/box/func_def.c          | 17 +++++++++++
>  src/box/func_def.h          | 12 ++++++++
>  src/box/schema.cc           | 37 +++++++++---------------
>  src/box/schema.h            | 10 +++----
>  test/box/function1.result   |  4 +++
>  test/box/function1.test.lua |  1 +
>  9 files changed, 81 insertions(+), 68 deletions(-)

Generally, this patch is fine. See a few very minor comments below.

> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index a37a68ce4..18b50a5da 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2574,24 +2574,11 @@ func_def_new_from_tuple(struct tuple *tuple)
>  
>  /** Remove a function from function cache */
>  static void
> -func_cache_remove_func(struct trigger * /* trigger */, void *event)
> +func_cache_remove_func(struct trigger *trigger, void * /* event */)
>  {
> -	struct txn_stmt *stmt = txn_last_stmt((struct txn *) event);
> -	uint32_t fid = tuple_field_u32_xc(stmt->old_tuple ?
> -				       stmt->old_tuple : stmt->new_tuple,
> -				       BOX_FUNC_FIELD_ID);
> -	func_cache_delete(fid);
> -}
> -
> -/** Replace a function in the function cache */
> -static void
> -func_cache_replace_func(struct trigger * /* trigger */, void *event)
> -{
> -	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
> -	struct func_def *def = func_def_new_from_tuple(stmt->new_tuple);
> -	auto def_guard = make_scoped_guard([=] { free(def); });
> -	func_cache_replace(def);
> -	def_guard.is_active = false;
> +	struct func *old_func = (struct func *) trigger->data;
> +	func_cache_delete(old_func->def->fid);
> +	func_delete(old_func);
>  }
>  
>  /**
> @@ -2612,13 +2599,21 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
>  	struct func *old_func = func_by_id(fid);
>  	if (new_tuple != NULL && old_func == NULL) { /* INSERT */
>  		struct func_def *def = func_def_new_from_tuple(new_tuple);
> +		auto def_guard = make_scoped_guard([=] { free(def); });
>  		access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION,
>  				 PRIV_C);
> -		auto def_guard = make_scoped_guard([=] { free(def); });
> -		func_cache_replace(def);
> +		struct func *func = func_new(def);
> +		if (func == NULL)
> +			diag_raise();
> +		auto func_guard = make_scoped_guard([=] {
> +			func_cache_delete(def->fid);
> +			func_delete(func);
> +		});

Nit: I would make func_cache_insert exception-free (see my comment
below), and move on_rollback trigger allocation upper. This way we
wouldn't need the func_guard at all.

>  		def_guard.is_active = false;
> +		func_cache_insert(func);
> +		func_guard.is_active = false;
>  		struct trigger *on_rollback =
> -			txn_alter_trigger_new(func_cache_remove_func, NULL);
> +			txn_alter_trigger_new(func_cache_remove_func, func);
>  		txn_on_rollback(txn, on_rollback);
>  	} else if (new_tuple == NULL) {         /* DELETE */
>  		uint32_t uid;
> @@ -2636,16 +2631,22 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
>  				  "function has grants");
>  		}
>  		struct trigger *on_commit =
> -			txn_alter_trigger_new(func_cache_remove_func, NULL);
> +			txn_alter_trigger_new(func_cache_remove_func, old_func);
>  		txn_on_commit(txn, on_commit);
>  	} else {                                /* UPDATE, REPLACE */
> -		struct func_def *def = func_def_new_from_tuple(new_tuple);
> -		auto def_guard = make_scoped_guard([=] { free(def); });
> -		access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION,
> -				 PRIV_A);
> -		struct trigger *on_commit =
> -			txn_alter_trigger_new(func_cache_replace_func, NULL);
> -		txn_on_commit(txn, on_commit);
> +		assert(new_tuple != NULL && old_tuple != NULL);
> +		/**
> +		 * Allow an alter that doesn't change the
> +		 * definition to support upgrade script.
> +		 */
> +		struct func_def *new_def = func_def_new_from_tuple(new_tuple);
> +		auto new_def_guard = make_scoped_guard([=] { free(new_def); });
> +		struct func_def *old_def = func_def_new_from_tuple(old_tuple);
> +		auto old_def_guard = make_scoped_guard([=] { free(old_def); });

Nit: one guard should be enough, I think:

		struct func_def *old_def = NULL, *new_def = NULL;
		auto guard = make_scoped_guard([=] {
			free(old_def);
			free(new_def);
		});
		old_def = func_def_new_from_tuple(old_tuple);
		new_def = func_def_new_from_tuple(new_tuple);

> +		if (func_def_cmp(new_def, old_def) != 0) {
> +			tnt_raise(ClientError, ER_UNSUPPORTED, "function",
> +				  "alter");
> +		}
>  	}
>  }
>  
> diff --git a/src/box/func.c b/src/box/func.c
> index a817851fd..d1b8879ad 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -451,14 +451,6 @@ func_call(struct func *func, box_function_ctx_t *ctx, const char *args,
>  	return rc;
>  }
>  
> -void
> -func_update(struct func *func, struct func_def *def)
> -{
> -	func_unload(func);
> -	free(func->def);
> -	func->def = def;
> -}
> -
>  void
>  func_delete(struct func *func)
>  {
> diff --git a/src/box/func.h b/src/box/func.h
> index 8dcd61d7b..fa4d738ab 100644
> --- a/src/box/func.h
> +++ b/src/box/func.h
> @@ -100,9 +100,6 @@ module_free(void);
>  struct func *
>  func_new(struct func_def *def);
>  
> -void
> -func_update(struct func *func, struct func_def *def);
> -
>  void
>  func_delete(struct func *func);
>  
> diff --git a/src/box/func_def.c b/src/box/func_def.c
> index 76ed77b24..2b135e2d7 100644
> --- a/src/box/func_def.c
> +++ b/src/box/func_def.c
> @@ -1,3 +1,20 @@
>  #include "func_def.h"
> +#include "string.h"
>  
>  const char *func_language_strs[] = {"LUA", "C"};
> +
> +int
> +func_def_cmp(struct func_def *def1, struct func_def *def2)
> +{
> +	if (def1->fid != def2->fid)
> +		return def1->fid - def2->fid;
> +	if (def1->uid != def2->uid)
> +		return def1->uid - def2->uid;
> +	if (def1->setuid != def2->setuid)
> +		return def1->setuid - def2->setuid;
> +	if (def1->language != def2->language)
> +		return def1->language - def2->language;
> +	if (strcmp(def1->name, def2->name) != 0)
> +		return strcmp(def1->name, def2->name);
> +	return 0;
> +}
> diff --git a/src/box/func_def.h b/src/box/func_def.h
> index 5b52ab498..4c9738aea 100644
> --- a/src/box/func_def.h
> +++ b/src/box/func_def.h
> @@ -34,6 +34,10 @@
>  #include "trivia/util.h"
>  #include <stdbool.h>
>  
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
>  /**
>   * The supported language of the stored function.
>   */
> @@ -79,6 +83,10 @@ func_def_sizeof(uint32_t name_len)
>  	return sizeof(struct func_def) + name_len + 1;
>  }
>  
> +/** Compare two given function definitions. */
> +int
> +func_def_cmp(struct func_def *def1, struct func_def *def2);
> +
>  /**
>   * API of C stored function.
>   */
> @@ -86,4 +94,8 @@ typedef struct box_function_ctx box_function_ctx_t;
>  typedef int (*box_function_f)(box_function_ctx_t *ctx,
>  	     const char *args, const char *args_end);
>  
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif /* TARANTOOL_BOX_FUNC_DEF_H_INCLUDED */
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 6d3153815..f834610a5 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -541,40 +541,32 @@ 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);

Nit: let's also move this check to func_def_new_from_tuple.
This would consistent with space_def_new_from_tuple and it
would make func_cache_insert protocol more straighforward -
panic on OOM, otherwise exception-free.

> -	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 };
> -	mh_int_t k1 = mh_i32ptr_put(funcs, &node, NULL, NULL);
> -	if (k1 == mh_end(funcs)) {
> -		func->def = NULL;
> -		func_delete(func);
> -		goto error;
> -	}
> +
> +	mh_int_t k1, k2;
> +	struct mh_i32ptr_node_t old_node, *old_node_ptr = &old_node;
> +	const struct mh_i32ptr_node_t node = { func->def->fid, func };

Nit: I wouldn't change this code. Instead I would add to the beginning
of this function:

	assert(func_by_id(func->def->id) == NULL);
	assert(func_by_name(func->def->name, func->def->name_len) == NULL);

>  	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);
> +	k1 = mh_i32ptr_put(funcs, &node, &old_node_ptr, NULL);
> +	if (k1 == mh_end(funcs))
> +		goto error;
> +	assert(old_node_ptr == NULL);
> +	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);
>  		goto error;
>  	}
> +	return;
> +error:
> +	panic_syserror("Out of memory for the data dictionary cache "
> +		       "(stored function).");
>  }
>  
>  void
> @@ -590,7 +582,6 @@ func_cache_delete(uint32_t fid)
>  				strlen(func->def->name));
>  	if (k != mh_end(funcs))
>  		mh_strnptr_del(funcs_by_name, k, NULL);
> -	func_delete(func);
>  }
>  
>  struct func *
> diff --git a/src/box/schema.h b/src/box/schema.h
> index 6f9a96117..d7b3c4c2d 100644
> --- a/src/box/schema.h
> +++ b/src/box/schema.h
> @@ -163,14 +163,12 @@ schema_free();
>  struct space *schema_space(uint32_t id);
>  
>  /**
> - * Insert a new function or update the old one.
> - *
> - * @param def Function definition. In a case of success the ownership
> - *        of @a def is transfered to the data dictionary, thus the caller
> - *        must not delete it.
> + * Insert a new function object in the function cache.
> + * @param func Function object to insert.
> + * @retval Returns 0 on success, -1 otherwise.

Nit: there's no return value :-)

It would be nice to mention that there must not be a function with
the same name or id present in the cache.

>   */
>  void
> -func_cache_replace(struct func_def *def);
> +func_cache_insert(struct func *func);
>  
>  void
>  func_cache_delete(uint32_t fid);
> diff --git a/test/box/function1.result b/test/box/function1.result
> index 5b04fa97d..cadeb0467 100644
> --- a/test/box/function1.result
> +++ b/test/box/function1.result
> @@ -16,6 +16,10 @@ c = net.connect(os.getenv("LISTEN"))
>  box.schema.func.create('function1', {language = "C"})
>  ---
>  ...
> +box.space._func:replace{2, 1, 'function1', 0, 'LUA'}
> +---
> +- error: function does not support alter
> +...
>  box.schema.user.grant('guest', 'execute', 'function', 'function1')
>  ---
>  ...
> diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
> index 855dd5382..e983495b6 100644
> --- a/test/box/function1.test.lua
> +++ b/test/box/function1.test.lua
> @@ -7,6 +7,7 @@ net = require('net.box')
>  c = net.connect(os.getenv("LISTEN"))
>  
>  box.schema.func.create('function1', {language = "C"})
> +box.space._func:replace{2, 1, 'function1', 0, 'LUA'}
>  box.schema.user.grant('guest', 'execute', 'function', 'function1')
>  _ = box.schema.space.create('test')
>  _ = box.space.test:create_index('primary')