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 5913727CB3 for ; Fri, 20 Jul 2018 11:03:33 -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 FhfElqjcdB6N for ; Fri, 20 Jul 2018 11:03:33 -0400 (EDT) Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (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 1550427CA2 for ; Fri, 20 Jul 2018 11:03:33 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: prevent executing crossengine sql References: <6134a0afdcfba13ca289e2d42fdd529a2787070a.1532094680.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <50ac96b7-d395-f528-f66b-c10847e54670@tarantool.org> Date: Fri, 20 Jul 2018 18:03:30 +0300 MIME-Version: 1.0 In-Reply-To: <6134a0afdcfba13ca289e2d42fdd529a2787070a.1532094680.git.kshcherbatov@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 Hi! Thanks for the good diligent attempt to fix the bug, the patch looks really cool except a pair of minor things, but I think, that it is fatally flawed because 1) It still do not begin_ro_stmt though Vinyl needs it to send iterators and transactions into read view; 2) Only opcodes analysis does not help when we have multiple statements each parsed into own Vdbe and then they are executed under the transaction. Example (yes, we do not have PREPARE, but we will do): p1 = PREPARE INSERT INTO memtx_space VALUES (1, 2, 3); p2 = PREPARE SELECT * FROM vinyl_space; -- In both no cross-engine things are detected. -- Now execute them together. START TRANSACTION; EXECUTE p1; EXECUTE p2; -- Fail. COMMIT; So we should use txn_begin_ro_stmt for iterators. Strictly speaking, it crashes already now, even after the patch applied: box.cfg{} box.sql.execute("CREATE TABLE test (id INT PRIMARY KEY)") box.sql.execute("PRAGMA sql_default_engine='vinyl'") box.sql.execute("CREATE TABLE test2 (id INT PRIMARY KEY)") box.sql.execute("INSERT INTO test2 VALUES (2)") function f() box.sql.execute("START TRANSACTION") box.sql.execute("INSERT INTO test VALUES (1)") box.sql.execute("SELECT * FROM test2") box.sql.execute("COMMIT") end tarantool> f() Assertion failed: (tx == NULL || tx->state == VINYL_TX_READY), function vinyl_index_create_iterator, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/vinyl.c, line 4038. Process 6270 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT frame #0: 0x00007fff5fdcab66 libsystem_kernel.dylib`__pthread_kill + 10 libsystem_kernel.dylib`__pthread_kill: -> 0x7fff5fdcab66 <+10>: jae 0x7fff5fdcab70 ; <+20> 0x7fff5fdcab68 <+12>: movq %rax, %rdi 0x7fff5fdcab6b <+15>: jmp 0x7fff5fdc1ae9 ; cerror_nocancel 0x7fff5fdcab70 <+20>: retq Target 0: (tarantool) stopped. On 20/07/2018 16:52, Kirill Shcherbatov wrote: > Some sql requests are complex and could contain R/W with > multiple spaces. As we have no ability to make such changes > transactionaly, we have to dissallow such requests. This mean > that we shouldn't create triggers on memtex spaces writing > something in vinyl and so on. > Since now, such checks are carried out on sql_finish_coding, > the last parsing step. > Pay attention, that _sql_stat1 and _sql_stat4 are exceptions. > Most requests(excluding ANALYZE) use them only for READ and it > is ok. > > Closes #3551 > --- > Branch: http://github.com/tarantool/tarantool/tree/kshch/kshch/gh-3551-crossengine-sql > Issue: https://github.com/tarantool/tarantool/issues/3551 >