[Tarantool-patches] [PATCH v1 1/1] sql: remove SQL built-in functions from _func

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 11 comments below.

On 30.07.2021 00:48, imeevma at tarantool.org wrote:
> This patch removes SQL built-in functions from _func. These functions
> could be called directly from Lua, however all they did was returnd an

1. returnd -> returned.

> error. After this patch, no SQL built-in functions can be called
> directly from LUA.
> 
> Closes #6106
> ---
> https://github.com/tarantool/tarantool/issues/6106
> https://github.com/tarantool/tarantool/tree/imeevma/gh-6106-remove-sql-builtins-from-_func
> > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 97afc0b4d..d0ca3db97 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -1003,19 +1003,19 @@ end
>  --------------------------------------------------------------------------------
>  -- Tarantool 2.9.1
>  --------------------------------------------------------------------------------
> -local function sql_builtin_function_uuid()
> +local function remove_sql_builtin_functions_from_func()
>      local _func = box.space._func
>      local _priv = box.space._priv
> -    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> -    local t = _func:auto_increment({ADMIN, 'UUID', 1, 'SQL_BUILTIN', '',
> -                                    'function', {}, 'any', 'none', 'none',
> -                                    false, false, true, {}, setmap({}), '',
> -                                    datetime, datetime})
> -    _priv:replace{ADMIN, PUBLIC, 'function', t.id, box.priv.X}
> +    for _, v in _func:pairs() do
> +        if (v.language == "SQL_BUILTIN") then

2. You do not need () around the condition.

