From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E00DB21399 for ; Tue, 10 Sep 2019 17:22:41 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Qz_D5DviYiCi for ; Tue, 10 Sep 2019 17:22:41 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 31F54212FC for ; Tue, 10 Sep 2019 17:22:41 -0400 (EDT) Date: Wed, 11 Sep 2019 00:22:37 +0300 From: Nikita Pettik Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: disallow ck using non-persistent function Message-ID: <20190910212237.GA27128@tarantool.org> References: <33fa81dad5745292133063d39a3c20144718bb60.1567496399.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <33fa81dad5745292133063d39a3c20144718bb60.1567496399.git.kshcherbatov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org 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 >