[tarantool-patches] Re: [PATCH v1 1/1] sql: prevent executing crossengine sql

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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
>