> +            _priv:delete({2, 'function', v.id})
> +            _func:delete({v.id})
> +        end
> +    end
>  end
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 3772596d6..7c1616a3a 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -326,11 +326,13 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
>  			break;
>  		}
>  		if (op == TK_FUNCTION) {
> -			uint32_t arg_count = p->x.pList == NULL ? 0 :
> -					     p->x.pList->nExpr;
> +			int arg_count = p->x.pList == NULL ? 0 :
> +					p->x.pList->nExpr;
>  			struct func *func =
> -				sql_func_by_signature(p->u.zToken, arg_count);
> -			if (func == NULL)
> +				sql_func_by_name(p->u.zToken);
> +			if (func == NULL || !func->def->exports.sql ||

3. Why don't you filter by !func->def->exports.sql in sql_func_by_name()?
Is it possible in some places to call a function not exported to SQL?

> +			    (func->def->param_count != -1 &&
> +			     func->def->param_count != arg_count))

4. Why is argument count important? Isn't SQL_FUNC_DERIVEDCOLL below
enough?

The same 2 comments in the next diff block.

>  				break;
>  			if (sql_func_flag_is_set(func, SQL_FUNC_DERIVEDCOLL) &&
>  			    arg_count > 0) {
> @@ -3979,8 +3981,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>  			nFarg = pFarg ? pFarg->nExpr : 0;
>  			assert(!ExprHasProperty(pExpr, EP_IntValue));
>  			zId = pExpr->u.zToken;
> -			struct func *func = sql_func_by_signature(zId, nFarg);
> -			if (func == NULL) {
> +			struct func *func = sql_func_by_name(zId);
> +			if (func == NULL || !func->def->exports.sql ||
> +			    (func->def->param_count != -1 &&
> +			     func->def->param_count != nFarg)) {
>  				diag_set(ClientError, ER_NO_SUCH_FUNCTION,
>  					 zId);
>  				pParse->is_aborted = true;
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 6ca852dec..b05038e2e 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -47,6 +47,37 @@
>  #include <unicode/ucol.h>
>  #include "box/coll_id_cache.h"
>  #include "box/schema.h"
> +#include "core/assoc.h"

5. core/ prefix is not not necessary. Also it might look better if
you group all box/ includes together.

> +#include "box/user.h"
> +
> +static struct mh_strnptr_t *built_in_functions = NULL;
> +
> +static struct func *
> +get_built_in_function(const char *name)
> +{
> +	uint32_t len = strlen(name);
> +	mh_int_t func = mh_strnptr_find_inp(built_in_functions, name, len);
> +	if (func == mh_end(built_in_functions))
> +		return NULL;
> +	return (struct func *) mh_strnptr_node(built_in_functions, func)->val;

6. In the new code we do not use whitespaces after unary
operators like cast. But even better - in C you do not need
an explicit cast from void * to any other pointer.

> +}
> +
> +static int
> +built_in_functions_cache_insert(struct func *func)

7. Please, make the hash methods have the same prefix. For instance,
sql_builtin_func_get(), sql_builtin_func_put().

> +{
> +	const char *name = func->def->name;
> +	uint32_t len = strlen(name);
> +	assert(get_built_in_function(name) == NULL);
> +
> +	uint32_t hash = mh_strn_hash(name, len);
> +	const struct mh_strnptr_node_t strnode = {name, len, hash, func};
> +	mh_int_t k = mh_strnptr_put(built_in_functions, &strnode, NULL, NULL);
> +	if (k == mh_end(built_in_functions)) {
> +		diag_set(OutOfMemory, sizeof(strnode), "malloc", "strnode");
> +		return -1;
> +	}
> +	return 0;
> +}
> @@ -2657,6 +2685,67 @@ static struct {
>  
>  static struct func_vtab func_sql_builtin_vtab;
>  
> +int
> +sql_built_in_functions_cache_init(void)
> +{
> +	built_in_functions = mh_strnptr_new();

8. It might make sense to check the result for NULL and call
panic() if it is.

> +	for (uint32_t i = 0; i < nelem(sql_builtins); ++i) {
> +		const char *name = sql_builtins[i].name;
> +		uint32_t len = strlen(name);
> +		uint32_t size = sizeof(struct func_def) + len + 1;
> +		struct func_def *def = malloc(size);

9. Ditto.

> +		def->fid = i;
> +		def->uid = 1;
> +		def->body = NULL;
> +		def->comment = NULL;
> +		def->setuid = true;
> +		def->is_deterministic = sql_builtins[i].is_deterministic;
> +		def->is_sandboxed = false;
> +		def->param_count = sql_builtins[i].param_count;
> +		def->returns = sql_builtins[i].returns;
> +		def->aggregate = sql_builtins[i].aggregate;
> +		def->language = FUNC_LANGUAGE_SQL_BUILTIN;
> +		def->name_len = len;
> +		def->exports.sql = sql_builtins[i].export_to_sql;
> +		func_opts_create(&def->opts);
> +		memcpy(def->name, name, len + 1);
> +
> +		struct func_sql_builtin *func = malloc(sizeof(*func));
> +		if (func == NULL) {
> +			diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
> +			return -1;

10. The result of sql_built_in_functions_cache_init() is never checked.
I would suggest to make it void and panic on all errors.

> +		}
> +
> +		func->base.def = def;
> +		func->base.vtab = &func_sql_builtin_vtab;
> +		credentials_create_empty(&func->base.owner_credentials);
> +		memset(func->base.access, 0, sizeof(func->base.access));
> +
> +		func->flags = sql_builtins[i].flags;
> +		func->call = sql_builtins[i].call;
> +		func->finalize = sql_builtins[i].finalize;
> +		if (built_in_functions_cache_insert(&func->base) != 0)
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +void
> +sql_built_in_functions_cache_free(void)
> +{
> +	if (built_in_functions == NULL)
> +		return;
> +	for (uint32_t i = 0; i < nelem(sql_builtins); ++i) {
> +		const char *name = sql_builtins[i].name;
> +		uint32_t len = strlen(name);
> +		mh_int_t k = mh_strnptr_find_inp(built_in_functions, name, len);
> +		if (k == mh_end(built_in_functions))
> +			continue;
> +		mh_strnptr_del(built_in_functions, k, NULL);

11. Perhaps a more 'canonical' way would be like

	while (mh_size() > 0) { mh_del(mh_first()); ... }

Also you didn't free the function objects.

> +	}
> +	mh_strnptr_delete(built_in_functions);
> +}