From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Kirill Shcherbatov <kshcherbatov@tarantool.org>, Nikita Pettik <korablev@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v1 2/2] sql: prevent executing crossengine sql Date: Thu, 26 Jul 2018 00:05:58 +0300 [thread overview] Message-ID: <12091997-2a63-951d-9477-86c3429d981d@tarantool.org> (raw) In-Reply-To: <a685a228-1758-85c2-eaee-403ea8cd1eb8@tarantool.org> Thanks for the patch! See 5 comments below. 1. Please, put a new patch version at the end of letter. > commit ae4310483bf126200f388be28a06b7580f3d9faa > Author: Kirill Shcherbatov <kshcherbatov@tarantool.org> > Date: Tue Jul 24 12:46:46 2018 +0300 > > sql: prevent executing cross-engine sql 2. crossengine looks like 'междудвижками'. Please, replace with cross-engine. > > 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.> Since now iterators in SQL start transaction so we cant prevent > such vicious and dangeros things. 3. 'dissalow', 'dangeros' - no such words. > > Closes #3551 > > diff --git a/src/box/sql.c b/src/box/sql.c > index d48c3cfe5..a964fcbb1 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1118,12 +1118,23 @@ cursor_seek(BtCursor *pCur, int *pRes) > return SQL_TARANTOOL_ITERATOR_FAIL; > } > > + struct space *space = pCur->space; > + struct txn *txn = NULL; > + > + if (space->def->id != 0) { > + if (txn_begin_ro_stmt(space, &txn) != 0) > + return SQL_TARANTOOL_ERROR; > + } > struct iterator *it = index_create_iterator(pCur->index, pCur->iter_type, > key, part_count); > if (it == NULL) { > + if (txn != NULL) > + txn_rollback_stmt(); > pCur->eState = CURSOR_INVALID; > return SQL_TARANTOOL_ITERATOR_FAIL; > } > + if (txn != NULL) > + txn_commit_ro_stmt(txn); 4. Both commit_ro and rollback_ro already check for txn != NULL so please, remove the redundant checks. > pCur->iter = it; > pCur->eState = CURSOR_VALID; > > diff --git a/test/sql/triggers.result b/test/sql/triggers.result > index dc0a2e57d..a692b8945 100644 > --- a/test/sql/triggers.result > +++ b/test/sql/triggers.result > @@ -246,3 +246,123 @@ box.sql.execute("DROP VIEW V1;") > box.sql.execute("DROP TABLE T1;") > --- > ... > +-- > +-- gh-3531: Assertion with trigger and two storage engines > +-- > +-- Case 1: Src 'vinyl' table; Dst 'memtx' table > +box.sql.execute("PRAGMA sql_default_engine ('vinyl');") > +--- > +... > +box.sql.execute("CREATE TABLE m (s1 SCALAR PRIMARY KEY);") > +--- > +... > +box.sql.execute("CREATE TRIGGER m1 BEFORE UPDATE ON m FOR EACH ROW BEGIN UPDATE n SET s2 = DATETIME('now'); END;") > +--- > +... > +box.sql.execute("PRAGMA sql_default_engine('memtx');") > +--- > +... > +box.sql.execute("CREATE TABLE n (s1 CHAR PRIMARY KEY, s2 char);") > +--- > +... > +box.sql.execute("INSERT INTO m VALUES ('');") > +--- > +... > +box.sql.execute("INSERT INTO n VALUES ('',null);") > +--- > +... > +box.sql.execute("UPDATE m SET s1 = 'The Rain In Spain';") > +--- > +- error: A multi-statement transaction can not use multiple storage engines > +... > +-- ANALYZE operates with _sql_stat{1,4} tables should work > +box.sql.execute("ANALYZE m;") > +--- > +... > +box.sql.execute("DROP TABLE m;") > +--- > +... > +box.sql.execute("DROP TABLE n;") > +--- > +... > +-- Case 2: Src 'memtx' table; Dst 'vinyl' table > +box.sql.execute("PRAGMA sql_default_engine ('memtx');") > +--- > +... > +box.sql.execute("CREATE TABLE m (s1 SCALAR PRIMARY KEY);") > +--- > +... > +box.sql.execute("CREATE TRIGGER m1 BEFORE UPDATE ON m FOR EACH ROW BEGIN UPDATE n SET s2 = DATETIME('now'); END;") > +--- > +... > +box.sql.execute("PRAGMA sql_default_engine('vinyl');") > +--- > +... > +box.sql.execute("CREATE TABLE n (s1 CHAR PRIMARY KEY, s2 char);") > +--- > +... > +box.sql.execute("INSERT INTO m VALUES ('');") > +--- > +... > +box.sql.execute("INSERT INTO n VALUES ('',null);") > +--- > +... > +box.sql.execute("UPDATE m SET s1 = 'The Rain In Spain';") > +--- > +- error: A multi-statement transaction can not use multiple storage engines > +... > +-- ANALYZE operates with _sql_stat{1,4} tables should work > +box.sql.execute("ANALYZE n;") > +--- > +... > +box.sql.execute("DROP TABLE m;") > +--- > +... > +box.sql.execute("DROP TABLE n;") > +--- > +... > +-- Test SQL Transaction with LUA > +box.sql.execute("PRAGMA sql_default_engine ('memtx');") > +--- > +... > +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)") > +--- > +... > +test_run:cmd("setopt delimiter ';'") > +--- > +- true > +... > +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; > +--- > +... > +f(); 5. Wit the same success you could inline f() and remove it. I gave this code to you into the function because I run it in the console. In test-run you anyway use "setopt delimiter" and can avoid this function enclosing. Or you even could put all this code in one line with no "setopt delimiter". There is no so many code. > +--- > +- error: A multi-statement transaction can not use multiple storage engines > +... > +box.sql.execute("ROLLBACK;"); > +--- > +... > +box.sql.execute("DROP TABLE test;"); > +--- > +... > +box.sql.execute("DROP TABLE test2;"); > +--- > +... > +test_run:cmd("setopt delimiter ''"); > +--- > +- true > +...
next prev parent reply other threads:[~2018-07-25 21:06 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-20 13:52 [tarantool-patches] [PATCH v1 1/1] " Kirill Shcherbatov 2018-07-20 15:03 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-20 17:50 ` Kirill Shcherbatov 2018-07-23 11:50 ` Vladislav Shpilevoy 2018-07-23 16:20 ` n.pettik 2018-07-23 16:39 ` Vladislav Shpilevoy 2018-07-23 17:09 ` n.pettik 2018-07-23 17:21 ` Vladislav Shpilevoy 2018-07-23 18:06 ` n.pettik 2018-07-23 18:29 ` Vladislav Shpilevoy 2018-07-24 11:05 ` [tarantool-patches] [PATCH v1 1/2] sql: use schema API to get index info in analyze Kirill Shcherbatov [not found] ` <cover.1532430181.git.kshcherbatov@tarantool.org> 2018-07-24 11:05 ` [tarantool-patches] [PATCH v1 2/2] sql: prevent executing crossengine sql Kirill Shcherbatov 2018-07-25 13:24 ` [tarantool-patches] " n.pettik 2018-07-25 17:07 ` Kirill Shcherbatov 2018-07-25 21:05 ` Vladislav Shpilevoy [this message] 2018-07-26 7:08 ` Kirill Shcherbatov 2018-07-26 8:54 ` Vladislav Shpilevoy 2018-07-26 11:22 ` Kirill Shcherbatov 2018-07-26 21:26 ` Vladislav Shpilevoy 2018-07-27 7:13 ` Kirill Shcherbatov 2018-07-27 8:55 ` Vladislav Shpilevoy 2018-07-27 10:02 ` Kirill Shcherbatov 2018-07-27 10:14 ` Vladislav Shpilevoy 2018-07-31 7:54 ` Kirill Yukhin 2018-07-25 13:22 ` [tarantool-patches] Re: [PATCH v1 1/2] sql: use schema API to get index info in analyze n.pettik 2018-07-25 17:07 ` Kirill Shcherbatov 2018-07-25 20:52 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=12091997-2a63-951d-9477-86c3429d981d@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=kshcherbatov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v1 2/2] sql: prevent executing crossengine sql' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox