Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func
@ 2021-08-04 12:58 Mergen Imeev via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 1/6] sql: introduce sql_func_flags() Mergen Imeev via Tarantool-patches
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-04 12:58 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch-set removes SQL built-in functions from _func and prohibits functions
with SQL_BUILTIN engine to be decribed in _func system space.

Changes in v2:
 - Added some functions that simplifies work with SQL built-in functions.
 - Removed some code that become unused due to removal of SQL built-in functions
   from _func.
 - Prohibited to insert tuples with "language" = 'SQL_BUILTIN' to _func.

Mergen Imeev (5):
  sql: introduce sql_func_flags()
  sql: introduce sql_func_find()
  sql: remove SQL built-in functions from _func
  alter: disallow creation of SQL built-in function
  sql: remove unnecessary function initialization

Vladislav Shpilevoy (1):
  alter: parse data dictionary version

 ...gh-6106-remove-sql-built-ins-from-_func.md |   7 +
 src/box/alter.cc                              |  63 +++++-
 src/box/bootstrap.snap                        | Bin 6016 -> 4891 bytes
 src/box/box.cc                                |   1 +
 src/box/func.c                                |   7 -
 src/box/lua/upgrade.lua                       |  16 +-
 src/box/schema.cc                             |   4 +
 src/box/schema.h                              |   1 +
 src/box/sql.c                                 |   1 +
 src/box/sql.h                                 |   9 +
 src/box/sql/analyze.c                         |  12 ++
 src/box/sql/expr.c                            |  23 +--
 src/box/sql/func.c                            | 189 +++++++++++++-----
 src/box/sql/resolve.c                         |  22 +-
 src/box/sql/sqlInt.h                          |  20 +-
 src/box/sql/vdbemem.c                         |   2 +-
 test/box-py/bootstrap.result                  |  66 ------
 test/box/access_bin.result                    |   4 +-
 test/box/access_bin.test.lua                  |   4 +-
 test/box/access_sysview.result                |   8 +-
 test/box/function1.result                     |  39 ++--
 test/box/function1.test.lua                   |  16 +-
 test/sql-tap/func5.test.lua                   |  57 +++++-
 test/wal_off/func_max.result                  |   8 +-
 24 files changed, 339 insertions(+), 240 deletions(-)
 create mode 100644 changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md

-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Tarantool-patches] [PATCH v2 1/6] sql: introduce sql_func_flags()
  2021-08-04 12:58 [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
@ 2021-08-04 12:58 ` Mergen Imeev via Tarantool-patches
  2021-08-05 22:14   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 2/6] sql: introduce sql_func_find() Mergen Imeev via Tarantool-patches
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-04 12:58 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This function returns a set of parameters for the function with the
given name. This function is used when we do not need to call a
function, but we need its parameters.

In addition, this function will allow us to split the parameters
between those that are the same for all implementations, and the
parameters, the value of which is implementation-dependent.

Needed for #6105
Part of #6106
---
 src/box/sql/expr.c   |  9 +++------
 src/box/sql/func.c   | 12 ++++++++++++
 src/box/sql/sqlInt.h | 10 ++++++++++
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3772596d6..0ba42ed71 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -328,11 +328,8 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
 		if (op == TK_FUNCTION) {
 			uint32_t 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)
-				break;
-			if (sql_func_flag_is_set(func, SQL_FUNC_DERIVEDCOLL) &&
+			uint32_t flags = sql_func_flags(p->u.zToken);
+			if (((flags & SQL_FUNC_DERIVEDCOLL) != 0) &&
 			    arg_count > 0) {
 				/*
 				 * Now we use quite straightforward
@@ -342,7 +339,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
 				 * built-in functions: trim, upper,
 				 * lower, replace, substr.
 				 */
-				assert(func->def->returns == FIELD_TYPE_STRING);
+				assert(p->type == FIELD_TYPE_STRING);
 				p = p->x.pList->a->pExpr;
 				continue;
 			}
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 6ca852dec..28d383293 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -2655,6 +2655,18 @@ static struct {
 	},
 };
 
+uint32_t
+sql_func_flags(const char *name)
+{
+	struct func *func = func_by_name(name, strlen(name));
+	if (func == NULL || func->def->language != FUNC_LANGUAGE_SQL_BUILTIN)
+		return 0;
+	uint32_t flags = ((struct func_sql_builtin *)func)->flags;
+	if (func->def->aggregate == FUNC_AGGREGATE_GROUP)
+		flags |= SQL_FUNC_AGG;
+	return flags;
+}
+
 static struct func_vtab func_sql_builtin_vtab;
 
 struct func *
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 115c52f96..97b7a2401 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1186,6 +1186,8 @@ struct type_def {
  *     SQL_FUNC_LENGTH    ==  OPFLAG_LENGTHARG
  *     SQL_FUNC_TYPEOF    ==  OPFLAG_TYPEOFARG
  */
+/** Function is one of aggregate functions. */
+#define SQL_FUNC_AGG		0x0001
 #define SQL_FUNC_LIKE     0x0004	/* Candidate for the LIKE optimization */
 #define SQL_FUNC_NEEDCOLL 0x0020	/* sqlGetFuncCollSeq() might be called.
 					 * The flag is set when the collation
@@ -4372,6 +4374,14 @@ sql_func_flag_is_set(struct func *func, uint16_t flag)
 struct func *
 sql_func_by_signature(const char *name, int argc);
 
+/**
+ * Return the parameters of the function with the given name. If the function
+ * with the given name does not exist, or the function is not a built-in SQL
+ * function, 0 is returned, which means no parameters have been set.
+ */
+uint32_t
+sql_func_flags(const char *name);
+
 /**
  * Generate VDBE code to halt execution with correct error if
  * the object with specified key is already present (or doesn't
-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Tarantool-patches] [PATCH v2 2/6] sql: introduce sql_func_find()
  2021-08-04 12:58 [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 1/6] sql: introduce sql_func_flags() Mergen Imeev via Tarantool-patches
@ 2021-08-04 12:58 ` Mergen Imeev via Tarantool-patches
  2021-08-05 22:15   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 3/6] sql: remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-04 12:58 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch introduces the sql_func_find() function. This function allows
us to centralize the look up of functions during parsing, which
simplifies code and fixes some incorrect error messages.

Part of #6106
---
 src/box/sql/analyze.c       | 12 ++++++++++++
 src/box/sql/expr.c          | 14 ++------------
 src/box/sql/func.c          | 38 ++++++++++++++++++++++++-------------
 src/box/sql/resolve.c       | 22 +--------------------
 src/box/sql/sqlInt.h        | 12 ++----------
 src/box/sql/vdbemem.c       |  2 +-
 test/sql-tap/func5.test.lua | 34 ++++++++++++++++++++++++++++++++-
 7 files changed, 76 insertions(+), 58 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index b87f69512..afa2331a1 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -719,6 +719,10 @@ callStatGet(Vdbe * v, int regStat4, int iParam, int regOut)
 {
 	assert(regOut != regStat4 && regOut != regStat4 + 1);
 	sqlVdbeAddOp2(v, OP_Integer, iParam, regStat4 + 1);
+	/*
+	 * Function sql_func_by_signature() was removed, so after enabling this
+	 * part should be changed.
+	 */
 	struct func *func = sql_func_by_signature("_sql_stat_get", 2);
 	assert(func != NULL);
 	sqlVdbeAddOp4(v, OP_BuiltinFunction0, 0, regStat4, regOut,
@@ -858,6 +862,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		sqlVdbeAddOp2(v, OP_Count, idx_cursor, stat4_reg + 3);
 		sqlVdbeAddOp2(v, OP_Integer, part_count, stat4_reg + 1);
 		sqlVdbeAddOp2(v, OP_Integer, part_count, stat4_reg + 2);
+		/*
+		 * Function sql_func_by_signature() was removed, so after
+		 * enabling this part should be changed.
+		 */
 		struct func *init_func =
 			sql_func_by_signature("_sql_stat_init", 3);
 		assert(init_func != NULL);
@@ -959,6 +967,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		sqlVdbeAddOp3(v, OP_MakeRecord, stat_key_reg,
 				  pk_part_count, key_reg);
 		assert(chng_reg == (stat4_reg + 1));
+		/*
+		 * Function sql_func_by_signature() was removed, so after
+		 * enabling this part should be changed.
+		 */
 		struct func *push_func =
 			sql_func_by_signature("_sql_stat_push", 3);
 		assert(push_func != NULL);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 0ba42ed71..e179f7ad1 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3962,7 +3962,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 	case TK_FUNCTION:{
 			ExprList *pFarg;	/* List of function arguments */
 			int nFarg;	/* Number of function arguments */
-			const char *zId;	/* The function name */
 			u32 constMask = 0;	/* Mask of function arguments that are constant */
 			int i;	/* Loop counter */
 			struct coll *coll = NULL;
@@ -3975,11 +3974,8 @@ 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);
+			struct func *func = sql_func_find(pExpr);
 			if (func == NULL) {
-				diag_set(ClientError, ER_NO_SUCH_FUNCTION,
-					 zId);
 				pParse->is_aborted = true;
 				break;
 			}
@@ -5444,14 +5440,8 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr)
 						pItem->iMem = ++pParse->nMem;
 						assert(!ExprHasProperty
 						       (pExpr, EP_IntValue));
-						const char *name =
-							pExpr->u.zToken;
-						uint32_t argc =
-							pExpr->x.pList != NULL ?
-							pExpr->x.pList->nExpr : 0;
 						pItem->func =
-							sql_func_by_signature(
-								name, argc);
+							sql_func_find(pExpr);
 						assert(pItem->func != NULL);
 						assert(pItem->func->def->
 						       language ==
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 28d383293..9514d070a 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1934,24 +1934,12 @@ sql_is_like_func(struct Expr *expr)
 	    expr->x.pList->nExpr != 2)
 		return 0;
 	assert(!ExprHasProperty(expr, EP_xIsSelect));
-	struct func *func = sql_func_by_signature(expr->u.zToken, 2);
+	struct func *func = sql_func_find(expr);
 	if (func == NULL || !sql_func_flag_is_set(func, SQL_FUNC_LIKE))
 		return 0;
 	return 1;
 }
 
-struct func *
-sql_func_by_signature(const char *name, int argc)
-{
-	struct func *base = func_by_name(name, strlen(name));
-	if (base == NULL || !base->def->exports.sql)
-		return NULL;
-
-	if (base->def->param_count != -1 && base->def->param_count != argc)
-		return NULL;
-	return base;
-}
-
 static int
 func_sql_builtin_call_stub(struct func *func, struct port *args,
 			   struct port *ret)
@@ -2655,6 +2643,30 @@ static struct {
 	},
 };
 
+struct func *
+sql_func_find(struct Expr *expr)
+{
+	const char *name = expr->u.zToken;
+	struct func *func = func_by_name(name, strlen(name));
+	if (func == NULL) {
+		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
+		return NULL;
+	}
+	if (!func->def->exports.sql) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+			 tt_sprintf("function %s() is not available in SQL",
+				     name));
+		return NULL;
+	}
+	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
+	if (func->def->param_count != -1 && func->def->param_count != n) {
+		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
+			 tt_sprintf("%d", func->def->param_count), n);
+		return NULL;
+	}
+	return func;
+}
+
 uint32_t
 sql_func_flags(const char *name)
 {
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 11b6139e3..35faddab5 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -598,28 +598,8 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
 			assert(!ExprHasProperty(pExpr, EP_xIsSelect));
 			zId = pExpr->u.zToken;
 			nId = sqlStrlen30(zId);
-			struct func *func = func_by_name(zId, nId);
+			struct func *func = sql_func_find(pExpr);
 			if (func == NULL) {
-				diag_set(ClientError, ER_NO_SUCH_FUNCTION, zId);
-				pParse->is_aborted = true;
-				pNC->nErr++;
-				return WRC_Abort;
-			}
-			if (!func->def->exports.sql) {
-				diag_set(ClientError, ER_SQL_PARSER_GENERIC,
-					 tt_sprintf("function %.*s() is not "
-						    "available in SQL",
-						     nId, zId));
-				pParse->is_aborted = true;
-				pNC->nErr++;
-				return WRC_Abort;
-			}
-			if (func->def->param_count != -1 &&
-			    func->def->param_count != n) {
-				uint32_t argc = func->def->param_count;
-				const char *err = tt_sprintf("%d", argc);
-				diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
-					 func->def->name, err, n);
 				pParse->is_aborted = true;
 				pNC->nErr++;
 				return WRC_Abort;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 97b7a2401..92281a530 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4362,17 +4362,9 @@ sql_func_flag_is_set(struct func *func, uint16_t flag)
 	return (((struct func_sql_builtin *)func)->flags & flag) != 0;
 }
 
-/**
- * A SQL method to find a function in a hash by its name and
- * count of arguments. Only functions that have 'SQL' engine
- * export field set true and have exactly the same signature
- * are returned.
- *
- * Returns not NULL function pointer when a valid and exported
- * to SQL engine function is found and NULL otherwise.
- */
+/** Return a function that matches the parameters described in given expr. */
 struct func *
-sql_func_by_signature(const char *name, int argc);
+sql_func_find(struct Expr *expr);
 
 /**
  * Return the parameters of the function with the given name. If the function
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 2c5099616..499089c8d 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -148,7 +148,7 @@ valueFromFunction(sql * db,	/* The database connection */
 	pList = p->x.pList;
 	if (pList)
 		nVal = pList->nExpr;
-	struct func *func = sql_func_by_signature(p->u.zToken, nVal);
+	struct func *func = sql_func_find(p);
 	if (func == NULL || func->def->language != FUNC_LANGUAGE_SQL_BUILTIN ||
 	    !func->def->is_deterministic ||
 	    sql_func_flag_is_set(func, SQL_FUNC_NEEDCOLL))
diff --git a/test/sql-tap/func5.test.lua b/test/sql-tap/func5.test.lua
index 9b1526aaf..13698582b 100755
--- a/test/sql-tap/func5.test.lua
+++ b/test/sql-tap/func5.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(25)
+test:plan(27)
 
 --!./tcltestrunner.lua
 -- 2010 August 27
