[tarantool-patches] Re: [PATCH v1 1/1] sql: disallow ck using non-persistent function

[Nikita Pettik]

On Tue, Sep 03, 2019 at 10:40:43AM +0300, Kirill Shcherbatov wrote:
> Each CK constraint object is a part of the database schema and
> is restored during recovery. It is not possible if a CK
> constraint uses some function inside.

Nit: not some, but user-defined (built-ins are still available).

>Thus we should disallow
> non-persistent functions participate in ck constraints.
> 
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 6f625dc18..3b4f01588 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -653,6 +653,18 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
>  				pExpr->iTable = func->def->name[0] == 'u' ?
>  						8388608 : 125829120;
>  			}
> +			if ((pNC->ncFlags & NC_IsCheck) != 0 &&
> +			    func->def->body == NULL &&
> +			    func->def->language != FUNC_LANGUAGE_SQL_BUILTIN) {
> +				diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +					 tt_sprintf("ck constraint could not "
> +						    "use non-persistent "
> +						    "function '%s'",
> +						    func->def->name));
> +		

Nit: "Check constraint can not invoke non-persistent function" - sounds better.

>		pParse->is_aborted = true;
> +				pNC->nErr++;
> +				return WRC_Abort;
> +			}

Ok, but still user can create persistent function, create check
constraint involving that function and then drop function. Voila - 
Tarantool crashes.

box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'}, param_list = {'integer'}, body = "function(x) return x < 10 end"})
box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
box.schema.func.drop("MYFUNC")
box.space.T6:insert({1})
Segmentation fault
  code: 0
  addr: 0x0
  context: 0x11197e9c8
  siginfo: 0x11197e960

Unfortunatelly, function is not bound to space(s), so we can't rebuild
only affected check constraints (like during format change). The only
option in this case rebuild all check constraints, but this solution
doesn't seem to be acceptable.

On the other hand, we can use the same approatch as for views: hold
ar eference counter in each function and increment it each time
when function is involved in check constraint (and in the future - 
in trigger). And disallow dropping function if its reference counter
doesn't equal to zero.

Please, come up with alternatives to this approach.

> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index cde213f8b..8ec3df84e 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -235,3 +235,20 @@ physics_ck
>  physics_ck:drop()
>  
>  test_run:cmd("clear filter")
> +
> +--
> +-- gh-4176: Can't recover if check constraint involves function.
> +--

Nit: please, write comment to test in form of "Make sure that ...".
For instance: "Make sure that non-persistent functions can't participate
in check constraints, since after instance reboot they disappear and
check constraint can't be created." Using this wording allows to
avoid 

> +function myfunc(x) return x < 10 end
> +box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'}, param_list = {'integer'}})
> +box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
> +box.func.MYFUNC:drop()
> +
> +box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'}, param_list = {'integer'}, body = "function(x) return x < 10 end"})
> +box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
> +box.space.T6:insert({11})
> +test_run:cmd("restart server default")
> +box.space.T6:insert({11})
> +
> +box.space.T6:drop()
> +box.func.MYFUNC:drop()
> -- 
> 2.22.1
>