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 D080529C44 for ; Tue, 16 Apr 2019 09:51:15 -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 pQV5vsIyZ-6X for ; Tue, 16 Apr 2019 09:51:15 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 85ADE29BDD for ; Tue, 16 Apr 2019 09:51:15 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 9/9] sql: run check constraint tests on space alter References: <9ba32d3784fdf28096b8ccde7d5fb3c6a55c0826.1548838034.git.kshcherbatov@tarantool.org> <383ecdc1-35d8-7a15-fe7c-e1cdba941dfd@tarantool.org> <121C5F34-691B-4FCC-A1C9-C995683D4A6A@tarantool.org> From: Kirill Shcherbatov Message-ID: <036f8814-67a1-916c-7935-32e50fa14b07@tarantool.org> Date: Tue, 16 Apr 2019 16:51:13 +0300 MIME-Version: 1.0 In-Reply-To: <121C5F34-691B-4FCC-A1C9-C995683D4A6A@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: tarantool-patches@freelists.org, "n.pettik" > Mention that format is required condition to use check > constraints: otherwise, name of field can’t be resolved. > Now it leads to assertion fault: > > s = box.schema.create_space('test’) > s:create_index('pk’) > box.space._ck_constraint:insert({'physics', s.id, 'X Assertion failed: (space_def->field_count > 0), function lookupName, file tarantool/src/box/sql/resolve.c, line 242. Thank you. Fixed it in previous patch. > >> box.space._ck_constraint:insert({'physics', s.id, 'X> box.space.test:insert({6, 5}) >> - error: 'Check constraint failed: physics’ > > To finish this patch-set I suggest to add Lua-wrapper to create > check constraints on any space using NoSQL interface and introduce > ALTER TABLE ADD CONSTRAINT CHECK(). > Last issue you can implement in a separate patch or delegate its > implementation to smb else. Consider my new commit. >> + vdbe_emit_ck_constraint(&parser, expr, ck_constraint->def->name, >> + new_tuple_reg); >> + sql_finish_coding(&parser); > > Do we need to call this function at all? Yes. I've tried to drop it. > >> + if (parser.is_aborted) { >> + diag_set(ClientError, ER_CREATE_CK_CONSTRAINT, >> + ck_constraint->def->name, >> + "can not compile expression”); > > This error will re-set original parsing error. I suggest to > concatenate them. Done. >> + sql_reset(ck_constraint->stmt); > > This function returns error code, don’t ignore it. Now I use it's result to determine execution state at the end of trigger. >> + parser->ckBase = new_tuple_reg; >> + struct Vdbe *v = sqlGetVdbe(parser); >> + const char *ck_constraint_name = sqlDbStrDup(parser->db, name); > > Where's this pointer released? > >> + VdbeNoopComment((v, "BEGIN: ck constraint %s test", name)); >> + /* Skip check when it is turned off. */ Now this string is a part of VdbeNoopComment so VDBE must release it. > When it can be turned off? Outdated comment. > Please, add descent set of tests verifying that check constraints > work in any possible scenario. > Make sure that check occurs before replace action. Many new good tests are the part of previous patch now.