@@ -314,4 +314,36 @@ test:do_execsql_test(
 box.func.COUNTER1:drop()
 box.func.COUNTER2:drop()
 
+--
+-- Make sure the correct error is displayed if the function throws an error when
+-- setting the default value.
+--
+local body = 'function(x) return 1 end'
+box.schema.func.create('F1', {language = 'Lua', returns = 'number', body = body,
+                       param_list = {}, exports = {'LUA'}});
+box.execute([[CREATE TABLE t01(i INT PRIMARY KEY, a INT DEFAULT(f1(1)));]])
+test:do_catchsql_test(
+    "func-7.1",
+    [[
+        INSERT INTO t01(i) VALUES(1);
+    ]], {
+        1, "function F1() is not available in SQL"
+    })
+
+box.schema.func.create('F2', {language = 'Lua', returns = 'number', body = body,
+                       exports = {'LUA', 'SQL'}});
+box.execute([[CREATE TABLE t02(i INT PRIMARY KEY, a INT DEFAULT(f2(1)));]])
+test:do_catchsql_test(
+    "func-7.2",
+    [[
+        INSERT INTO t02(i) VALUES(1);
+    ]], {
+        1, "Wrong number of arguments is passed to F2(): expected 0, got 1"
+    })
+
+box.func.F1:drop()
+box.func.F2:drop()
+box.space.T01:drop()
+box.space.T02:drop()
+
 test:finish_test()
-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Tarantool-patches] [PATCH v2 3/6] sql: remove SQL built-in functions from _func
  2021-08-04 12:58 [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 1/6] sql: introduce sql_func_flags() Mergen Imeev via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 2/6] sql: introduce sql_func_find() Mergen Imeev via Tarantool-patches
@ 2021-08-04 12:58 ` Mergen Imeev via Tarantool-patches
  2021-08-05 22:17   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 4/6] alter: parse data dictionary version Mergen Imeev via Tarantool-patches
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-04 12:58 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! I did not include diff since there were too many
changes and part of them were moved to the other patches. My answers and new
patch below.

On 02.08.2021 23:12, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> See 11 comments below.
>
> On 30.07.2021 00:48, imeevma@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.
>
Fixed.

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

>> +            _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?
>
Done, I moved all similar checks to new function sql_func_find().

>> +			    (func->def->param_count != -1 &&
>> +			     func->def->param_count != arg_count))
>
> 4. Why is argument count important? Isn't SQL_FUNC_DERIVEDCOLL below
> enough?
>
Fixed. I think here it is not important. Also, in some cases we only need
function flags without function itself. I used this place to introduce it, but
its real usage will be later, during addition of static type checking for SQL
built-in functions.

> The same 2 comments in the next diff block.
>
In these two places and one place in resolve.c I used new function, that have
all needed checks inside of it.

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

>> +#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.
>
Thanks, fixed.
>> +}
>> +
>> +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().
>
Thanks, replaced to suggested names.

>> +{
>> +	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.
>
Fixed.

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

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

>> +		}
>> +
>> +		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()); ... }
>
I believe it cannot be implemented on hash table with string key. Added an
assert to check that hash table is empty.

> Also you didn't free the function objects.
>
Fixed.

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



New patch:

commit cd575f0326c6276a0b44470532acff22eb97f694
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Jul 29 13:54:53 2021 +0300

    sql: remove SQL built-in functions from _func
    
    This patch removes SQL built-in functions from _func. These functions
    could be called directly from Lua, however all they did was returned an
    error. After this patch, no SQL built-in functions can be called
    directly from LUA.
    
    Part of #6106

diff --git a/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
new file mode 100644
index 000000000..88b458540
--- /dev/null
+++ b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
@@ -0,0 +1,7 @@
+## feature/sql
+
+* SQL built-in functions were removed from \_func system space (gh-6106).
+* After this patch, functions will be looked up first in SQL built-in functions
+  and then in user-defined functions.
+* Fixed incorrect error message in case of misuse of the function used to set
+  the default value.
\ No newline at end of file
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 390199298..935790df4 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3548,13 +3548,6 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 				  "function has references");
 			return -1;
 		}
-		/* Can't' drop a builtin function. */
-		if (old_func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
-			diag_set(ClientError, ER_DROP_FUNCTION,
-				  (unsigned) old_func->def->uid,
-				  "function is SQL built-in");
-			return -1;
-		}
 		struct trigger *on_commit =
 			txn_alter_trigger_new(on_drop_func_commit, old_func);
 		struct trigger *on_rollback =
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 57374decc..018670d2a 100644
Binary files a/src/box/bootstrap.snap and b/src/box/bootstrap.snap differ
diff --git a/src/box/box.cc b/src/box/box.cc
index 0ff7ee468..a7eda4214 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2914,6 +2914,7 @@ box_free(void)
 		gc_free();
 		engine_shutdown();
 		wal_free();
+		sql_built_in_functions_cache_free();
 	}
 }
 
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 97afc0b4d..6abce50f4 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
+            _priv:delete({2, 'function', v.id})
+            _func:delete({v.id})
+        end
+    end
 end
 
 local function upgrade_to_2_9_1()
-    sql_builtin_function_uuid()
+    remove_sql_builtin_functions_from_func()
 end
 
 --------------------------------------------------------------------------------
diff --git a/src/box/sql.c b/src/box/sql.c
index 433264abe..f18dfa063 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -75,6 +75,7 @@ sql_init(void)
 		panic("failed to initialize SQL subsystem");
 
 	sql_stmt_cache_init();
+	sql_built_in_functions_cache_init();
 
 	assert(db != NULL);
 }
diff --git a/src/box/sql.h b/src/box/sql.h
index 4c364306c..2ac97c762 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -56,6 +56,15 @@ sql_init(void);
 struct sql *
 sql_get(void);
 
+/** Initialize global cache for built-in functions. */
+void
+sql_built_in_functions_cache_init(void);
+
+/** Free global cache for built-in functions. */
+void
+sql_built_in_functions_cache_free(void);
+
+
 struct Expr;
 struct Parse;
 struct Select;
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9514d070a..2a3a5d457 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -47,6 +47,10 @@
 #include <unicode/ucol.h>
 #include "box/coll_id_cache.h"
 #include "box/schema.h"
+#include "box/user.h"
+#include "assoc.h"
+
+static struct mh_strnptr_t *built_in_functions = NULL;
 
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
@@ -2643,11 +2647,49 @@ static struct {
 	},
 };
 
+static struct func *
+built_in_func_get(const char *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))
+		return NULL;
+	return mh_strnptr_node(built_in_functions, k)->val;
+}
+
+static void
+built_in_func_put(struct func *func)
+{
+	const char *name = func->def->name;
+	uint32_t len = strlen(name);
+	assert(built_in_func_get(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)) {
+		panic("Out of memory on insertion into SQL built-in functions "
+		      "hash");
+	}
+}
+
 struct func *
 sql_func_find(struct Expr *expr)
 {
 	const char *name = expr->u.zToken;
-	struct func *func = func_by_name(name, strlen(name));
+	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
+	struct func *func = built_in_func_get(name);
+	if (func != NULL) {
+		assert(func->def->exports.sql);
+		int param_count = func->def->param_count;
+		if (param_count != -1 && param_count != n) {
+			diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
+				 tt_sprintf("%d", func->def->param_count), n);
+			return NULL;
+		}
+		return func;
+	}
+	func = func_by_name(name, strlen(name));
 	if (func == NULL) {
 		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
 		return NULL;
@@ -2658,8 +2700,7 @@ sql_func_find(struct Expr *expr)
 				     name));
 		return NULL;
 	}
-	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
-	if (func->def->param_count != -1 && func->def->param_count != n) {
+	if (func->def->param_count != n) {
 		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
 			 tt_sprintf("%d", func->def->param_count), n);
 		return NULL;
@@ -2670,9 +2711,10 @@ sql_func_find(struct Expr *expr)
 uint32_t
 sql_func_flags(const char *name)
 {
-	struct func *func = func_by_name(name, strlen(name));
-	if (func == NULL || func->def->language != FUNC_LANGUAGE_SQL_BUILTIN)
+	struct func *func = built_in_func_get(name);
+	if (func == NULL)
 		return 0;
+	assert(func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
 	uint32_t flags = ((struct func_sql_builtin *)func)->flags;
 	if (func->def->aggregate == FUNC_AGGREGATE_GROUP)
 		flags |= SQL_FUNC_AGG;
@@ -2681,6 +2723,72 @@ sql_func_flags(const char *name)
 
 static struct func_vtab func_sql_builtin_vtab;
 
+void
+sql_built_in_functions_cache_init(void)
+{
+	built_in_functions = mh_strnptr_new();
+	if (built_in_functions == NULL)
+		panic("Out of memory on creating SQL built-in functions hash");
+	for (uint32_t i = 0; i < nelem(sql_builtins); ++i) {
+		const char *name = sql_builtins[i].name;
+		if (!sql_builtins[i].export_to_sql)
+			continue;
+		uint32_t len = strlen(name);
+		uint32_t size = sizeof(struct func_def) + len + 1;
+		struct func_def *def = malloc(size);
+		if (def == NULL)
+			panic("Out of memory on creating SQL built-in");
+		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)
+			panic("Out of memory on creating SQL built-in");
+
+		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;
+		built_in_func_put(&func->base);
+	}
+}
+
+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;
+		struct func *func = mh_strnptr_node(built_in_functions, k)->val;
+		mh_strnptr_del(built_in_functions, k, NULL);
+		func_delete(func);
+	}
+	assert(mh_size(built_in_functions) == 0);
+	mh_strnptr_delete(built_in_functions);
+}
+
 struct func *
 func_sql_builtin_new(struct func_def *def)
 {
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index b2328487c..cea440c64 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -176,73 +176,7 @@ box.space._priv:select{}
   - [1, 0, 'universe', 0, 24]
   - [1, 1, 'universe', 0, 4294967295]
   - [1, 2, 'function', 1, 4]
-  - [1, 2, 'function', 2, 4]
-  - [1, 2, 'function', 3, 4]
-  - [1, 2, 'function', 4, 4]
-  - [1, 2, 'function', 5, 4]
-  - [1, 2, 'function', 6, 4]
-  - [1, 2, 'function', 7, 4]
-  - [1, 2, 'function', 8, 4]
-  - [1, 2, 'function', 9, 4]
-  - [1, 2, 'function', 10, 4]
-  - [1, 2, 'function', 11, 4]
-  - [1, 2, 'function', 12, 4]
-  - [1, 2, 'function', 13, 4]
-  - [1, 2, 'function', 14, 4]
-  - [1, 2, 'function', 15, 4]
-  - [1, 2, 'function', 16, 4]
-  - [1, 2, 'function', 17, 4]
-  - [1, 2, 'function', 18, 4]
-  - [1, 2, 'function', 19, 4]
-  - [1, 2, 'function', 20, 4]
-  - [1, 2, 'function', 21, 4]
-  - [1, 2, 'function', 22, 4]
-  - [1, 2, 'function', 23, 4]
-  - [1, 2, 'function', 24, 4]
-  - [1, 2, 'function', 25, 4]
-  - [1, 2, 'function', 26, 4]
-  - [1, 2, 'function', 27, 4]
-  - [1, 2, 'function', 28, 4]
-  - [1, 2, 'function', 29, 4]
-  - [1, 2, 'function', 30, 4]
-  - [1, 2, 'function', 31, 4]
-  - [1, 2, 'function', 32, 4]
-  - [1, 2, 'function', 33, 4]
-  - [1, 2, 'function', 34, 4]
-  - [1, 2, 'function', 35, 4]
-  - [1, 2, 'function', 36, 4]
-  - [1, 2, 'function', 37, 4]
-  - [1, 2, 'function', 38, 4]
-  - [1, 2, 'function', 39, 4]
-  - [1, 2, 'function', 40, 4]
-  - [1, 2, 'function', 41, 4]
-  - [1, 2, 'function', 42, 4]
-  - [1, 2, 'function', 43, 4]
-  - [1, 2, 'function', 44, 4]
-  - [1, 2, 'function', 45, 4]
-  - [1, 2, 'function', 46, 4]
-  - [1, 2, 'function', 47, 4]
-  - [1, 2, 'function', 48, 4]
-  - [1, 2, 'function', 49, 4]
-  - [1, 2, 'function', 50, 4]
-  - [1, 2, 'function', 51, 4]
-  - [1, 2, 'function', 52, 4]
-  - [1, 2, 'function', 53, 4]
-  - [1, 2, 'function', 54, 4]
-  - [1, 2, 'function', 55, 4]
-  - [1, 2, 'function', 56, 4]
-  - [1, 2, 'function', 57, 4]
-  - [1, 2, 'function', 58, 4]
-  - [1, 2, 'function', 59, 4]
-  - [1, 2, 'function', 60, 4]
-  - [1, 2, 'function', 61, 4]
-  - [1, 2, 'function', 62, 4]
-  - [1, 2, 'function', 63, 4]
-  - [1, 2, 'function', 64, 4]
   - [1, 2, 'function', 65, 4]
-  - [1, 2, 'function', 66, 4]
-  - [1, 2, 'function', 67, 4]
-  - [1, 2, 'function', 68, 4]
   - [1, 2, 'space', 276, 2]
   - [1, 2, 'space', 277, 1]
   - [1, 2, 'space', 281, 1]
diff --git a/test/box/access_bin.result b/test/box/access_bin.result
index aeb8b3bd8..7c720192f 100644
--- a/test/box/access_bin.result
+++ b/test/box/access_bin.result
@@ -295,10 +295,10 @@ test:drop()
 box.schema.user.grant('guest', 'execute', 'universe')
 ---
 ...
-function f1() return box.space._func:get(1)[4] end
+function f1() return box.space._func.index[2]:get({'f1'})[4] end
 ---
 ...
-function f2() return box.space._func:get(69)[4] end
+function f2() return box.space._func.index[2]:get({'f1'})[4] end
 ---
 ...
 box.schema.func.create('f1')
diff --git a/test/box/access_bin.test.lua b/test/box/access_bin.test.lua
index 954266858..e82ec759c 100644
--- a/test/box/access_bin.test.lua
+++ b/test/box/access_bin.test.lua
@@ -111,8 +111,8 @@ test:drop()
 --
 -- notice that guest can execute stuff, but can't read space _func
 box.schema.user.grant('guest', 'execute', 'universe')
-function f1() return box.space._func:get(1)[4] end
-function f2() return box.space._func:get(69)[4] end
+function f1() return box.space._func.index[2]:get({'f1'})[4] end
+function f2() return box.space._func.index[2]:get({'f1'})[4] end
 box.schema.func.create('f1')
 box.schema.func.create('f2',{setuid=true})
 c = net.connect(box.cfg.listen)
diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
index d7a7b7534..071fc8de2 100644
--- a/test/box/access_sysview.result
+++ b/test/box/access_sysview.result
@@ -258,11 +258,11 @@ box.session.su('guest')
 ...
 #box.space._vpriv:select{}
 ---
-- 83
+- 17
 ...
 #box.space._vfunc:select{}
 ---
-- 68
+- 2
 ...
 #box.space._vcollation:select{}
 ---
@@ -290,11 +290,11 @@ box.session.su('guest')
 ...
 #box.space._vpriv:select{}
 ---
-- 83
+- 17
 ...
 #box.space._vfunc:select{}
 ---
-- 68
+- 2
 ...
 #box.space._vsequence:select{}
 ---
diff --git a/test/box/function1.result b/test/box/function1.result
index 0166c828f..a49a133f7 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -97,7 +97,7 @@ box.func["function1.args"]
   exports:
     lua: true
     sql: false
-  id: 69
+  id: 66
   setuid: false
   is_multikey: false
   is_deterministic: false
@@ -593,7 +593,7 @@ func
   exports:
     lua: true
     sql: false
-  id: 69
+  id: 66
   setuid: false
   is_multikey: false
   is_deterministic: false
@@ -665,7 +665,7 @@ func
   exports:
     lua: true
     sql: false
-  id: 69
+  id: 66
   setuid: false
   is_multikey: false
   is_deterministic: false
@@ -1032,7 +1032,7 @@ box.func.test ~= nil
 box.func.test:drop()
 ---
 ...
--- Check SQL builtins
+-- Make sure there is no SQL built-in functions in _func.
 test_run:cmd("setopt delimiter ';'")
 ---
 - true
@@ -1048,7 +1048,7 @@ sql_builtin_list = {
 	"RANDOMBLOB", "NULLIF", "ZEROBLOB", "MIN", "MAX", "COALESCE", "EVERY",
 	"EXISTS", "EXTRACT", "SOME", "GREATER", "LESSER", "SOUNDEX",
 	"LIKELIHOOD", "LIKELY", "UNLIKELY", "_sql_stat_get", "_sql_stat_push",
-	"_sql_stat_init", "GREATEST", "LEAST"
+	"_sql_stat_init", "GREATEST", "LEAST", "UUID"
 }
 test_run:cmd("setopt delimiter ''");
 ---
@@ -1056,7 +1056,7 @@ test_run:cmd("setopt delimiter ''");
 ok = true
 ---
 ...
-for _, v in pairs(sql_builtin_list) do ok = ok and (box.space._func.index.name:get(v) ~= nil) end
+for _, v in pairs(sql_builtin_list) do ok = ok and (box.space._func.index.name:get(v) == nil) end
 ---
 ...
 ok == true
@@ -1067,26 +1067,6 @@ box.func.LUA:call({"return 1 + 1"})
 ---
 - 2
 ...
-box.schema.user.grant('guest', 'execute', 'function', 'SUM')
----
-...
-c = net.connect(box.cfg.listen)
----
-...
-c:call("SUM")
----
-- error: sql builtin function does not support Lua frontend
-...
-c:close()
----
-...
-box.schema.user.revoke('guest', 'execute', 'function', 'SUM')
----
-...
-box.schema.func.drop("SUM")
----
-- error: 'Can''t drop function 1: function is SQL built-in'
-...
 -- Introduce function options
 box.schema.func.create('test', {body = "function(tuple) return tuple end", is_deterministic = true, opts = {is_multikey = true}})
 ---
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index ab7b586a0..4fdd48520 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -363,7 +363,7 @@ f == nil
 box.func.test ~= nil
 box.func.test:drop()
 
--- Check SQL builtins
+-- Make sure there is no SQL built-in functions in _func.
 test_run:cmd("setopt delimiter ';'")
 sql_builtin_list = {
 	"TRIM", "TYPEOF", "PRINTF", "UNICODE", "CHAR", "HEX", "VERSION",
@@ -376,22 +376,15 @@ sql_builtin_list = {
 	"RANDOMBLOB", "NULLIF", "ZEROBLOB", "MIN", "MAX", "COALESCE", "EVERY",
 	"EXISTS", "EXTRACT", "SOME", "GREATER", "LESSER", "SOUNDEX",
 	"LIKELIHOOD", "LIKELY", "UNLIKELY", "_sql_stat_get", "_sql_stat_push",
-	"_sql_stat_init", "GREATEST", "LEAST"
+	"_sql_stat_init", "GREATEST", "LEAST", "UUID"
 }
 test_run:cmd("setopt delimiter ''");
 ok = true
-for _, v in pairs(sql_builtin_list) do ok = ok and (box.space._func.index.name:get(v) ~= nil) end
+for _, v in pairs(sql_builtin_list) do ok = ok and (box.space._func.index.name:get(v) == nil) end
 ok == true
 
 box.func.LUA:call({"return 1 + 1"})
 
-box.schema.user.grant('guest', 'execute', 'function', 'SUM')
-c = net.connect(box.cfg.listen)
-c:call("SUM")
-c:close()
-box.schema.user.revoke('guest', 'execute', 'function', 'SUM')
-box.schema.func.drop("SUM")
-
 -- Introduce function options
 box.schema.func.create('test', {body = "function(tuple) return tuple end", is_deterministic = true, opts = {is_multikey = true}})
 box.func['test'].is_multikey == true
diff --git a/test/sql-tap/func5.test.lua b/test/sql-tap/func5.test.lua
index 13698582b..bd4561afc 100755
--- a/test/sql-tap/func5.test.lua
+++ b/test/sql-tap/func5.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(27)
+test:plan(29)
 
 --!./tcltestrunner.lua
 -- 2010 August 27
@@ -346,4 +346,27 @@ box.func.F2:drop()
 box.space.T01:drop()
 box.space.T02:drop()
 
+--
+-- gh-6105:  Make sure that functions that were described in _func but were not
+-- implemented are now removed.
+--
+test:do_catchsql_test(
+    "func-7.3",
+    [[
+        SELECT SQRT();
+    ]], {
+        1, "Function 'SQRT' does not exist"
+    })
+
+-- Make sure that functions are looked up in built-in functions first.
+box.schema.func.create('ABS', {language = 'Lua', param_list = {"INTEGER"},
+                       body = body, returns = 'number', exports = {'LUA'}});
+test:do_execsql_test(
+    "func-7.4",
+    [[
+        SELECT ABS(-111);
+    ]], {
+        111
+    })
+
 test:finish_test()
diff --git a/test/wal_off/func_max.result b/test/wal_off/func_max.result
index cc5bcc141..a3ab5b431 100644
--- a/test/wal_off/func_max.result
+++ b/test/wal_off/func_max.result
@@ -42,11 +42,11 @@ test_run:cmd("setopt delimiter ''");
 ...
 func_limit()
 ---
-- error: 'Failed to create function ''func31933'': function id is too big'
+- error: 'Failed to create function ''func31936'': function id is too big'
 ...
 drop_limit_func()
 ---
-- error: Function 'func31933' does not exist
+- error: Function 'func31936' does not exist
 ...
 box.schema.user.create('testuser')
 ---
@@ -62,11 +62,11 @@ session.su('testuser')
 ...
 func_limit()
 ---
-- error: 'Failed to create function ''func31933'': function id is too big'
+- error: 'Failed to create function ''func31936'': function id is too big'
 ...
 drop_limit_func()
 ---
-- error: Function 'func31933' does not exist
+- error: Function 'func31936' does not exist
 ...
 session.su('admin')
 ---

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Tarantool-patches] [PATCH v2 4/6] alter: parse data dictionary version
  2021-08-04 12:58 [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 3/6] sql: remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
@ 2021-08-04 12:58 ` Mergen Imeev via Tarantool-patches
  2021-08-05 22:17   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 5/6] alter: disallow creation of SQL built-in function Mergen Imeev via Tarantool-patches
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-04 12:58 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

