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 1F68521B32 for ; Mon, 28 May 2018 07:19: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 Hloksxddsp1X for ; Mon, 28 May 2018 07:19:15 -0400 (EDT) Received: from smtp18.mail.ru (smtp18.mail.ru [94.100.176.155]) (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 6313321AEC for ; Mon, 28 May 2018 07:19:13 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server References: <2baee418a3799c91c5a8b92ac62e59de3456da0d.1527084287.git.kshcherbatov@tarantool.org> <4221becc-9d9c-852a-ccd1-6ed3618cde9f@tarantool.org> <6b860615-cd91-414a-3948-32600b69e05f@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 28 May 2018 14:19:11 +0300 MIME-Version: 1.0 In-Reply-To: <6b860615-cd91-414a-3948-32600b69e05f@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 , tarantool-patches@freelists.org Thanks for the fixes! See my 3 comments below. And I have pushed several review fix commits. Look at them and squash. > > >> box.cfg{} >> opts = {checks = {{exp = 'X<5'}}} >> format = {{name = 'X', type = 'unsigned'}} >> t = {513, 1, 'test', 'memtx', 0, opts, format} >> box.space._space:insert(t) 1. I do not see a test on it. > >> 10. deleteTable leaks with the same reason: you delete space_def only when >> opts.is_temporary flag is set. Including checks, that are on malloc. >> 11. Why do you space->def->opts.checks here? I scanned sql_view_column_names, >> and looks like it does not require compiled check. It needs only check names >> and check span. Table.def has both, it is not? >>> + /* >>> + * As rebuild creates a new ExpList tree and old_def >>> + * is allocated on region release old tree manually. >>> + */ >>> + struct ExprList *old_checks = p->def->opts.checks; >>> if (sql_table_def_rebuild(db, p) != 0) >>> return; > >>> + /* Get server checks. */ >>> + ExprList *checks = space_checks_expr_list(table->def->id); >> > We don't replicate checks in SQL. At the end of sqlite3EndTable checks are released and the only checks collection present on the server side. 2. Are you sure? I added assert(def->checks == NULL) in deleteTable() and it fails: box.sql.execute("CREATE TABLE t1(x INTEGER CHECK( x<5 ), y REAL CHECK( y>x ), z primary key, invalid:parameter);") Assertion failed: (pTable->def->opts.checks == NULL), function deleteTable, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/build.c, line 414. Process 24676 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT frame #0: 0x00007fff5eed8b6e libsystem_kernel.dylib`__pthread_kill + 10 libsystem_kernel.dylib`__pthread_kill: -> 0x7fff5eed8b6e <+10>: jae 0x7fff5eed8b78 ; <+20> 0x7fff5eed8b70 <+12>: movq %rax, %rdi 0x7fff5eed8b73 <+15>: jmp 0x7fff5eecfb00 ; cerror_nocancel 0x7fff5eed8b78 <+20>: retq > I've wrote comments about this in sqlite3EndTable: > > /* > * As rebuild creates a new ExpList tree and old_def > * allocated on region release old tree manually. > */ > /* > * Checks are useless for now as all operations process with > * the server checks instance. Remove to do not consume extra memory, > * don't require make a copy on space_def_dup and to improve > * debuggability. > */ >> >> 12. Please, write tests on checks insertion from Lua. I know such checks will >> not work, but at least you should do sanity test. You should test, that a user >> can insert a check with no name, that a check can be compiled even if no SQL >> Table, that a check expr can not be number or bool and the same about name. >> In the same tests you can check errors on non-existing column names. 3. One test is not enough. You did not test my example with 'exp' above, you did not test 'expr' type mismatch, unexpected options in a check (expr + name + some unexpected one) etc.