Version is needed to disallow creation of SQL built-in functions using
_func starting with 2.9.0.

Needed for #6106
---
 src/box/alter.cc  | 18 ++++++++++++++++++
 src/box/schema.cc |  4 ++++
 src/box/schema.h  |  1 +
 3 files changed, 23 insertions(+)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 935790df4..217b882ba 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4167,6 +4167,24 @@ on_replace_dd_schema(struct trigger * /* trigger */, void *event)
 			return -1;
 		REPLICASET_UUID = uu;
 		say_info("cluster uuid %s", tt_uuid_str(&uu));
+	} else if (strcmp(key, "version") == 0) {
+		if (new_tuple != NULL) {
+			uint32_t major, minor, patch;
+			if (tuple_field_u32(new_tuple, 1, &major) != 0 ||
+			    tuple_field_u32(new_tuple, 2, &minor) != 0)
+				tnt_raise(ClientError, ER_WRONG_DD_VERSION);
+			/* Version can be major.minor with no patch. */
+			if (tuple_field_u32(new_tuple, 3, &patch) != 0)
+				patch = 0;
+			dd_version_id = version_id(major, minor, patch);
+		} else {
+			assert(old_tuple != NULL);
+			/*
+			 * _schema:delete({'version'}) for
+			 * example, for box.internal.bootstrap().
+			 */
+			dd_version_id = tarantool_version_id();
+		}
 	}
 	return 0;
 }
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 963278b19..35026619a 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -38,6 +38,7 @@
 #include "user.h"
 #include "vclock/vclock.h"
 #include "fiber.h"
+#include "version.h"
 
 /**
  * @module Data Dictionary
@@ -70,6 +71,9 @@ uint32_t schema_version = 0;
  */
 uint32_t space_cache_version = 0;
 
+/** Persistent version of the schema, stored in _schema["version"]. */
+uint32_t dd_version_id = tarantool_version_id();
+
 struct rlist on_schema_init = RLIST_HEAD_INITIALIZER(on_schema_init);
 struct rlist on_alter_space = RLIST_HEAD_INITIALIZER(on_alter_space);
 struct rlist on_alter_sequence = RLIST_HEAD_INITIALIZER(on_alter_sequence);
diff --git a/src/box/schema.h b/src/box/schema.h
index 25ac6f110..d3bbdd590 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -44,6 +44,7 @@ struct func;
 
 extern uint32_t schema_version;
 extern uint32_t space_cache_version;
+extern uint32_t dd_version_id;
 
 /** Triggers invoked after schema initialization. */
 extern struct rlist on_schema_init;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Tarantool-patches] [PATCH v2 5/6] alter: disallow creation of SQL built-in function
  2021-08-04 12:58 [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
                   ` (3 preceding siblings ...)
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 4/6] alter: parse data dictionary version Mergen Imeev via Tarantool-patches
@ 2021-08-04 12:58 ` Mergen Imeev via Tarantool-patches
  2021-08-05 22:18   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 6/6] sql: remove unnecessary function initialization Mergen Imeev via Tarantool-patches
  2021-08-08 12:08 ` [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Vladislav Shpilevoy via Tarantool-patches
  6 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-04 12:58 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch prohibits creation of user-defined functions with SQL_BUILTIN
engine.

Closes #6106
---
 src/box/alter.cc            | 38 +++++++++++++++++++++++++++++++++++++
 test/box/function1.result   |  7 ++++++-
 test/box/function1.test.lua |  3 +++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 217b882ba..fd9921ae0 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3213,6 +3213,36 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 	return 0;
 }
 
+/**
+ * Check if the version of the data dictionary is lower than 2.9.0 and return
+ * new func def if it is the case. If it is the case, then it is possible to
+ * insert values with the "SQL_BUILTIN" language into _func, otherwise it is
+ * prohibited. This is for upgradeability from 2.1.3 to 2.3.0. Since all we need
+ * is to allow such inserts, we set func def to its default values.
+ */
+static int
+func_def_new_sql_built_in(struct func_def *def)
+{
+	if (dd_version_id >= version_id(2, 9, 0)) {
+		diag_set(ClientError, ER_FUNCTION_LANGUAGE, "SQL_BUILTIN",
+			 def->name);
+		return -1;
+	}
+	def->body = NULL;
+	def->comment = NULL;
+	def->setuid = 1;
+	def->is_deterministic = false;
+	def->is_sandboxed = false;
+	def->param_count = 0;
+	def->returns = FIELD_TYPE_ANY;
+	def->aggregate = FUNC_AGGREGATE_NONE;
+	def->language = FUNC_LANGUAGE_LUA;
+	def->exports.lua = true;
+	def->exports.sql = true;
+	func_opts_create(&def->opts);
+	return 0;
+}
+
 /**
  * Get function identifiers from a tuple.
  *
@@ -3344,6 +3374,14 @@ func_def_new_from_tuple(struct tuple *tuple)
 				  language, def->name);
 			return NULL;
 		}
+		if (def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
+			if (func_def_new_sql_built_in(def) != 0)
+				return NULL;
+			if (func_def_check(def) != 0)
+				return NULL;
+			def_guard.is_active = false;
+			return def;
+		}
 	} else {
 		/* Lua is the default. */
 		def->language = FUNC_LANGUAGE_LUA;
diff --git a/test/box/function1.result b/test/box/function1.result
index a49a133f7..a1c89850d 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -372,7 +372,7 @@ c:close()
 box.schema.func.create('WAITFOR', {language = 'SQL_BUILTIN', \
 	param_list = {'integer'}, returns = 'integer',exports = {'SQL'}})
 ---
-- error: 'Failed to create function ''WAITFOR'': given built-in is not predefined'
+- error: Unsupported language 'SQL_BUILTIN' specified for function 'WAITFOR'
 ...
 test_run:cmd("setopt delimiter ';'")
 ---
@@ -1078,3 +1078,8 @@ box.func['test'].is_multikey == true
 box.func['test']:drop()
 ---
 ...
+-- gh-6106: Check that user-defined functions cannot have SQL_BUILTIN engine.
+box.schema.func.create("ABS", {language = 'SQL_BUILTIN', returns = 'integer'})
+---
+- error: Unsupported language 'SQL_BUILTIN' specified for function 'ABS'
+...
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index 4fdd48520..e635b6e18 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -389,3 +389,6 @@ box.func.LUA:call({"return 1 + 1"})
 box.schema.func.create('test', {body = "function(tuple) return tuple end", is_deterministic = true, opts = {is_multikey = true}})
 box.func['test'].is_multikey == true
 box.func['test']:drop()
+
+-- gh-6106: Check that user-defined functions cannot have SQL_BUILTIN engine.
+box.schema.func.create("ABS", {language = 'SQL_BUILTIN', returns = 'integer'})
-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Tarantool-patches] [PATCH v2 6/6] sql: remove unnecessary function initialization
  2021-08-04 12:58 [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
                   ` (4 preceding siblings ...)
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 5/6] alter: disallow creation of SQL built-in function Mergen Imeev via Tarantool-patches
@ 2021-08-04 12:58 ` Mergen Imeev via Tarantool-patches
  2021-08-06 19:59   ` Mergen Imeev via Tarantool-patches
  2021-08-08 12:08 ` [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Vladislav Shpilevoy via Tarantool-patches
  6 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-04 12:58 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

After removing the SQL built-in functions from _func, the code used to
initialize these SQL built-in functions is no longer used and should be
removed.

Follow-up #6106
---
 src/box/func.c     |  7 -------
 src/box/sql/func.c | 49 ----------------------------------------------
 2 files changed, 56 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 731f18a3b..04ae1b958 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -394,10 +394,6 @@ restore_fail:
 static struct func *
 func_c_new(struct func_def *def);
 
-/** Construct a SQL builtin function object. */
-extern struct func *
-func_sql_builtin_new(struct func_def *def);
-
 struct func *
 func_new(struct func_def *def)
 {
@@ -409,9 +405,6 @@ func_new(struct func_def *def)
 	case FUNC_LANGUAGE_LUA:
 		func = func_lua_new(def);
 		break;
-	case FUNC_LANGUAGE_SQL_BUILTIN:
-		func = func_sql_builtin_new(def);
-		break;
 	default:
 		unreachable();
 	}
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 2a3a5d457..50014b756 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -2789,55 +2789,6 @@ sql_built_in_functions_cache_free(void)
 	mh_strnptr_delete(built_in_functions);
 }
 
-struct func *
-func_sql_builtin_new(struct func_def *def)
-{
-	assert(def->language == FUNC_LANGUAGE_SQL_BUILTIN);
-	/** Binary search for corresponding builtin entry. */
-	int idx = -1, left = 0, right = nelem(sql_builtins) - 1;
-	while (left <= right) {
-		uint32_t mid = (left + right) / 2;
-		int rc = strcmp(def->name, sql_builtins[mid].name);
-		if (rc == 0) {
-			idx = mid;
-			break;
-		}
-		if (rc < 0)
-			right = mid - 1;
-		else
-			left = mid + 1;
-	}
-	/*
-	 * All SQL built-in(s) (stubs) are defined in a snapshot.
-	 * Implementation-specific metadata is defined in
-	 * sql_builtins list. When a definition were not found
-	 * above, the function name is invalid, i.e. it is
-	 * not built-in function.
-	 */
-	if (idx == -1) {
-		diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
-			 "given built-in is not predefined");
-		return NULL;
-	}
-	struct func_sql_builtin *func =
-		(struct func_sql_builtin *) calloc(1, sizeof(*func));
-	if (func == NULL) {
-		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
-		return NULL;
-	}
-	func->base.def = def;
-	func->base.vtab = &func_sql_builtin_vtab;
-	func->flags = sql_builtins[idx].flags;
-	func->call = sql_builtins[idx].call;
-	func->finalize = sql_builtins[idx].finalize;
-	def->param_count = sql_builtins[idx].param_count;
-	def->is_deterministic = sql_builtins[idx].is_deterministic;
-	def->returns = sql_builtins[idx].returns;
-	def->aggregate = sql_builtins[idx].aggregate;
-	def->exports.sql = sql_builtins[idx].export_to_sql;
-	return &func->base;
-}
-
 static void
 func_sql_builtin_destroy(struct func *func)
 {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/6] sql: introduce sql_func_flags()
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 1/6] sql: introduce sql_func_flags() Mergen Imeev via Tarantool-patches
@ 2021-08-05 22:14   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-06 19:41     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-05 22:14 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the patch!

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 115c52f96..97b7a2401 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -1186,6 +1186,8 @@ struct type_def {
>   *     SQL_FUNC_LENGTH    ==  OPFLAG_LENGTHARG
>   *     SQL_FUNC_TYPEOF    ==  OPFLAG_TYPEOFARG
>   */
> +/** Function is one of aggregate functions. */
> +#define SQL_FUNC_AGG		0x0001

The value is misaligned with the values below. They are using
whitespaces, so this new line probably also should use them.

>  #define SQL_FUNC_LIKE     0x0004	/* Candidate for the LIKE optimization */
>  #define SQL_FUNC_NEEDCOLL 0x0020	/* sqlGetFuncCollSeq() might be called.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 2/6] sql: introduce sql_func_find()
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 2/6] sql: introduce sql_func_find() Mergen Imeev via Tarantool-patches
@ 2021-08-05 22:15   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-06 19:42     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-05 22:15 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 28d383293..9514d070a 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -2655,6 +2643,30 @@ static struct {
>  	},
>  };
>  
> +struct func *
> +sql_func_find(struct Expr *expr)
> +{
> +	const char *name = expr->u.zToken;
> +	struct func *func = func_by_name(name, strlen(name));
> +	if (func == NULL) {
> +		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
> +		return NULL;
> +	}
> +	if (!func->def->exports.sql) {
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +			 tt_sprintf("function %s() is not available in SQL",
> +				     name));
> +		return NULL;
> +	}
> +	int n = expr->x.pList ? expr->x.pList->nExpr : 0;

!= NULL.

> +	if (func->def->param_count != -1 && func->def->param_count != n) {
> +		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
> +			 tt_sprintf("%d", func->def->param_count), n);
> +		return NULL;
> +	}
> +	return func;
> +}

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 3/6] sql: remove SQL built-in functions from _func
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 3/6] sql: remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
@ 2021-08-05 22:17   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-06 19:45     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-05 22:17 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

> diff --git a/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
> new file mode 100644
> index 000000000..88b458540
> --- /dev/null
> +++ b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
> @@ -0,0 +1,7 @@
> +## feature/sql
> +
> +* SQL built-in functions were removed from \_func system space (gh-6106).
> +* After this patch, functions will be looked up first in SQL built-in functions
> +  and then in user-defined functions.

1. In the changelog you shouldn't talk about 'this patch'. Maybe
about the release. But even better - just say 'Function are now lookeup ...'.
The users won't care about patches anyway.

> +* Fixed incorrect error message in case of misuse of the function used to set
> +  the default value.
> \ No newline at end of file

2. Please, add an empty line at the end of the file.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 4/6] alter: parse data dictionary version
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 4/6] alter: parse data dictionary version Mergen Imeev via Tarantool-patches
@ 2021-08-05 22:17   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-06 19:47     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-05 22:17 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 963278b19..35026619a 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -70,6 +71,9 @@ uint32_t schema_version = 0;
>   */
>  uint32_t space_cache_version = 0;
>  
> +/** Persistent version of the schema, stored in _schema["version"]. */
> +uint32_t dd_version_id = tarantool_version_id();

I propose to assign it to 0. Because at start there is no
schema at all until box.cfg{} is called first time. (Also then
version.h include could be dropped above.)

> +
>  struct rlist on_schema_init = RLIST_HEAD_INITIALIZER(on_schema_init);
>  struct rlist on_alter_space = RLIST_HEAD_INITIALIZER(on_alter_space);
>  struct rlist on_alter_sequence = RLIST_HEAD_INITIALIZER(on_alter_sequence);

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 5/6] alter: disallow creation of SQL built-in function
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 5/6] alter: disallow creation of SQL built-in function Mergen Imeev via Tarantool-patches
@ 2021-08-05 22:18   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-06 19:54     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-05 22:18 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 217b882ba..fd9921ae0 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3213,6 +3213,36 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
>  	return 0;
>  }
>  
> +/**
> + * Check if the version of the data dictionary is lower than 2.9.0 and return
> + * new func def if it is the case. If it is the case, then it is possible to
> + * insert values with the "SQL_BUILTIN" language into _func, otherwise it is
> + * prohibited. This is for upgradeability from 2.1.3 to 2.3.0. Since all we need
> + * is to allow such inserts, we set func def to its default values.
> + */
> +static int
> +func_def_new_sql_built_in(struct func_def *def)

1. 'new' stands for new memory allocation. Here you need to use
'create'.

> +{
> +	if (dd_version_id >= version_id(2, 9, 0)) {
> +		diag_set(ClientError, ER_FUNCTION_LANGUAGE, "SQL_BUILTIN",
> +			 def->name);
> +		return -1;
> +	}
> +	def->body = NULL;
> +	def->comment = NULL;
> +	def->setuid = 1;
> +	def->is_deterministic = false;
> +	def->is_sandboxed = false;
> +	def->param_count = 0;
> +	def->returns = FIELD_TYPE_ANY;
> +	def->aggregate = FUNC_AGGREGATE_NONE;
> +	def->language = FUNC_LANGUAGE_LUA;
> +	def->exports.lua = true;
> +	def->exports.sql = true;
> +	func_opts_create(&def->opts);
> +	return 0;
> +}
> +
>  /**
>   * Get function identifiers from a tuple.
>   *
> @@ -3344,6 +3374,14 @@ func_def_new_from_tuple(struct tuple *tuple)
>  				  language, def->name);
>  			return NULL;
>  		}
> +		if (def->language == FUNC_LANGUAGE_SQL_BUILTIN) {

2. Is it possible to just skip such functions when the schema is old?
Simply not create anything for them assuming that they will be deleted
right afterwards. Not even store them in the func hash. Like they do
not exist. Then you can also drop sql_builtin support from
func_def_check().

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/6] sql: introduce sql_func_flags()
  2021-08-05 22:14   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-06 19:41     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-06 19:41 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! My answer, diff and new patch below.

On Fri, Aug 06, 2021 at 12:14:45AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> > index 115c52f96..97b7a2401 100644
> > --- a/src/box/sql/sqlInt.h
> > +++ b/src/box/sql/sqlInt.h
> > @@ -1186,6 +1186,8 @@ struct type_def {
> >   *     SQL_FUNC_LENGTH    ==  OPFLAG_LENGTHARG
> >   *     SQL_FUNC_TYPEOF    ==  OPFLAG_TYPEOFARG
> >   */
> > +/** Function is one of aggregate functions. */
> > +#define SQL_FUNC_AGG		0x0001
> 
> The value is misaligned with the values below. They are using
> whitespaces, so this new line probably also should use them.
> 
Fixed.

> >  #define SQL_FUNC_LIKE     0x0004	/* Candidate for the LIKE optimization */
> >  #define SQL_FUNC_NEEDCOLL 0x0020	/* sqlGetFuncCollSeq() might be called.


Diff:

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 97b7a2401..a92de0a2f 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1187,7 +1187,7 @@ struct type_def {
  *     SQL_FUNC_TYPEOF    ==  OPFLAG_TYPEOFARG
  */
 /** Function is one of aggregate functions. */
-#define SQL_FUNC_AGG		0x0001
+#define SQL_FUNC_AGG      0x0001
 #define SQL_FUNC_LIKE     0x0004	/* Candidate for the LIKE optimization */
 #define SQL_FUNC_NEEDCOLL 0x0020	/* sqlGetFuncCollSeq() might be called.
 					 * The flag is set when the collation



New patch:


commit 9f04ffb91f7347af0fe7ce83f80e6e62d7d4a64f
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Aug 3 11:35:33 2021 +0300

    sql: introduce sql_func_flags()
    
    This function returns a set of parameters for the function with the
    given name. This function is used when we do not need to call a
    function, but we need its parameters.
    
    In addition, this function will allow us to split the parameters
    between those that are the same for all implementations, and the
    parameters, the value of which is implementation-dependent.
    
    Needed for #6105
    Part of #6106

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index d2624516c..80f2d349a 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -328,11 +328,8 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
 		if (op == TK_FUNCTION) {
 			uint32_t 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)
-				break;
-			if (sql_func_flag_is_set(func, SQL_FUNC_DERIVEDCOLL) &&
+			uint32_t flags = sql_func_flags(p->u.zToken);
+			if (((flags & SQL_FUNC_DERIVEDCOLL) != 0) &&
 			    arg_count > 0) {
 				/*
 				 * Now we use quite straightforward
@@ -342,7 +339,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
 				 * built-in functions: trim, upper,
 				 * lower, replace, substr.
 				 */
-				assert(func->def->returns == FIELD_TYPE_STRING);
+				assert(p->type == FIELD_TYPE_STRING);
 				p = p->x.pList->a->pExpr;
 				continue;
 			}
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 6ca852dec..28d383293 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -2655,6 +2655,18 @@ static struct {
 	},
 };
 
+uint32_t
+sql_func_flags(const char *name)
+{
+	struct func *func = func_by_name(name, strlen(name));
+	if (func == NULL || func->def->language != FUNC_LANGUAGE_SQL_BUILTIN)
+		return 0;
+	uint32_t flags = ((struct func_sql_builtin *)func)->flags;
+	if (func->def->aggregate == FUNC_AGGREGATE_GROUP)
+		flags |= SQL_FUNC_AGG;
+	return flags;
+}
+
 static struct func_vtab func_sql_builtin_vtab;
 
 struct func *
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 115c52f96..a92de0a2f 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1186,6 +1186,8 @@ struct type_def {
  *     SQL_FUNC_LENGTH    ==  OPFLAG_LENGTHARG
  *     SQL_FUNC_TYPEOF    ==  OPFLAG_TYPEOFARG
  */
+/** Function is one of aggregate functions. */
+#define SQL_FUNC_AGG      0x0001
 #define SQL_FUNC_LIKE     0x0004	/* Candidate for the LIKE optimization */
 #define SQL_FUNC_NEEDCOLL 0x0020	/* sqlGetFuncCollSeq() might be called.
 					 * The flag is set when the collation
@@ -4372,6 +4374,14 @@ sql_func_flag_is_set(struct func *func, uint16_t flag)
 struct func *
 sql_func_by_signature(const char *name, int argc);
 
+/**
+ * Return the parameters of the function with the given name. If the function
+ * with the given name does not exist, or the function is not a built-in SQL
+ * function, 0 is returned, which means no parameters have been set.
+ */
+uint32_t
+sql_func_flags(const char *name);
+
 /**
  * Generate VDBE code to halt execution with correct error if
  * the object with specified key is already present (or doesn't

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 2/6] sql: introduce sql_func_find()
  2021-08-05 22:15   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-06 19:42     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-06 19:42 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review! My answer, diff and new patch below.

On Fri, Aug 06, 2021 at 12:15:20AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 28d383293..9514d070a 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -2655,6 +2643,30 @@ static struct {
> >  	},
> >  };
> >  
> > +struct func *
> > +sql_func_find(struct Expr *expr)
> > +{
> > +	const char *name = expr->u.zToken;
> > +	struct func *func = func_by_name(name, strlen(name));
> > +	if (func == NULL) {
> > +		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
> > +		return NULL;
> > +	}
> > +	if (!func->def->exports.sql) {
> > +		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> > +			 tt_sprintf("function %s() is not available in SQL",
> > +				     name));
> > +		return NULL;
> > +	}
> > +	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
> 
> != NULL.
> 
Fixed.

> > +	if (func->def->param_count != -1 && func->def->param_count != n) {
> > +		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
> > +			 tt_sprintf("%d", func->def->param_count), n);
> > +		return NULL;
> > +	}
> > +	return func;
> > +}

Diff:


diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9514d070a..7cdcce6bc 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -2658,7 +2658,7 @@ sql_func_find(struct Expr *expr)
 				     name));
 		return NULL;
 	}
-	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
+	int n = expr->x.pList != NULL ? expr->x.pList->nExpr : 0;
 	if (func->def->param_count != -1 && func->def->param_count != n) {
 		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
 			 tt_sprintf("%d", func->def->param_count), n);



New patch:


commit fbb57e6bf3a40d387a1641f4233680613d3b7f5e
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Jul 31 19:05:05 2021 +0300

    sql: introduce sql_func_find()
    
    This patch introduces the sql_func_find() function. This function allows
    us to centralize the look up of functions during parsing, which
    simplifies code and fixes some incorrect error messages.
    
    Part of #6106

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index b87f69512..afa2331a1 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -719,6 +719,10 @@ callStatGet(Vdbe * v, int regStat4, int iParam, int regOut)
 {
 	assert(regOut != regStat4 && regOut != regStat4 + 1);
 	sqlVdbeAddOp2(v, OP_Integer, iParam, regStat4 + 1);
+	/*
+	 * Function sql_func_by_signature() was removed, so after enabling this
+	 * part should be changed.
+	 */
 	struct func *func = sql_func_by_signature("_sql_stat_get", 2);
 	assert(func != NULL);
 	sqlVdbeAddOp4(v, OP_BuiltinFunction0, 0, regStat4, regOut,
@@ -858,6 +862,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		sqlVdbeAddOp2(v, OP_Count, idx_cursor, stat4_reg + 3);
 		sqlVdbeAddOp2(v, OP_Integer, part_count, stat4_reg + 1);
 		sqlVdbeAddOp2(v, OP_Integer, part_count, stat4_reg + 2);
+		/*
+		 * Function sql_func_by_signature() was removed, so after
+		 * enabling this part should be changed.
+		 */
 		struct func *init_func =
 			sql_func_by_signature("_sql_stat_init", 3);
 		assert(init_func != NULL);
@@ -959,6 +967,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		sqlVdbeAddOp3(v, OP_MakeRecord, stat_key_reg,
 				  pk_part_count, key_reg);
 		assert(chng_reg == (stat4_reg + 1));
+		/*
+		 * Function sql_func_by_signature() was removed, so after
+		 * enabling this part should be changed.
+		 */
 		struct func *push_func =
 			sql_func_by_signature("_sql_stat_push", 3);
 		assert(push_func != NULL);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 80f2d349a..20d22455c 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3957,7 +3957,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 	case TK_FUNCTION:{
 			ExprList *pFarg;	/* List of function arguments */
 			int nFarg;	/* Number of function arguments */
-			const char *zId;	/* The function name */
 			u32 constMask = 0;	/* Mask of function arguments that are constant */
 			int i;	/* Loop counter */
 			struct coll *coll = NULL;
@@ -3970,11 +3969,8 @@ 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);
+			struct func *func = sql_func_find(pExpr);
 			if (func == NULL) {
-				diag_set(ClientError, ER_NO_SUCH_FUNCTION,
-					 zId);
 				pParse->is_aborted = true;
 				break;
 			}
@@ -5431,14 +5427,8 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr)
 						pItem->iMem = ++pParse->nMem;
 						assert(!ExprHasProperty
 						       (pExpr, EP_IntValue));
-						const char *name =
-							pExpr->u.zToken;
-						uint32_t argc =
-							pExpr->x.pList != NULL ?
-							pExpr->x.pList->nExpr : 0;
 						pItem->func =
-							sql_func_by_signature(
-								name, argc);
+							sql_func_find(pExpr);
 						assert(pItem->func != NULL);
 						assert(pItem->func->def->
 						       language ==
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 28d383293..7cdcce6bc 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1934,24 +1934,12 @@ sql_is_like_func(struct Expr *expr)
 	    expr->x.pList->nExpr != 2)
 		return 0;
 	assert(!ExprHasProperty(expr, EP_xIsSelect));
-	struct func *func = sql_func_by_signature(expr->u.zToken, 2);
+	struct func *func = sql_func_find(expr);
 	if (func == NULL || !sql_func_flag_is_set(func, SQL_FUNC_LIKE))
 		return 0;
 	return 1;
 }
 
-struct func *
-sql_func_by_signature(const char *name, int argc)
-{
-	struct func *base = func_by_name(name, strlen(name));
-	if (base == NULL || !base->def->exports.sql)
-		return NULL;
-
-	if (base->def->param_count != -1 && base->def->param_count != argc)
-		return NULL;
-	return base;
-}
-
 static int
 func_sql_builtin_call_stub(struct func *func, struct port *args,
 			   struct port *ret)
@@ -2655,6 +2643,30 @@ static struct {
 	},
 };
 
+struct func *
+sql_func_find(struct Expr *expr)
+{
+	const char *name = expr->u.zToken;
+	struct func *func = func_by_name(name, strlen(name));
+	if (func == NULL) {
+		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
+		return NULL;
+	}
+	if (!func->def->exports.sql) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+			 tt_sprintf("function %s() is not available in SQL",
+				     name));
+		return NULL;
+	}
+	int n = expr->x.pList != NULL ? expr->x.pList->nExpr : 0;
+	if (func->def->param_count != -1 && func->def->param_count != n) {
+		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
+			 tt_sprintf("%d", func->def->param_count), n);
+		return NULL;
+	}
+	return func;
+}
+
 uint32_t
 sql_func_flags(const char *name)
 {
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 11b6139e3..35faddab5 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -598,28 +598,8 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
 			assert(!ExprHasProperty(pExpr, EP_xIsSelect));
 			zId = pExpr->u.zToken;
 			nId = sqlStrlen30(zId);
-			struct func *func = func_by_name(zId, nId);
+			struct func *func = sql_func_find(pExpr);
 			if (func == NULL) {
-				diag_set(ClientError, ER_NO_SUCH_FUNCTION, zId);
-				pParse->is_aborted = true;
-				pNC->nErr++;
-				return WRC_Abort;
-			}
-			if (!func->def->exports.sql) {
-				diag_set(ClientError, ER_SQL_PARSER_GENERIC,
-					 tt_sprintf("function %.*s() is not "
-						    "available in SQL",
-						     nId, zId));
-				pParse->is_aborted = true;
-				pNC->nErr++;
-				return WRC_Abort;
-			}
-			if (func->def->param_count != -1 &&
-			    func->def->param_count != n) {
-				uint32_t argc = func->def->param_count;
-				const char *err = tt_sprintf("%d", argc);
-				diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
-					 func->def->name, err, n);
 				pParse->is_aborted = true;
 				pNC->nErr++;
 				return WRC_Abort;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index a92de0a2f..c6927e1e4 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4362,17 +4362,9 @@ sql_func_flag_is_set(struct func *func, uint16_t flag)
 	return (((struct func_sql_builtin *)func)->flags & flag) != 0;
 }
 
-/**
- * A SQL method to find a function in a hash by its name and
- * count of arguments. Only functions that have 'SQL' engine
- * export field set true and have exactly the same signature
- * are returned.
- *
- * Returns not NULL function pointer when a valid and exported
- * to SQL engine function is found and NULL otherwise.
- */
+/** Return a function that matches the parameters described in given expr. */
 struct func *
-sql_func_by_signature(const char *name, int argc);
+sql_func_find(struct Expr *expr);
 
 /**
  * Return the parameters of the function with the given name. If the function
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 2c5099616..499089c8d 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -148,7 +148,7 @@ valueFromFunction(sql * db,	/* The database connection */
 	pList = p->x.pList;
 	if (pList)
 		nVal = pList->nExpr;
-	struct func *func = sql_func_by_signature(p->u.zToken, nVal);
+	struct func *func = sql_func_find(p);
 	if (func == NULL || func->def->language != FUNC_LANGUAGE_SQL_BUILTIN ||
 	    !func->def->is_deterministic ||
 	    sql_func_flag_is_set(func, SQL_FUNC_NEEDCOLL))
diff --git a/test/sql-tap/func5.test.lua b/test/sql-tap/func5.test.lua
index 9b1526aaf..13698582b 100755
--- a/test/sql-tap/func5.test.lua
+++ b/test/sql-tap/func5.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(25)
+test:plan(27)
 
 --!./tcltestrunner.lua
 -- 2010 August 27
@@ -314,4 +314,36 @@ test:do_execsql_test(
 box.func.COUNTER1:drop()
 box.func.COUNTER2:drop()
 
+--
+-- Make sure the correct error is displayed if the function throws an error when
+-- setting the default value.
+--
+local body = 'function(x) return 1 end'
+box.schema.func.create('F1', {language = 'Lua', returns = 'number', body = body,
+                       param_list = {}, exports = {'LUA'}});
+box.execute([[CREATE TABLE t01(i INT PRIMARY KEY, a INT DEFAULT(f1(1)));]])
+test:do_catchsql_test(
+    "func-7.1",
+    [[
+        INSERT INTO t01(i) VALUES(1);
+    ]], {
+        1, "function F1() is not available in SQL"
+    })
+
+box.schema.func.create('F2', {language = 'Lua', returns = 'number', body = body,
+                       exports = {'LUA', 'SQL'}});
+box.execute([[CREATE TABLE t02(i INT PRIMARY KEY, a INT DEFAULT(f2(1)));]])
+test:do_catchsql_test(
+    "func-7.2",
+    [[
+        INSERT INTO t02(i) VALUES(1);
+    ]], {
+        1, "Wrong number of arguments is passed to F2(): expected 0, got 1"
+    })
+
+box.func.F1:drop()
+box.func.F2:drop()
+box.space.T01:drop()
+box.space.T02:drop()
+
 test:finish_test()

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 3/6] sql: remove SQL built-in functions from _func
  2021-08-05 22:17   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-06 19:45     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-06 19:45 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review! My answers, diff and new patch below.

On Fri, Aug 06, 2021 at 12:17:01AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
> > new file mode 100644
> > index 000000000..88b458540
> > --- /dev/null
> > +++ b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
> > @@ -0,0 +1,7 @@
> > +## feature/sql
> > +
> > +* SQL built-in functions were removed from \_func system space (gh-6106).
> > +* After this patch, functions will be looked up first in SQL built-in functions
> > +  and then in user-defined functions.
> 
> 1. In the changelog you shouldn't talk about 'this patch'. Maybe
> about the release. But even better - just say 'Function are now lookeup ...'.
> The users won't care about patches anyway.
> 
Thanks! Fixed.

> > +* Fixed incorrect error message in case of misuse of the function used to set
> > +  the default value.
> > \ No newline at end of file
> 
> 2. Please, add an empty line at the end of the file.
Fixed.


Diff:

diff --git a/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
index 88b458540..02f33dd50 100644
--- a/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
+++ b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
@@ -1,7 +1,7 @@
 ## feature/sql
 
 * SQL built-in functions were removed from \_func system space (gh-6106).
-* After this patch, functions will be looked up first in SQL built-in functions
-  and then in user-defined functions.
+* Function are now looked up first in SQL built-in functions and then in
+  user-defined functions.
 * Fixed incorrect error message in case of misuse of the function used to set
-  the default value.
\ No newline at end of file
+  the default value.


New patch:


commit 11992521e3347773167ab6009b9f67bac0385a58
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Jul 29 13:54:53 2021 +0300

    sql: remove SQL built-in functions from _func
    
    This patch removes SQL built-in functions from _func. These functions
    could be called directly from Lua, however all they did was returned an
    error. After this patch, no SQL built-in functions can be called
    directly from LUA.
    
    Part of #6106

diff --git a/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
new file mode 100644
index 000000000..02f33dd50
--- /dev/null
+++ b/changelogs/unreleased/gh-6106-remove-sql-built-ins-from-_func.md
@@ -0,0 +1,7 @@
+## feature/sql
+
+* SQL built-in functions were removed from \_func system space (gh-6106).
+* Function are now looked up first in SQL built-in functions and then in
+  user-defined functions.
+* Fixed incorrect error message in case of misuse of the function used to set
+  the default value.
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 390199298..935790df4 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3548,13 +3548,6 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 				  "function has references");
 			return -1;
 		}
-		/* Can't' drop a builtin function. */
-		if (old_func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
-			diag_set(ClientError, ER_DROP_FUNCTION,
-				  (unsigned) old_func->def->uid,
-				  "function is SQL built-in");
-			return -1;
-		}
 		struct trigger *on_commit =
 			txn_alter_trigger_new(on_drop_func_commit, old_func);
 		struct trigger *on_rollback =
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 57374decc..018670d2a 100644
Binary files a/src/box/bootstrap.snap and b/src/box/bootstrap.snap differ
diff --git a/src/box/box.cc b/src/box/box.cc
index 8dc3b130b..66e658fc3 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -3041,6 +3041,7 @@ box_free(void)
 		gc_free();
 		engine_shutdown();
 		wal_free();
+		sql_built_in_functions_cache_free();
 	}
 }
 
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 97afc0b4d..6abce50f4 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
+            _priv:delete({2, 'function', v.id})
+            _func:delete({v.id})
+        end
+    end
 end
 
 local function upgrade_to_2_9_1()
-    sql_builtin_function_uuid()
+    remove_sql_builtin_functions_from_func()
 end
 
 --------------------------------------------------------------------------------
diff --git a/src/box/sql.c b/src/box/sql.c
index 433264abe..f18dfa063 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -75,6 +75,7 @@ sql_init(void)
 		panic("failed to initialize SQL subsystem");
 
 	sql_stmt_cache_init();
+	sql_built_in_functions_cache_init();
 
 	assert(db != NULL);
 }
diff --git a/src/box/sql.h b/src/box/sql.h
index 4c364306c..2ac97c762 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -56,6 +56,15 @@ sql_init(void);
 struct sql *
 sql_get(void);
 
+/** Initialize global cache for built-in functions. */
+void
+sql_built_in_functions_cache_init(void);
+
+/** Free global cache for built-in functions. */
+void
+sql_built_in_functions_cache_free(void);
+
+
 struct Expr;
 struct Parse;
 struct Select;
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 7cdcce6bc..2a3a5d457 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -47,6 +47,10 @@
 #include <unicode/ucol.h>
 #include "box/coll_id_cache.h"
 #include "box/schema.h"
+#include "box/user.h"
+#include "assoc.h"
+
+static struct mh_strnptr_t *built_in_functions = NULL;
 
 static const unsigned char *
 mem_as_ustr(struct Mem *mem)
@@ -2643,11 +2647,49 @@ static struct {
 	},
 };
 
+static struct func *
+built_in_func_get(const char *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))
+		return NULL;
+	return mh_strnptr_node(built_in_functions, k)->val;
+}
+
+static void
+built_in_func_put(struct func *func)
+{
+	const char *name = func->def->name;
+	uint32_t len = strlen(name);
+	assert(built_in_func_get(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)) {
+		panic("Out of memory on insertion into SQL built-in functions "
+		      "hash");
+	}
+}
+
 struct func *
 sql_func_find(struct Expr *expr)
 {
 	const char *name = expr->u.zToken;
-	struct func *func = func_by_name(name, strlen(name));
+	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
+	struct func *func = built_in_func_get(name);
+	if (func != NULL) {
+		assert(func->def->exports.sql);
+		int param_count = func->def->param_count;
+		if (param_count != -1 && param_count != n) {
+			diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
+				 tt_sprintf("%d", func->def->param_count), n);
+			return NULL;
+		}
+		return func;
+	}
+	func = func_by_name(name, strlen(name));
 	if (func == NULL) {
 		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
 		return NULL;
@@ -2658,8 +2700,7 @@ sql_func_find(struct Expr *expr)
 				     name));
 		return NULL;
 	}
-	int n = expr->x.pList != NULL ? expr->x.pList->nExpr : 0;
-	if (func->def->param_count != -1 && func->def->param_count != n) {
+	if (func->def->param_count != n) {
 		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
 			 tt_sprintf("%d", func->def->param_count), n);
 		return NULL;
@@ -2670,9 +2711,10 @@ sql_func_find(struct Expr *expr)
 uint32_t
 sql_func_flags(const char *name)
 {
-	struct func *func = func_by_name(name, strlen(name));
-	if (func == NULL || func->def->language != FUNC_LANGUAGE_SQL_BUILTIN)
+	struct func *func = built_in_func_get(name);
+	if (func == NULL)
 		return 0;
+	assert(func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
 	uint32_t flags = ((struct func_sql_builtin *)func)->flags;
 	if (func->def->aggregate == FUNC_AGGREGATE_GROUP)
 		flags |= SQL_FUNC_AGG;
@@ -2681,6 +2723,72 @@ sql_func_flags(const char *name)
 
 static struct func_vtab func_sql_builtin_vtab;
 
+void
+sql_built_in_functions_cache_init(void)
+{
+	built_in_functions = mh_strnptr_new();
+	if (built_in_functions == NULL)
+		panic("Out of memory on creating SQL built-in functions hash");
+	for (uint32_t i = 0; i < nelem(sql_builtins); ++i) {
+		const char *name = sql_builtins[i].name;
+		if (!sql_builtins[i].export_to_sql)
+			continue;
+		uint32_t len = strlen(name);
+		uint32_t size = sizeof(struct func_def) + len + 1;
+		struct func_def *def = malloc(size);
+		if (def == NULL)
+			panic("Out of memory on creating SQL built-in");
+		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)
+			panic("Out of memory on creating SQL built-in");
+
+		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;
+		built_in_func_put(&func->base);
+	}
+}
+
+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;
+		struct func *func = mh_strnptr_node(built_in_functions, k)->val;
+		mh_strnptr_del(built_in_functions, k, NULL);
+		func_delete(func);
+	}
+	assert(mh_size(built_in_functions) == 0);
+	mh_strnptr_delete(built_in_functions);
+}
+
 struct func *
 func_sql_builtin_new(struct func_def *def)
 {
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index b2328487c..cea440c64 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -176,73 +176,7 @@ box.space._priv:select{}
   - [1, 0, 'universe', 0, 24]
   - [1, 1, 'universe', 0, 4294967295]
   - [1, 2, 'function', 1, 4]
-  - [1, 2, 'function', 2, 4]
-  - [1, 2, 'function', 3, 4]
-  - [1, 2, 'function', 4, 4]
-  - [1, 2, 'function', 5, 4]
-  - [1, 2, 'function', 6, 4]
-  - [1, 2, 'function', 7, 4]
-  - [1, 2, 'function', 8, 4]
-  - [1, 2, 'function', 9, 4]
-  - [1, 2, 'function', 10, 4]
-  - [1, 2, 'function', 11, 4]
-  - [1, 2, 'function', 12, 4]
-  - [1, 2, 'function', 13, 4]
-  - [1, 2, 'function', 14, 4]
-  - [1, 2, 'function', 15, 4]
-  - [1, 2, 'function', 16, 4]
-  - [1, 2, 'function', 17, 4]
-  - [1, 2, 'function', 18, 4]
-  - [1, 2, 'function', 19, 4]
-  - [1, 2, 'function', 20, 4]
-  - [1, 2, 'function', 21, 4]
-  - [1, 2, 'function', 22, 4]
-  - [1, 2, 'function', 23, 4]
-  - [1, 2, 'function', 24, 4]
-  - [1, 2, 'function', 25, 4]
-  - [1, 2, 'function', 26, 4]
-  - [1, 2, 'function', 27, 4]
-  - [1, 2, 'function', 28, 4]
-  - [1, 2, 'function', 29, 4]
-  - [1, 2, 'function', 30, 4]
-  - [1, 2, 'function', 31, 4]
-  - [1, 2, 'function', 32, 4]
-  - [1, 2, 'function', 33, 4]
-  - [1, 2, 'function', 34, 4]
-  - [1, 2, 'function', 35, 4]
-  - [1, 2, 'function', 36, 4]
-  - [1, 2, 'function', 37, 4]
-  - [1, 2, 'function', 38, 4]
-  - [1, 2, 'function', 39, 4]
-  - [1, 2, 'function', 40, 4]
-  - [1, 2, 'function', 41, 4]
-  - [1, 2, 'function', 42, 4]
-  - [1, 2, 'function', 43, 4]
-  - [1, 2, 'function', 44, 4]
-  - [1, 2, 'function', 45, 4]
-  - [1, 2, 'function', 46, 4]
-  - [1, 2, 'function', 47, 4]
-  - [1, 2, 'function', 48, 4]
-  - [1, 2, 'function', 49, 4]
-  - [1, 2, 'function', 50, 4]
-  - [1, 2, 'function', 51, 4]
-  - [1, 2, 'function', 52, 4]
-  - [1, 2, 'function', 53, 4]
-  - [1, 2, 'function', 54, 4]
-  - [1, 2, 'function', 55, 4]
-  - [1, 2, 'function', 56, 4]
-  - [1, 2, 'function', 57, 4]
-  - [1, 2, 'function', 58, 4]
-  - [1, 2, 'function', 59, 4]
-  - [1, 2, 'function', 60, 4]
-  - [1, 2, 'function', 61, 4]
-  - [1, 2, 'function', 62, 4]
-  - [1, 2, 'function', 63, 4]
-  - [1, 2, 'function', 64, 4]
   - [1, 2, 'function', 65, 4]
-  - [1, 2, 'function', 66, 4]
-  - [1, 2, 'function', 67, 4]
-  - [1, 2, 'function', 68, 4]
   - [1, 2, 'space', 276, 2]
   - [1, 2, 'space', 277, 1]
   - [1, 2, 'space', 281, 1]
diff --git a/test/box/access_bin.result b/test/box/access_bin.result
index aeb8b3bd8..7c720192f 100644
--- a/test/box/access_bin.result
+++ b/test/box/access_bin.result
@@ -295,10 +295,10 @@ test:drop()
 box.schema.user.grant('guest', 'execute', 'universe')
 ---
 ...
-function f1() return box.space._func:get(1)[4] end
+function f1() return box.space._func.index[2]:get({'f1'})[4] end
 ---
 ...
-function f2() return box.space._func:get(69)[4] end
+function f2() return box.space._func.index[2]:get({'f1'})[4] end
 ---
 ...
 box.schema.func.create('f1')
diff --git a/test/box/access_bin.test.lua b/test/box/access_bin.test.lua
index 954266858..e82ec759c 100644
--- a/test/box/access_bin.test.lua
+++ b/test/box/access_bin.test.lua
@@ -111,8 +111,8 @@ test:drop()
 --
 -- notice that guest can execute stuff, but can't read space _func
 box.schema.user.grant('guest', 'execute', 'universe')
-function f1() return box.space._func:get(1)[4] end
-function f2() return box.space._func:get(69)[4] end
+function f1() return box.space._func.index[2]:get({'f1'})[4] end
+function f2() return box.space._func.index[2]:get({'f1'})[4] end
 box.schema.func.create('f1')
 box.schema.func.create('f2',{setuid=true})
 c = net.connect(box.cfg.listen)
diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
index d7a7b7534..071fc8de2 100644
--- a/test/box/access_sysview.result
+++ b/test/box/access_sysview.result
@@ -258,11 +258,11 @@ box.session.su('guest')
 ...
 #box.space._vpriv:select{}
 ---
-- 83
+- 17
 ...
 #box.space._vfunc:select{}
 ---
-- 68
+- 2
 ...
 #box.space._vcollation:select{}
 ---
@@ -290,11 +290,11 @@ box.session.su('guest')
 ...
 #box.space._vpriv:select{}
 ---
-- 83
+- 17
 ...
 #box.space._vfunc:select{}
 ---
-- 68
+- 2
 ...
 #box.space._vsequence:select{}
 ---
diff --git a/test/box/function1.result b/test/box/function1.result
index 0166c828f..a49a133f7 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -97,7 +97,7 @@ box.func["function1.args"]
   exports:
     lua: true
     sql: false
-  id: 69
+  id: 66
   setuid: false
   is_multikey: false
   is_deterministic: false
@@ -593,7 +593,7 @@ func
   exports:
     lua: true
     sql: false
-  id: 69
+  id: 66
   setuid: false
   is_multikey: false
   is_deterministic: false
@@ -665,7 +665,7 @@ func
   exports:
     lua: true
     sql: false
-  id: 69
+  id: 66
   setuid: false
   is_multikey: false
   is_deterministic: false
@@ -1032,7 +1032,7 @@ box.func.test ~= nil
 box.func.test:drop()
 ---
 ...
--- Check SQL builtins
+-- Make sure there is no SQL built-in functions in _func.
 test_run:cmd("setopt delimiter ';'")
 ---
 - true
@@ -1048,7 +1048,7 @@ sql_builtin_list = {
 	"RANDOMBLOB", "NULLIF", "ZEROBLOB", "MIN", "MAX", "COALESCE", "EVERY",
 	"EXISTS", "EXTRACT", "SOME", "GREATER", "LESSER", "SOUNDEX",
 	"LIKELIHOOD", "LIKELY", "UNLIKELY", "_sql_stat_get", "_sql_stat_push",
-	"_sql_stat_init", "GREATEST", "LEAST"
+	"_sql_stat_init", "GREATEST", "LEAST", "UUID"
 }
 test_run:cmd("setopt delimiter ''");
 ---
@@ -1056,7 +1056,7 @@ test_run:cmd("setopt delimiter ''");
 ok = true
 ---
 ...
-for _, v in pairs(sql_builtin_list) do ok = ok and (box.space._func.index.name:get(v) ~= nil) end
+for _, v in pairs(sql_builtin_list) do ok = ok and (box.space._func.index.name:get(v) == nil) end
 ---
 ...
 ok == true
@@ -1067,26 +1067,6 @@ box.func.LUA:call({"return 1 + 1"})
 ---
 - 2
 ...
-box.schema.user.grant('guest', 'execute', 'function', 'SUM')
----
-...
-c = net.connect(box.cfg.listen)
----
-...
-c:call("SUM")
----
-- error: sql builtin function does not support Lua frontend
-...
-c:close()
----
-...
-box.schema.user.revoke('guest', 'execute', 'function', 'SUM')
----
-...
-box.schema.func.drop("SUM")
----
-- error: 'Can''t drop function 1: function is SQL built-in'
-...
 -- Introduce function options
 box.schema.func.create('test', {body = "function(tuple) return tuple end", is_deterministic = true, opts = {is_multikey = true}})
 ---
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index ab7b586a0..4fdd48520 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -363,7 +363,7 @@ f == nil
 box.func.test ~= nil
 box.func.test:drop()
 
--- Check SQL builtins
+-- Make sure there is no SQL built-in functions in _func.
 test_run:cmd("setopt delimiter ';'")
 sql_builtin_list = {
 	"TRIM", "TYPEOF", "PRINTF", "UNICODE", "CHAR", "HEX", "VERSION",
@@ -376,22 +376,15 @@ sql_builtin_list = {
 	"RANDOMBLOB", "NULLIF", "ZEROBLOB", "MIN", "MAX", "COALESCE", "EVERY",
 	"EXISTS", "EXTRACT", "SOME", "GREATER", "LESSER", "SOUNDEX",
 	"LIKELIHOOD", "LIKELY", "UNLIKELY", "_sql_stat_get", "_sql_stat_push",
-	"_sql_stat_init", "GREATEST", "LEAST"
+	"_sql_stat_init", "GREATEST", "LEAST", "UUID"
 }
 test_run:cmd("setopt delimiter ''");
 ok = true
-for _, v in pairs(sql_builtin_list) do ok = ok and (box.space._func.index.name:get(v) ~= nil) end
+for _, v in pairs(sql_builtin_list) do ok = ok and (box.space._func.index.name:get(v) == nil) end
 ok == true
 
 box.func.LUA:call({"return 1 + 1"})
 
-box.schema.user.grant('guest', 'execute', 'function', 'SUM')
-c = net.connect(box.cfg.listen)
-c:call("SUM")
-c:close()
-box.schema.user.revoke('guest', 'execute', 'function', 'SUM')
-box.schema.func.drop("SUM")
-
 -- Introduce function options
 box.schema.func.create('test', {body = "function(tuple) return tuple end", is_deterministic = true, opts = {is_multikey = true}})
 box.func['test'].is_multikey == true
diff --git a/test/sql-tap/func5.test.lua b/test/sql-tap/func5.test.lua
index 13698582b..bd4561afc 100755
--- a/test/sql-tap/func5.test.lua
+++ b/test/sql-tap/func5.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(27)
+test:plan(29)
 
 --!./tcltestrunner.lua
 -- 2010 August 27
@@ -346,4 +346,27 @@ box.func.F2:drop()
 box.space.T01:drop()
 box.space.T02:drop()
 
+--
+-- gh-6105:  Make sure that functions that were described in _func but were not
+-- implemented are now removed.
+--
+test:do_catchsql_test(
+    "func-7.3",
+    [[
+        SELECT SQRT();
+    ]], {
+        1, "Function 'SQRT' does not exist"
+    })
+
+-- Make sure that functions are looked up in built-in functions first.
+box.schema.func.create('ABS', {language = 'Lua', param_list = {"INTEGER"},
+                       body = body, returns = 'number', exports = {'LUA'}});
+test:do_execsql_test(
+    "func-7.4",
+    [[
+        SELECT ABS(-111);
+    ]], {
+        111
+    })
+
 test:finish_test()
diff --git a/test/wal_off/func_max.result b/test/wal_off/func_max.result
index cc5bcc141..a3ab5b431 100644
--- a/test/wal_off/func_max.result
+++ b/test/wal_off/func_max.result
@@ -42,11 +42,11 @@ test_run:cmd("setopt delimiter ''");
 ...
 func_limit()
 ---
-- error: 'Failed to create function ''func31933'': function id is too big'
+- error: 'Failed to create function ''func31936'': function id is too big'
 ...
 drop_limit_func()
 ---
-- error: Function 'func31933' does not exist
+- error: Function 'func31936' does not exist
 ...
 box.schema.user.create('testuser')
 ---
@@ -62,11 +62,11 @@ session.su('testuser')
 ...
 func_limit()
 ---
-- error: 'Failed to create function ''func31933'': function id is too big'
+- error: 'Failed to create function ''func31936'': function id is too big'
 ...
 drop_limit_func()
 ---
-- error: Function 'func31933' does not exist
+- error: Function 'func31936' does not exist
 ...
 session.su('admin')
 ---

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 4/6] alter: parse data dictionary version
  2021-08-05 22:17   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-06 19:47     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-06 19:47 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review! My answer, diff and newpatch below.

On Fri, Aug 06, 2021 at 12:17:30AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/schema.cc b/src/box/schema.cc
> > index 963278b19..35026619a 100644
> > --- a/src/box/schema.cc
> > +++ b/src/box/schema.cc
> > @@ -70,6 +71,9 @@ uint32_t schema_version = 0;
> >   */
> >  uint32_t space_cache_version = 0;
> >  
> > +/** Persistent version of the schema, stored in _schema["version"]. */
> > +uint32_t dd_version_id = tarantool_version_id();
> 
> I propose to assign it to 0. Because at start there is no
> schema at all until box.cfg{} is called first time. (Also then
> version.h include could be dropped above.)
> 
Thanks! Fixed.

> > +
> >  struct rlist on_schema_init = RLIST_HEAD_INITIALIZER(on_schema_init);
> >  struct rlist on_alter_space = RLIST_HEAD_INITIALIZER(on_alter_space);
> >  struct rlist on_alter_sequence = RLIST_HEAD_INITIALIZER(on_alter_sequence);


Diff:

diff --git a/src/box/schema.cc b/src/box/schema.cc
index 35026619a..5659e15b7 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -38,7 +38,6 @@
 #include "user.h"
 #include "vclock/vclock.h"
 #include "fiber.h"
-#include "version.h"
 
 /**
  * @module Data Dictionary
@@ -72,7 +71,7 @@ uint32_t schema_version = 0;
 uint32_t space_cache_version = 0;
 
 /** Persistent version of the schema, stored in _schema["version"]. */
-uint32_t dd_version_id = tarantool_version_id();
+uint32_t dd_version_id = 0;
 
 struct rlist on_schema_init = RLIST_HEAD_INITIALIZER(on_schema_init);
 struct rlist on_alter_space = RLIST_HEAD_INITIALIZER(on_alter_space);


Patch:

commit ae9274f3f0ee8de36b8060c1236aa0d4279a5172
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Tue Sep 12 17:46:25 2017 +0300

    alter: parse data dictionary version
    
    Version is needed to disallow creation of SQL built-in functions using
    _func starting with 2.9.0.
    
    Needed for #6106

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 935790df4..217b882ba 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4167,6 +4167,24 @@ on_replace_dd_schema(struct trigger * /* trigger */, void *event)
 			return -1;
 		REPLICASET_UUID = uu;
 		say_info("cluster uuid %s", tt_uuid_str(&uu));
+	} else if (strcmp(key, "version") == 0) {
+		if (new_tuple != NULL) {
+			uint32_t major, minor, patch;
+			if (tuple_field_u32(new_tuple, 1, &major) != 0 ||
+			    tuple_field_u32(new_tuple, 2, &minor) != 0)
+				tnt_raise(ClientError, ER_WRONG_DD_VERSION);
+			/* Version can be major.minor with no patch. */
+			if (tuple_field_u32(new_tuple, 3, &patch) != 0)
+				patch = 0;
+			dd_version_id = version_id(major, minor, patch);
+		} else {
+			assert(old_tuple != NULL);
+			/*
+			 * _schema:delete({'version'}) for
+			 * example, for box.internal.bootstrap().
+			 */
+			dd_version_id = tarantool_version_id();
+		}
 	}
 	return 0;
 }
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 963278b19..5659e15b7 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -70,6 +70,9 @@ uint32_t schema_version = 0;
  */
 uint32_t space_cache_version = 0;
 
+/** Persistent version of the schema, stored in _schema["version"]. */
+uint32_t dd_version_id = 0;
+
 struct rlist on_schema_init = RLIST_HEAD_INITIALIZER(on_schema_init);
 struct rlist on_alter_space = RLIST_HEAD_INITIALIZER(on_alter_space);
 struct rlist on_alter_sequence = RLIST_HEAD_INITIALIZER(on_alter_sequence);
diff --git a/src/box/schema.h b/src/box/schema.h
index 25ac6f110..d3bbdd590 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -44,6 +44,7 @@ struct func;
 
 extern uint32_t schema_version;
 extern uint32_t space_cache_version;
+extern uint32_t dd_version_id;
 
 /** Triggers invoked after schema initialization. */
 extern struct rlist on_schema_init;

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 5/6] alter: disallow creation of SQL built-in function
  2021-08-05 22:18   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-06 19:54     ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-06 19:54 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review! My answers, diff and new patch below.

On Fri, Aug 06, 2021 at 12:18:22AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/alter.cc b/src/box/alter.cc
> > index 217b882ba..fd9921ae0 100644
> > --- a/src/box/alter.cc
> > +++ b/src/box/alter.cc
> > @@ -3213,6 +3213,36 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * Check if the version of the data dictionary is lower than 2.9.0 and return
> > + * new func def if it is the case. If it is the case, then it is possible to
> > + * insert values with the "SQL_BUILTIN" language into _func, otherwise it is
> > + * prohibited. This is for upgradeability from 2.1.3 to 2.3.0. Since all we need
> > + * is to allow such inserts, we set func def to its default values.
> > + */
> > +static int
> > +func_def_new_sql_built_in(struct func_def *def)
> 
> 1. 'new' stands for new memory allocation. Here you need to use
> 'create'.
> 
Fixed.

> > +{
> > +	if (dd_version_id >= version_id(2, 9, 0)) {
> > +		diag_set(ClientError, ER_FUNCTION_LANGUAGE, "SQL_BUILTIN",
> > +			 def->name);
> > +		return -1;
> > +	}
> > +	def->body = NULL;
> > +	def->comment = NULL;
> > +	def->setuid = 1;
> > +	def->is_deterministic = false;
> > +	def->is_sandboxed = false;
> > +	def->param_count = 0;
> > +	def->returns = FIELD_TYPE_ANY;
> > +	def->aggregate = FUNC_AGGREGATE_NONE;
> > +	def->language = FUNC_LANGUAGE_LUA;
> > +	def->exports.lua = true;
> > +	def->exports.sql = true;
> > +	func_opts_create(&def->opts);
> > +	return 0;
> > +}
> > +
> >  /**
> >   * Get function identifiers from a tuple.
> >   *
> > @@ -3344,6 +3374,14 @@ func_def_new_from_tuple(struct tuple *tuple)
> >  				  language, def->name);
> >  			return NULL;
> >  		}
> > +		if (def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
> 
> 2. Is it possible to just skip such functions when the schema is old?
> Simply not create anything for them assuming that they will be deleted
> right afterwards. Not even store them in the func hash. Like they do
> not exist. Then you can also drop sql_builtin support from
> func_def_check().

It is possible to not create new functions, but in this case inserting into
_priv will result in an error or assertion (see #6295). In any case, the
upgrade will fail. Also, I change the language in def to LUA, so there really
is no problem dropping support for sql_builtin from func_def_check(). I did it
in the next patch.


Diff:

diff --git a/src/box/alter.cc b/src/box/alter.cc
index fd9921ae0..8a4f0b5a6 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3221,7 +3221,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
  * is to allow such inserts, we set func def to its default values.
  */
 static int
-func_def_new_sql_built_in(struct func_def *def)
+func_def_create_sql_built_in(struct func_def *def)
 {
 	if (dd_version_id >= version_id(2, 9, 0)) {
 		diag_set(ClientError, ER_FUNCTION_LANGUAGE, "SQL_BUILTIN",
@@ -3375,7 +3375,7 @@ func_def_new_from_tuple(struct tuple *tuple)
 			return NULL;
 		}
 		if (def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
-			if (func_def_new_sql_built_in(def) != 0)
+			if (func_def_create_sql_built_in(def) != 0)
 				return NULL;
 			if (func_def_check(def) != 0)
 				return NULL;


New patch:

commit f0551d7cd7fe5e776fefebb74d0b2f36dac6ca48
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Aug 4 11:18:46 2021 +0300

    alter: disallow creation of SQL built-in function
    
    This patch prohibits creation of user-defined functions with SQL_BUILTIN
    engine.
    
    Closes #6106

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 217b882ba..8a4f0b5a6 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3213,6 +3213,36 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 	return 0;
 }
 
+/**
+ * Check if the version of the data dictionary is lower than 2.9.0 and return
+ * new func def if it is the case. If it is the case, then it is possible to
+ * insert values with the "SQL_BUILTIN" language into _func, otherwise it is
+ * prohibited. This is for upgradeability from 2.1.3 to 2.3.0. Since all we need
+ * is to allow such inserts, we set func def to its default values.
+ */
+static int
+func_def_create_sql_built_in(struct func_def *def)
+{
+	if (dd_version_id >= version_id(2, 9, 0)) {
+		diag_set(ClientError, ER_FUNCTION_LANGUAGE, "SQL_BUILTIN",
+			 def->name);
+		return -1;
+	}
+	def->body = NULL;
+	def->comment = NULL;
+	def->setuid = 1;
+	def->is_deterministic = false;
+	def->is_sandboxed = false;
+	def->param_count = 0;
+	def->returns = FIELD_TYPE_ANY;
+	def->aggregate = FUNC_AGGREGATE_NONE;
+	def->language = FUNC_LANGUAGE_LUA;
+	def->exports.lua = true;
+	def->exports.sql = true;
+	func_opts_create(&def->opts);
+	return 0;
+}
+
 /**
  * Get function identifiers from a tuple.
  *
@@ -3344,6 +3374,14 @@ func_def_new_from_tuple(struct tuple *tuple)
 				  language, def->name);
 			return NULL;
 		}
+		if (def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
+			if (func_def_create_sql_built_in(def) != 0)
+				return NULL;
+			if (func_def_check(def) != 0)
+				return NULL;
+			def_guard.is_active = false;
+			return def;
+		}
 	} else {
 		/* Lua is the default. */
 		def->language = FUNC_LANGUAGE_LUA;
diff --git a/test/box/function1.result b/test/box/function1.result
index a49a133f7..a1c89850d 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -372,7 +372,7 @@ c:close()
 box.schema.func.create('WAITFOR', {language = 'SQL_BUILTIN', \
 	param_list = {'integer'}, returns = 'integer',exports = {'SQL'}})
 ---
-- error: 'Failed to create function ''WAITFOR'': given built-in is not predefined'
+- error: Unsupported language 'SQL_BUILTIN' specified for function 'WAITFOR'
 ...
 test_run:cmd("setopt delimiter ';'")
 ---
@@ -1078,3 +1078,8 @@ box.func['test'].is_multikey == true
 box.func['test']:drop()
 ---
 ...
+-- gh-6106: Check that user-defined functions cannot have SQL_BUILTIN engine.
+box.schema.func.create("ABS", {language = 'SQL_BUILTIN', returns = 'integer'})
+---
+- error: Unsupported language 'SQL_BUILTIN' specified for function 'ABS'
+...
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index 4fdd48520..e635b6e18 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -389,3 +389,6 @@ box.func.LUA:call({"return 1 + 1"})
 box.schema.func.create('test', {body = "function(tuple) return tuple end", is_deterministic = true, opts = {is_multikey = true}})
 box.func['test'].is_multikey == true
 box.func['test']:drop()
+
+-- gh-6106: Check that user-defined functions cannot have SQL_BUILTIN engine.
+box.schema.func.create("ABS", {language = 'SQL_BUILTIN', returns = 'integer'})

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 6/6] sql: remove unnecessary function initialization
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 6/6] sql: remove unnecessary function initialization Mergen Imeev via Tarantool-patches
@ 2021-08-06 19:59   ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-06 19:59 UTC (permalink / raw)
  To: v.shpilevoy, tarantool-patches

I removed SQL built-ins support from func_def_check(). Diff and new patch below.

On Wed, Aug 04, 2021 at 03:58:47PM +0300, Mergen Imeev via Tarantool-patches wrote:
> After removing the SQL built-in functions from _func, the code used to
> initialize these SQL built-in functions is no longer used and should be
> removed.
> 
> Follow-up #6106
> ---
>  src/box/func.c     |  7 -------
>  src/box/sql/func.c | 49 ----------------------------------------------
>  2 files changed, 56 deletions(-)
> 
> diff --git a/src/box/func.c b/src/box/func.c
> index 731f18a3b..04ae1b958 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -394,10 +394,6 @@ restore_fail:
>  static struct func *
>  func_c_new(struct func_def *def);
>  
> -/** Construct a SQL builtin function object. */
> -extern struct func *
> -func_sql_builtin_new(struct func_def *def);
> -
>  struct func *
>  func_new(struct func_def *def)
>  {
> @@ -409,9 +405,6 @@ func_new(struct func_def *def)
>  	case FUNC_LANGUAGE_LUA:
>  		func = func_lua_new(def);
>  		break;
> -	case FUNC_LANGUAGE_SQL_BUILTIN:
> -		func = func_sql_builtin_new(def);
> -		break;
>  	default:
>  		unreachable();
>  	}
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 2a3a5d457..50014b756 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -2789,55 +2789,6 @@ sql_built_in_functions_cache_free(void)
>  	mh_strnptr_delete(built_in_functions);
>  }
>  
> -struct func *
> -func_sql_builtin_new(struct func_def *def)
> -{
> -	assert(def->language == FUNC_LANGUAGE_SQL_BUILTIN);
> -	/** Binary search for corresponding builtin entry. */
> -	int idx = -1, left = 0, right = nelem(sql_builtins) - 1;
> -	while (left <= right) {
> -		uint32_t mid = (left + right) / 2;
> -		int rc = strcmp(def->name, sql_builtins[mid].name);
> -		if (rc == 0) {
> -			idx = mid;
> -			break;
> -		}
> -		if (rc < 0)
> -			right = mid - 1;
> -		else
> -			left = mid + 1;
> -	}
> -	/*
> -	 * All SQL built-in(s) (stubs) are defined in a snapshot.
> -	 * Implementation-specific metadata is defined in
> -	 * sql_builtins list. When a definition were not found
> -	 * above, the function name is invalid, i.e. it is
> -	 * not built-in function.
> -	 */
> -	if (idx == -1) {
> -		diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
> -			 "given built-in is not predefined");
> -		return NULL;
> -	}
> -	struct func_sql_builtin *func =
> -		(struct func_sql_builtin *) calloc(1, sizeof(*func));
> -	if (func == NULL) {
> -		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
> -		return NULL;
> -	}
> -	func->base.def = def;
> -	func->base.vtab = &func_sql_builtin_vtab;
> -	func->flags = sql_builtins[idx].flags;
> -	func->call = sql_builtins[idx].call;
> -	func->finalize = sql_builtins[idx].finalize;
> -	def->param_count = sql_builtins[idx].param_count;
> -	def->is_deterministic = sql_builtins[idx].is_deterministic;
> -	def->returns = sql_builtins[idx].returns;
> -	def->aggregate = sql_builtins[idx].aggregate;
> -	def->exports.sql = sql_builtins[idx].export_to_sql;
> -	return &func->base;
> -}
> -
>  static void
>  func_sql_builtin_destroy(struct func *func)
>  {
> -- 
> 2.25.1
> 


Diff:

diff --git a/src/box/func_def.c b/src/box/func_def.c
index 11d2bdb84..630b4a43c 100644
--- a/src/box/func_def.c
+++ b/src/box/func_def.c
@@ -116,14 +116,6 @@ func_def_check(struct func_def *def)
 			return -1;
 		}
 		break;
-	case FUNC_LANGUAGE_SQL_BUILTIN:
-		if (def->body != NULL || def->is_sandboxed) {
-			diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
-				 "body and is_sandboxed options are not compatible "
-				 "with SQL language");
-			return -1;
-		}
-		break;
 	default:
 		break;
 	}



New patch:

commit 79871a45476aca400ecb921a44550a3807c309ca
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Jul 31 13:09:07 2021 +0300

    sql: remove unnecessary function initialization
    
    After removing the SQL built-in functions from _func, the code used to
    initialize these SQL built-in functions is no longer used and should be
    removed.
    
    Follow-up #6106

diff --git a/src/box/func.c b/src/box/func.c
index 731f18a3b..04ae1b958 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -394,10 +394,6 @@ restore_fail:
 static struct func *
 func_c_new(struct func_def *def);
 
-/** Construct a SQL builtin function object. */
-extern struct func *
-func_sql_builtin_new(struct func_def *def);
-
 struct func *
 func_new(struct func_def *def)
 {
@@ -409,9 +405,6 @@ func_new(struct func_def *def)
 	case FUNC_LANGUAGE_LUA:
 		func = func_lua_new(def);
 		break;
-	case FUNC_LANGUAGE_SQL_BUILTIN:
-		func = func_sql_builtin_new(def);
-		break;
 	default:
 		unreachable();
 	}
diff --git a/src/box/func_def.c b/src/box/func_def.c
index 11d2bdb84..630b4a43c 100644
--- a/src/box/func_def.c
+++ b/src/box/func_def.c
@@ -116,14 +116,6 @@ func_def_check(struct func_def *def)
 			return -1;
 		}
 		break;
-	case FUNC_LANGUAGE_SQL_BUILTIN:
-		if (def->body != NULL || def->is_sandboxed) {
-			diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
-				 "body and is_sandboxed options are not compatible "
-				 "with SQL language");
-			return -1;
-		}
-		break;
 	default:
 		break;
 	}
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 2a3a5d457..50014b756 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -2789,55 +2789,6 @@ sql_built_in_functions_cache_free(void)
 	mh_strnptr_delete(built_in_functions);
 }
 
-struct func *
-func_sql_builtin_new(struct func_def *def)
-{
-	assert(def->language == FUNC_LANGUAGE_SQL_BUILTIN);
-	/** Binary search for corresponding builtin entry. */
-	int idx = -1, left = 0, right = nelem(sql_builtins) - 1;
-	while (left <= right) {
-		uint32_t mid = (left + right) / 2;
-		int rc = strcmp(def->name, sql_builtins[mid].name);
-		if (rc == 0) {
-			idx = mid;
-			break;
-		}
-		if (rc < 0)
-			right = mid - 1;
-		else
-			left = mid + 1;
-	}
-	/*
-	 * All SQL built-in(s) (stubs) are defined in a snapshot.
-	 * Implementation-specific metadata is defined in
-	 * sql_builtins list. When a definition were not found
-	 * above, the function name is invalid, i.e. it is
-	 * not built-in function.
-	 */
-	if (idx == -1) {
-		diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
-			 "given built-in is not predefined");
-		return NULL;
-	}
-	struct func_sql_builtin *func =
-		(struct func_sql_builtin *) calloc(1, sizeof(*func));
-	if (func == NULL) {
-		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
-		return NULL;
-	}
-	func->base.def = def;
-	func->base.vtab = &func_sql_builtin_vtab;
-	func->flags = sql_builtins[idx].flags;
-	func->call = sql_builtins[idx].call;
-	func->finalize = sql_builtins[idx].finalize;
-	def->param_count = sql_builtins[idx].param_count;
-	def->is_deterministic = sql_builtins[idx].is_deterministic;
-	def->returns = sql_builtins[idx].returns;
-	def->aggregate = sql_builtins[idx].aggregate;
-	def->exports.sql = sql_builtins[idx].export_to_sql;
-	return &func->base;
-}
-
 static void
 func_sql_builtin_destroy(struct func *func)
 {

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func
  2021-08-04 12:58 [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
                   ` (5 preceding siblings ...)
  2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 6/6] sql: remove unnecessary function initialization Mergen Imeev via Tarantool-patches
@ 2021-08-08 12:08 ` Vladislav Shpilevoy via Tarantool-patches
  6 siblings, 0 replies; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-08 12:08 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the fixes!

LGTM.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Tarantool-patches] [PATCH v2 6/6] sql: remove unnecessary function initialization
  2021-08-09  7:18 Mergen Imeev via Tarantool-patches
@ 2021-08-09  7:19 ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-09  7:19 UTC (permalink / raw)
  To: kyukhin; +Cc: tarantool-patches

After removing the SQL built-in functions from _func, the code used to
initialize these SQL built-in functions is no longer used and should be
removed.

Follow-up #6106
---
 src/box/func.c     |  7 -------
 src/box/func_def.c |  8 --------
 src/box/sql/func.c | 49 ----------------------------------------------
 3 files changed, 64 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 731f18a3b..04ae1b958 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -394,10 +394,6 @@ restore_fail:
 static struct func *
 func_c_new(struct func_def *def);
 
-/** Construct a SQL builtin function object. */
-extern struct func *
-func_sql_builtin_new(struct func_def *def);
-
 struct func *
 func_new(struct func_def *def)
 {
@@ -409,9 +405,6 @@ func_new(struct func_def *def)
 	case FUNC_LANGUAGE_LUA:
 		func = func_lua_new(def);
 		break;
-	case FUNC_LANGUAGE_SQL_BUILTIN:
-		func = func_sql_builtin_new(def);
-		break;
 	default:
 		unreachable();
 	}
diff --git a/src/box/func_def.c b/src/box/func_def.c
index 11d2bdb84..630b4a43c 100644
--- a/src/box/func_def.c
+++ b/src/box/func_def.c
@@ -116,14 +116,6 @@ func_def_check(struct func_def *def)
 			return -1;
 		}
 		break;
-	case FUNC_LANGUAGE_SQL_BUILTIN:
-		if (def->body != NULL || def->is_sandboxed) {
-			diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
-				 "body and is_sandboxed options are not compatible "
-				 "with SQL language");
-			return -1;
-		}
-		break;
 	default:
 		break;
 	}
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 2a3a5d457..50014b756 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -2789,55 +2789,6 @@ sql_built_in_functions_cache_free(void)
 	mh_strnptr_delete(built_in_functions);
 }
 
-struct func *
-func_sql_builtin_new(struct func_def *def)
-{
-	assert(def->language == FUNC_LANGUAGE_SQL_BUILTIN);
-	/** Binary search for corresponding builtin entry. */
-	int idx = -1, left = 0, right = nelem(sql_builtins) - 1;
-	while (left <= right) {
-		uint32_t mid = (left + right) / 2;
-		int rc = strcmp(def->name, sql_builtins[mid].name);
-		if (rc == 0) {
-			idx = mid;
-			break;
-		}
-		if (rc < 0)
-			right = mid - 1;
-		else
-			left = mid + 1;
-	}
-	/*
-	 * All SQL built-in(s) (stubs) are defined in a snapshot.
-	 * Implementation-specific metadata is defined in
-	 * sql_builtins list. When a definition were not found
-	 * above, the function name is invalid, i.e. it is
-	 * not built-in function.
-	 */
-	if (idx == -1) {
-		diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
-			 "given built-in is not predefined");
-		return NULL;
-	}
-	struct func_sql_builtin *func =
-		(struct func_sql_builtin *) calloc(1, sizeof(*func));
-	if (func == NULL) {
-		diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
-		return NULL;
-	}
-	func->base.def = def;
-	func->base.vtab = &func_sql_builtin_vtab;
-	func->flags = sql_builtins[idx].flags;
-	func->call = sql_builtins[idx].call;
-	func->finalize = sql_builtins[idx].finalize;
-	def->param_count = sql_builtins[idx].param_count;
-	def->is_deterministic = sql_builtins[idx].is_deterministic;
-	def->returns = sql_builtins[idx].returns;
-	def->aggregate = sql_builtins[idx].aggregate;
-	def->exports.sql = sql_builtins[idx].export_to_sql;
-	return &func->base;
-}
-
 static void
 func_sql_builtin_destroy(struct func *func)
 {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-08-09  7:22 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-04 12:58 [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 1/6] sql: introduce sql_func_flags() Mergen Imeev via Tarantool-patches
2021-08-05 22:14   ` Vladislav Shpilevoy via Tarantool-patches
2021-08-06 19:41     ` Mergen Imeev via Tarantool-patches
2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 2/6] sql: introduce sql_func_find() Mergen Imeev via Tarantool-patches
2021-08-05 22:15   ` Vladislav Shpilevoy via Tarantool-patches
2021-08-06 19:42     ` Mergen Imeev via Tarantool-patches
2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 3/6] sql: remove SQL built-in functions from _func Mergen Imeev via Tarantool-patches
2021-08-05 22:17   ` Vladislav Shpilevoy via Tarantool-patches
2021-08-06 19:45     ` Mergen Imeev via Tarantool-patches
2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 4/6] alter: parse data dictionary version Mergen Imeev via Tarantool-patches
2021-08-05 22:17   ` Vladislav Shpilevoy via Tarantool-patches
2021-08-06 19:47     ` Mergen Imeev via Tarantool-patches
2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 5/6] alter: disallow creation of SQL built-in function Mergen Imeev via Tarantool-patches
2021-08-05 22:18   ` Vladislav Shpilevoy via Tarantool-patches
2021-08-06 19:54     ` Mergen Imeev via Tarantool-patches
2021-08-04 12:58 ` [Tarantool-patches] [PATCH v2 6/6] sql: remove unnecessary function initialization Mergen Imeev via Tarantool-patches
2021-08-06 19:59   ` Mergen Imeev via Tarantool-patches
2021-08-08 12:08 ` [Tarantool-patches] [PATCH v2 0/6] Remove SQL built-in functions from _func Vladislav Shpilevoy via Tarantool-patches
2021-08-09  7:18 Mergen Imeev via Tarantool-patches
2021-08-09  7:19 ` [Tarantool-patches] [PATCH v2 6/6] sql: remove unnecessary function initialization Mergen Imeev via Tarantool-patches

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