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

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

[Kirill Shcherbatov]

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

 src/box/sql/build.c        | 103 +++++++++++++++++++++++++++++++++++++++++++++
 test/sql/triggers.result   |  75 +++++++++++++++++++++++++++++++++
 test/sql/triggers.test.lua |  34 +++++++++++++++
 3 files changed, 212 insertions(+)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a64d723..e52c05b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -54,6 +54,106 @@
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
 
+/**
+ * Test if bytecode specified with @ops and @ops_cnt contain
+ * Read/Write operations with spaces of different storage engines.
+ * This function also fills @subprograms array containing pointers
+ * to SubProgram structures extracted from OP_Program instructions.
+ * Note: _stat_space(s) are not taken into account.
+ *
+ * @param ops instructions bytecode pointer.
+ * @param ops_cnt count of instructions.
+ * @param[out] space_cache pointer to last readed space.
+ *        Required to raise error on engine mistmatch.
+ * @param[out] subprograms pointer to array of SubProgram to fill.
+ *             May be ommited, in this case, no further action is
+ *             taken.
+ * @param[out] subprograms_cnt count of subprograms stored in
+ *             @subprograms.
+ * @retval whether the byte contains the operation code for spikes
+ *         of different engines.
+ */
+static bool
+vdbe_ops_has_crossengine_action(struct VdbeOp *ops, int ops_cnt,
+				struct space **space_cache,
+				struct SubProgram **subprograms,
+				int *subprograms_cnt)
+{
+	for (int i = 1; i < ops_cnt; i++) {
+		struct VdbeOp *op = &ops[i];
+		if (op->opcode == OP_OpenRead || op->opcode == OP_OpenWrite) {
+			assert(op->p4type == P4_SPACEPTR);
+			struct space *space = op->p4.space;
+			if (space->def->id == BOX_SQL_STAT1_ID ||
+			    space->def->id == BOX_SQL_STAT4_ID)
+				continue;
+			if (*space_cache != NULL &&
+			    space->engine->vtab != (*space_cache)->engine->vtab)
+				return true;
+			*space_cache = space;
+		} else if (subprograms != NULL && op->opcode == OP_Program){
+			assert(op->p4type == P4_SUBPROGRAM);
+			subprograms[*subprograms_cnt] = op->p4.pProgram;
+			*subprograms_cnt = *subprograms_cnt + 1;
+		}
+	}
+	return false;
+}
+
+/**
+ * Check the @parse Vdbe program for operations with the spaces
+ * of different engines. Change parser state.
+ * @param parse The parsing context.
+ * @return 0 on success.
+ * @retval -1 on error.
+ */
+static int
+vdbe_check_crossengine_actions(struct Parse *parse)
+{
+	if (parse->explain)
+		return 0;
+	struct space *space_cache = NULL;
+	struct Vdbe *v = sqlite3GetVdbe(parse);
+	struct SubProgram **subprograms_list = NULL;
+
+	for (; v != NULL; v = v->pNext) {
+		/* The upper-bound of count of OP_Program ops. */
+		subprograms_list = malloc(v->nOp*sizeof(struct SubProgram *));
+		int subprograms_cnt = 0;
+		if (subprograms_list == NULL) {
+			diag_set(OutOfMemory, v->nOp*sizeof(struct SubProgram *),
+				"calloc", "subprograms_list");
+			parse->nErr++;
+			parse->rc = SQL_TARANTOOL_ERROR;
+			return -1;
+		}
+		/* Check main code and fill subprograms_list. */
+		if (vdbe_ops_has_crossengine_action(v->aOp, v->nOp,
+						    &space_cache,
+						    subprograms_list,
+						    &subprograms_cnt))
+			goto raise_error;
+
+		for (int i = 0; i < subprograms_cnt; i++) {
+			struct SubProgram *p = subprograms_list[i];
+			if (vdbe_ops_has_crossengine_action(p->aOp, p->nOp,
+							    &space_cache,
+							    NULL, 0))
+				goto raise_error;
+		}
+
+		free(subprograms_list);
+	}
+	return 0;
+
+raise_error:
+	free(subprograms_list);
+	diag_set(ClientError, ER_CROSS_ENGINE_TRANSACTION);
+	parse->rc = SQL_TARANTOOL_ERROR;
+	parse->nErr++;
+	return -1;
+}
+
 void
 sql_finish_coding(struct Parse *parse_context)
 {
@@ -66,6 +166,9 @@ sql_finish_coding(struct Parse *parse_context)
 			parse_context->rc = SQLITE_ERROR;
 		return;
 	}
+	if (vdbe_check_crossengine_actions(parse_context) != 0)
+		return;
+
 	/*
 	 * Begin by generating some termination code at the end
 	 * of the vdbe program
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index dc0a2e5..5b164f6 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -246,3 +246,78 @@ 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;")
+---
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index e019c00..9277990 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -95,3 +95,37 @@ box.space._trigger:insert(tuple)
 
 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';")
+
+-- 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';")
+
+-- 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;")
-- 
2.7.4

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

[Vladislav Shpilevoy]

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
> 

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

[Kirill Shcherbatov]

Hi! Thank you for participating. I've reworked this patch to open transaction before
index_create_iterator.
=========================================

diff --git a/src/box/sql.c b/src/box/sql.c
index d2cc0a9..0440edf 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -585,6 +585,7 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur)
 	assert(pCur);
 	assert(pCur->curFlags & BTCF_TEphemCursor);
 
+	assert(space_is_memtx(pCur->space));
 	struct iterator *it = index_create_iterator(*pCur->space->index,
 						    ITER_ALL, nil_key,
 						    0 /* part_count */);
@@ -1134,12 +1135,26 @@ cursor_seek(BtCursor *pCur, int *pRes)
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	}
 
+	struct space *space = pCur->space;
+	struct txn *txn = NULL;
+
+	assert(space->def->id != 0 || space_is_memtx(space));
+	if (space->def->id != 0 &&
+	    space->def->id != BOX_SQL_STAT4_ID &&
+	    space->def->id != BOX_SQL_STAT1_ID) {
+		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);
 	pCur->iter = it;
 	pCur->eState = CURSOR_VALID;
 
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index dc0a2e5..a692b89 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();
+---
+- 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
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index e019c00..a9df278 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -95,3 +95,56 @@ box.space._trigger:insert(tuple)
 
 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';")
+
+-- 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';")
+
+-- 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 ';'")
+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();
+box.sql.execute("ROLLBACK;");
+box.sql.execute("DROP TABLE test;");
+box.sql.execute("DROP TABLE test2;");
+test_run:cmd("setopt delimiter ''");

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 20/07/2018 20:50, Kirill Shcherbatov wrote:
> Hi! Thank you for participating. I've reworked this patch to open transaction before
> index_create_iterator.
> =========================================
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index d2cc0a9..0440edf 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -585,6 +585,7 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur)
>   	assert(pCur);
>   	assert(pCur->curFlags & BTCF_TEphemCursor);
>   
> +	assert(space_is_memtx(pCur->space));

1. I do not think this assertion is related to the patch. As well as
the next one about (id != 0 || is_memtx).

>   	struct iterator *it = index_create_iterator(*pCur->space->index,
>   						    ITER_ALL, nil_key,
>   						    0 /* part_count */);
> @@ -1134,12 +1135,26 @@ cursor_seek(BtCursor *pCur, int *pRes)
>   		return SQL_TARANTOOL_ITERATOR_FAIL;
>   	}
>   
> +	struct space *space = pCur->space;
> +	struct txn *txn = NULL;> +
> +	assert(space->def->id != 0 || space_is_memtx(space));
> +	if (space->def->id != 0 &&> +	    space->def->id != BOX_SQL_STAT4_ID &&
> +	    space->def->id != BOX_SQL_STAT1_ID) {

2. I strongly do not like these 3 checks.

* space->id == 0 can be skipped, since begin_ro_stmt needs
only space engine.

* id != stat4/1 is a huge crutch that should be removed. The
later will be dilapidated by any stat or tx manager change.
I guess it is because of ANALYZE that scans spaces and then
updates _stat1/4. But I can not understand, why it can not
firstly read the samples from user spaces, commit ro, and
secondly insert the samples into stats in another transaction.
Besides, I believe, that user spaces and analyze ones should
not be in a single transaction ever as former are actually
system ones and should not be mixed with user spaces.

Nikita, I appeal to you for onlooking. Is it possible to
split ANALYZE in two transactions? How could Kirill do it?


Besides there is another option, but IMHO it could seem even
more flawed. We could introduce a new opcode or add an option
for OP_Seek like 'is_atomic' that will trigger ro stmt start.
For spaces like analyze and ephemeral ones we could set this
option in True thereby "allowing" cross-engine transactions
for internal usage when we do not need read-view and other tx
manager facilities.


> +		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);
>   	pCur->iter = it;
>   	pCur->eState = CURSOR_VALID;
>   

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

[n.pettik]

>>  	struct iterator *it = index_create_iterator(*pCur->space->index,
>>  						    ITER_ALL, nil_key,
>>  						    0 /* part_count */);
>> @@ -1134,12 +1135,26 @@ cursor_seek(BtCursor *pCur, int *pRes)
>>  		return SQL_TARANTOOL_ITERATOR_FAIL;
>>  	}
>>  +	struct space *space = pCur->space;
>> +	struct txn *txn = NULL;> +
>> +	assert(space->def->id != 0 || space_is_memtx(space));
>> +	if (space->def->id != 0 &&> +	    space->def->id != BOX_SQL_STAT4_ID &&
>> +	    space->def->id != BOX_SQL_STAT1_ID) {
> 
> 2. I strongly do not like these 3 checks.
> 
> * space->id == 0 can be skipped, since begin_ro_stmt needs
> only space engine.
> 
> * id != stat4/1 is a huge crutch that should be removed. The
> later will be dilapidated by any stat or tx manager change.
> I guess it is because of ANALYZE that scans spaces and then
> updates _stat1/4. But I can not understand, why it can not
> firstly read the samples from user spaces, commit ro, and
> secondly insert the samples into stats in another transaction.
> Besides, I believe, that user spaces and analyze ones should
> not be in a single transaction ever as former are actually
> system ones and should not be mixed with user spaces.
> Nikita, I appeal to you for onlooking. Is it possible to
> split ANALYZE in two transactions? How could Kirill do it?
> Besides there is another option, but IMHO it could seem even
> more flawed.

Why do you think so? Actually, I can’t come up with better solution:
when we start executing OP_AnalysisLoad we always start transaction
(member that issue with gathering statistics on region to make it consistent?
See sql_analysis_load()).
Mb we always can avoid calling txn_begin_ro_stmt() on system spaces?

Quite similar to this issue might be CREATE TABLE AS SELECT
when it will be implemented. However, in this case we still can avoid
mixing user and system spaces within one transaction:

INSERT TO _SPACE/_INDEX etc
SELECT FROM source INSERT TO ephemeral
START TRANSACTION
SELECT FROM ephemeral INSERT TO destination
COMMIT TRANSACTION

> We could introduce a new opcode or add an option
> for OP_Seek like 'is_atomic' that will trigger ro stmt start.
> For spaces like analyze and ephemeral ones we could set this
> option in True thereby "allowing" cross-engine transactions
> for internal usage when we do not need read-view and other tx
> manager facilities.
> 
> 
>> +		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);
>>  	pCur->iter = it;
>>  	pCur->eState = CURSOR_VALID;
>>  
> 

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

[Vladislav Shpilevoy]

On 23/07/2018 19:20, n.pettik wrote:
> 
>>>   	struct iterator *it = index_create_iterator(*pCur->space->index,
>>>   						    ITER_ALL, nil_key,
>>>   						    0 /* part_count */);
>>> @@ -1134,12 +1135,26 @@ cursor_seek(BtCursor *pCur, int *pRes)
>>>   		return SQL_TARANTOOL_ITERATOR_FAIL;
>>>   	}
>>>   +	struct space *space = pCur->space;
>>> +	struct txn *txn = NULL;> +
>>> +	assert(space->def->id != 0 || space_is_memtx(space));
>>> +	if (space->def->id != 0 &&> +	    space->def->id != BOX_SQL_STAT4_ID &&
>>> +	    space->def->id != BOX_SQL_STAT1_ID) {
>>
>> 2. I strongly do not like these 3 checks.
>>
>> * space->id == 0 can be skipped, since begin_ro_stmt needs
>> only space engine.
>>
>> * id != stat4/1 is a huge crutch that should be removed. The
>> later will be dilapidated by any stat or tx manager change.
>> I guess it is because of ANALYZE that scans spaces and then
>> updates _stat1/4. But I can not understand, why it can not
>> firstly read the samples from user spaces, commit ro, and
>> secondly insert the samples into stats in another transaction.
>> Besides, I believe, that user spaces and analyze ones should
>> not be in a single transaction ever as former are actually
>> system ones and should not be mixed with user spaces.
>> Nikita, I appeal to you for onlooking. Is it possible to
>> split ANALYZE in two transactions? How could Kirill do it?
>> Besides there is another option, but IMHO it could seem even
>> more flawed.
> 
> Why do you think so?

I am afraid of additional 'if's for such internal thing.

> Actually, I can’t come up with better solution:
> when we start executing OP_AnalysisLoad we always start transaction
> (member that issue with gathering statistics on region to make it consistent?
> See sql_analysis_load()).

I think that on insertion into analyze spaces we correctly start a
transaction, and it is not for region usage only. As I remember,
there were problems with stat spaces consistency if we update stat
not in a transaction. Region is just a convenient benefit.

> Mb we always can avoid calling txn_begin_ro_stmt() on system spaces?

It is okay, but for ephemeral spaces too, it is not? On the parsing
stage we know is the space system one or not. Besides, I think that if
a user calls explicit "SELECT * FROM _space", we should begin ro,
but not when we open an internal cursor (ephemeral, or to insert into
stats or something). It looks like a good question for the server
team chat - should be start read-only statements on explicit SELECT
from a system space? Box API now does it (inside box_index_iterator()).

> 
> Quite similar to this issue might be CREATE TABLE AS SELECT
> when it will be implemented. However, in this case we still can avoid
> mixing user and system spaces within one transaction:
> 
> INSERT TO _SPACE/_INDEX etc
> SELECT FROM source INSERT TO ephemeral
> START TRANSACTION
> SELECT FROM ephemeral INSERT TO destination
> COMMIT TRANSACTION

Then I do not understand why can not we do the same for ANALYZE:

SELECT samples from different engines with no ro stmts;
START TX;
INSERT tuples into stat1/4;
COMMIT;

> 
>> We could introduce a new opcode or add an option
>> for OP_Seek like 'is_atomic' that will trigger ro stmt start.
>> For spaces like analyze and ephemeral ones we could set this
>> option in True thereby "allowing" cross-engine transactions
>> for internal usage when we do not need read-view and other tx
>> manager facilities.
>>
>>
>>> +		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);
>>>   	pCur->iter = it;
>>>   	pCur->eState = CURSOR_VALID;
>>>   
>>
> 

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 4333 bytes --]


>>> 2. I strongly do not like these 3 checks.
>>> 
>>> * space->id == 0 can be skipped, since begin_ro_stmt needs
>>> only space engine.
>>> 
>>> * id != stat4/1 is a huge crutch that should be removed. The
>>> later will be dilapidated by any stat or tx manager change.
>>> I guess it is because of ANALYZE that scans spaces and then
>>> updates _stat1/4. But I can not understand, why it can not
>>> firstly read the samples from user spaces, commit ro, and
>>> secondly insert the samples into stats in another transaction.
>>> Besides, I believe, that user spaces and analyze ones should
>>> not be in a single transaction ever as former are actually
>>> system ones and should not be mixed with user spaces.
>>> Nikita, I appeal to you for onlooking. Is it possible to
>>> split ANALYZE in two transactions? How could Kirill do it?
>>> Besides there is another option, but IMHO it could seem even
>>> more flawed.
>> Why do you think so?
> 
> I am afraid of additional 'if's for such internal thing.
> 
>> Actually, I can’t come up with better solution:
>> when we start executing OP_AnalysisLoad we always start transaction
>> (member that issue with gathering statistics on region to make it consistent?
>> See sql_analysis_load()).
> 
> I think that on insertion into analyze spaces we correctly start a
> transaction, and it is not for region usage only. As I remember,
> there were problems with stat spaces consistency if we update stat
> not in a transaction. Region is just a convenient benefit.

It is not an insertion: OP_AnalysisLoad executes SELECT from
stat spaces, and before those SELECTs we start transaction.

>> Mb we always can avoid calling txn_begin_ro_stmt() on system spaces?
> 
> It is okay, but for ephemeral spaces too, it is not?

Then, I don’t understand why we can’t avoid calling it for any memtx space?
As far as I see (before this patch) we don’t call txn_begin_ro_stmt() at all.

> On the parsing
> stage we know is the space system one or not. Besides, I think that if
> a user calls explicit "SELECT * FROM _space", we should begin ro,
> but not when we open an internal cursor (ephemeral, or to insert into
> stats or something). It looks like a good question for the server
> team chat - should be start read-only statements on explicit SELECT
> from a system space? Box API now does it (inside box_index_iterator()).
> 
>> Quite similar to this issue might be CREATE TABLE AS SELECT
>> when it will be implemented. However, in this case we still can avoid
>> mixing user and system spaces within one transaction:
>> INSERT TO _SPACE/_INDEX etc
>> SELECT FROM source INSERT TO ephemeral
>> START TRANSACTION
>> SELECT FROM ephemeral INSERT TO destination
>> COMMIT TRANSACTION
> 
> Then I do not understand why can not we do the same for ANALYZE:
> 
> SELECT samples from different engines with no ro stmts;
> START TX;
> INSERT tuples into stat1/4;
> COMMIT;

Look how things happen:

1. SELECT samples from different engines INSERT INTO ephemeral space
(It doesn’t matter with or without ro stmt since there is no active transaction);
2. SELECT from ephemeral space INSERT INTO _stat spaces;
3. START TX;
4. SELECT FROM _stat space INSERT INTO tarantool data-dictionary
(i.e. fill index->def->opts->stat with current statistics);

Now, tarantool fails on last (4th) step due to obvious reason: SELECT
results in explicit 

> 
>>> We could introduce a new opcode or add an option
>>> for OP_Seek like 'is_atomic' that will trigger ro stmt start.
>>> For spaces like analyze and ephemeral ones we could set this
>>> option in True thereby "allowing" cross-engine transactions
>>> for internal usage when we do not need read-view and other tx
>>> manager facilities.
>>> 
>>> 
>>>> +		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);
>>>>  	pCur->iter = it;
>>>>  	pCur->eState = CURSOR_VALID;


[-- Attachment #2: Type: text/html, Size: 22492 bytes --]

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

[Vladislav Shpilevoy]

On 23/07/2018 20:09, n.pettik wrote:
> 
>>>> 2. I strongly do not like these 3 checks.
>>>>
>>>> * space->id == 0 can be skipped, since begin_ro_stmt needs
>>>> only space engine.
>>>>
>>>> * id != stat4/1 is a huge crutch that should be removed. The
>>>> later will be dilapidated by any stat or tx manager change.
>>>> I guess it is because of ANALYZE that scans spaces and then
>>>> updates _stat1/4. But I can not understand, why it can not
>>>> firstly read the samples from user spaces, commit ro, and
>>>> secondly insert the samples into stats in another transaction.
>>>> Besides, I believe, that user spaces and analyze ones should
>>>> not be in a single transaction ever as former are actually
>>>> system ones and should not be mixed with user spaces.
>>>> Nikita, I appeal to you for onlooking. Is it possible to
>>>> split ANALYZE in two transactions? How could Kirill do it?
>>>> Besides there is another option, but IMHO it could seem even
>>>> more flawed.
>>> Why do you think so?
>>
>> I am afraid of additional 'if's for such internal thing.
>>
>>> Actually, I can’t come up with better solution:
>>> when we start executing OP_AnalysisLoad we always start transaction
>>> (member that issue with gathering statistics on region to make it consistent?
>>> See sql_analysis_load()).
>>
>> I think that on insertion into analyze spaces we correctly start a
>> transaction, and it is not for region usage only. As I remember,
>> there were problems with stat spaces consistency if we update stat
>> not in a transaction. Region is just a convenient benefit.
> 
> It is not an insertion: OP_AnalysisLoad executes SELECT from
> stat spaces, and before those SELECTs we start transaction.
> 
>>> Mb we always can avoid calling txn_begin_ro_stmt() on system spaces?
>>
>> It is okay, but for ephemeral spaces too, it is not?
> 
> Then, I don’t understand why we can’t avoid calling it for any memtx space?

Because when we start a transaction, we expect a kind of read-view,
conflict resolving, serialization. But in a case of memtx we can not
provide each of these things after a yield. So memtx and vinyl
DDL/DML/DQL should not get in touch in a transaction.

If a transaction touches vinyl, then yields are possible and memtx
is unable to deal with them.

For memtx txn_begin_ro_stmt only checks that engine is not changed.

For vinyl txn_begin_ro_stmt does more - it creates vinyl-internal
transaction object, remembers current read-view LSN etc.

> As far as I see (before this patch) we don’t call txn_begin_ro_stmt() at all.

And this is why we got the bug.

> 
>> On the parsing
>> stage we know is the space system one or not. Besides, I think that if
>> a user calls explicit "SELECT * FROM _space", we should begin ro,
>> but not when we open an internal cursor (ephemeral, or to insert into
>> stats or something). It looks like a good question for the server
>> team chat - should be start read-only statements on explicit SELECT
>> from a system space? Box API now does it (inside box_index_iterator()).
>>
>>> Quite similar to this issue might be CREATE TABLE AS SELECT
>>> when it will be implemented. However, in this case we still can avoid
>>> mixing user and system spaces within one transaction:
>>> INSERT TO _SPACE/_INDEX etc
>>> SELECT FROM source INSERT TO ephemeral
>>> START TRANSACTION
>>> SELECT FROM ephemeral INSERT TO destination
>>> COMMIT TRANSACTION
>>
>> Then I do not understand why can not we do the same for ANALYZE:
>>
>> SELECT samples from different engines with no ro stmts;
>> START TX;
>> INSERT tuples into stat1/4;
>> COMMIT;
> 
> Look how things happen:
> 
> 1. SELECT samples from different engines INSERT INTO ephemeral space
> (It doesn’t matter with or without ro stmt since there is no active transaction);
> 2. SELECT from ephemeral space INSERT INTO _stat spaces;
> 3. START TX;
> 4. SELECT FROM _stat space INSERT INTO tarantool data-dictionary
> (i.e. fill index->def->opts->stat with current statistics);
> 
> Now, tarantool fails on last (4th) step due to obvious reason: SELECT
> results in explicit

Then I did not get it. From your words, after step 3 we use only
ephemeral spaces and stat1/4, but all of them are memtx. At the
same time when I remove id != stat1/4 check from the Kirill's
patch, I see errors about multi-engine transactions.

> 
>>
>>>> We could introduce a new opcode or add an option
>>>> for OP_Seek like 'is_atomic' that will trigger ro stmt start.
>>>> For spaces like analyze and ephemeral ones we could set this
>>>> option in True thereby "allowing" cross-engine transactions
>>>> for internal usage when we do not need read-view and other tx
>>>> manager facilities.
>>>>
>>>>
>>>>> +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);
>>>>> pCur->iter = it;
>>>>> pCur->eState = CURSOR_VALID;
> 

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

[n.pettik]

>>>>> 2. I strongly do not like these 3 checks.
>>>>> 
>>>>> * space->id == 0 can be skipped, since begin_ro_stmt needs
>>>>> only space engine.
>>>>> 
>>>>> * id != stat4/1 is a huge crutch that should be removed. The
>>>>> later will be dilapidated by any stat or tx manager change.
>>>>> I guess it is because of ANALYZE that scans spaces and then
>>>>> updates _stat1/4. But I can not understand, why it can not
>>>>> firstly read the samples from user spaces, commit ro, and
>>>>> secondly insert the samples into stats in another transaction.
>>>>> Besides, I believe, that user spaces and analyze ones should
>>>>> not be in a single transaction ever as former are actually
>>>>> system ones and should not be mixed with user spaces.
>>>>> Nikita, I appeal to you for onlooking. Is it possible to
>>>>> split ANALYZE in two transactions? How could Kirill do it?
>>>>> Besides there is another option, but IMHO it could seem even
>>>>> more flawed.
>>>> Why do you think so?
>>> 
>>> I am afraid of additional 'if's for such internal thing.
>>> 
>>>> Actually, I can’t come up with better solution:
>>>> when we start executing OP_AnalysisLoad we always start transaction
>>>> (member that issue with gathering statistics on region to make it consistent?
>>>> See sql_analysis_load()).
>>> 
>>> I think that on insertion into analyze spaces we correctly start a
>>> transaction, and it is not for region usage only. As I remember,
>>> there were problems with stat spaces consistency if we update stat
>>> not in a transaction. Region is just a convenient benefit.
>> It is not an insertion: OP_AnalysisLoad executes SELECT from
>> stat spaces, and before those SELECTs we start transaction.
>>>> Mb we always can avoid calling txn_begin_ro_stmt() on system spaces?
>>> 
>>> It is okay, but for ephemeral spaces too, it is not?
>> Then, I don’t understand why we can’t avoid calling it for any memtx space?
> 
> Because when we start a transaction, we expect a kind of read-view,
> conflict resolving, serialization. But in a case of memtx we can not
> provide each of these things after a yield. So memtx and vinyl
> DDL/DML/DQL should not get in touch in a transaction.
> 
> If a transaction touches vinyl, then yields are possible and memtx
> is unable to deal with them.
> 
> For memtx txn_begin_ro_stmt only checks that engine is not changed.
> 
> For vinyl txn_begin_ro_stmt does more - it creates vinyl-internal
> transaction object, remembers current read-view LSN etc.
> 
>> As far as I see (before this patch) we don’t call txn_begin_ro_stmt() at all.
> 
> And this is why we got the bug.
> 
>>> On the parsing
>>> stage we know is the space system one or not. Besides, I think that if
>>> a user calls explicit "SELECT * FROM _space", we should begin ro,
>>> but not when we open an internal cursor (ephemeral, or to insert into
>>> stats or something). It looks like a good question for the server
>>> team chat - should be start read-only statements on explicit SELECT
>>> from a system space? Box API now does it (inside box_index_iterator()).
>>> 
>>>> Quite similar to this issue might be CREATE TABLE AS SELECT
>>>> when it will be implemented. However, in this case we still can avoid
>>>> mixing user and system spaces within one transaction:
>>>> INSERT TO _SPACE/_INDEX etc
>>>> SELECT FROM source INSERT TO ephemeral
>>>> START TRANSACTION
>>>> SELECT FROM ephemeral INSERT TO destination
>>>> COMMIT TRANSACTION
>>> 
>>> Then I do not understand why can not we do the same for ANALYZE:
>>> 
>>> SELECT samples from different engines with no ro stmts;
>>> START TX;
>>> INSERT tuples into stat1/4;
>>> COMMIT;
>> Look how things happen:
>> 1. SELECT samples from different engines INSERT INTO ephemeral space
>> (It doesn’t matter with or without ro stmt since there is no active transaction);
>> 2. SELECT from ephemeral space INSERT INTO _stat spaces;
>> 3. START TX;
>> 4. SELECT FROM _stat space INSERT INTO tarantool data-dictionary
>> (i.e. fill index->def->opts->stat with current statistics);
> 
> Then I did not get it. From your words, after step 3 we use only
> ephemeral spaces and stat1/4, but all of them are memtx. At the
> same time when I remove id != stat1/4 check from the Kirill's
> patch, I see errors about multi-engine transactions.

My initial thought after your explanation concerning txn_begin_ro_stmt() work
seems to be wrong. However, it really fails on execution sql_analysis_load():
The reason seems to be call of box_space_id_by_name().
I set br on diag_set():

  * frame #0: 0x00000001000d3d00 tarantool`diag_add_error(diag=0x0000000104800250, e=0x0000000104c90c68) at diag.h:172
    frame #1: 0x00000001000d3eab tarantool`txn_begin_in_engine(engine=0x000000010294ae80, txn=0x000000010480c038) at txn.c:160
    frame #2: 0x0000000100012e09 tarantool`txn_begin_ro_stmt(space=0x0000000104c85ba0, txn=0x000000010481edf8) at txn.h:264
    frame #3: 0x0000000100012c67 tarantool`::box_index_get(space_id=281, index_id=2, key="?M\x04\x01", key_end="\x04\x01", result=0x000000010481ee90) at index.cc:247
    frame #4: 0x00000001000db6f8 tarantool`::box_space_id_by_name(name="M", len=1) at box.cc:959
    frame #5: 0x000000010036fbde tarantool`analysis_loader(data=0x000000010481f130, argc=3, argv=0x000000011d809020, unused=0x000000011d809008) at analyze.c:1250
    frame #6: 0x000000010039fbdb tarantool`sqlite3_exec(db=0x0000000102d343f8, zSql="SELECT \"tbl\",\"idx\",\"stat\" FROM \"_sql_stat1\"", xCallback=(tarantool`analysis_loader at analyze.c:1242), pArg=0x000000010481f130, pzErrMsg=0x0000000000000000) at legacy.c:139
    frame #7: 0x000000010036f50c tarantool`sql_analysis_load(db=0x0000000102d343f8) at analyze.c:1734
    frame #8: 0x00000001003e7b25 tarantool`sqlite3VdbeExec(p=0x000000011d80c808) at vdbe.c:4778
    frame #9: 0x00000001003eb75c tarantool`sqlite3Step(p=0x000000011d80c808) at vdbeapi.c:570

One engine is memtx, another is sys view.
I guess it would be easy to fix: just use schema_find_id.

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

[Vladislav Shpilevoy]

On 23/07/2018 21:06, n.pettik wrote:
> 
>>>>>> 2. I strongly do not like these 3 checks.
>>>>>>
>>>>>> * space->id == 0 can be skipped, since begin_ro_stmt needs
>>>>>> only space engine.
>>>>>>
>>>>>> * id != stat4/1 is a huge crutch that should be removed. The
>>>>>> later will be dilapidated by any stat or tx manager change.
>>>>>> I guess it is because of ANALYZE that scans spaces and then
>>>>>> updates _stat1/4. But I can not understand, why it can not
>>>>>> firstly read the samples from user spaces, commit ro, and
>>>>>> secondly insert the samples into stats in another transaction.
>>>>>> Besides, I believe, that user spaces and analyze ones should
>>>>>> not be in a single transaction ever as former are actually
>>>>>> system ones and should not be mixed with user spaces.
>>>>>> Nikita, I appeal to you for onlooking. Is it possible to
>>>>>> split ANALYZE in two transactions? How could Kirill do it?
>>>>>> Besides there is another option, but IMHO it could seem even
>>>>>> more flawed.
>>>>> Why do you think so?
>>>>
>>>> I am afraid of additional 'if's for such internal thing.
>>>>
>>>>> Actually, I can’t come up with better solution:
>>>>> when we start executing OP_AnalysisLoad we always start transaction
>>>>> (member that issue with gathering statistics on region to make it consistent?
>>>>> See sql_analysis_load()).
>>>>
>>>> I think that on insertion into analyze spaces we correctly start a
>>>> transaction, and it is not for region usage only. As I remember,
>>>> there were problems with stat spaces consistency if we update stat
>>>> not in a transaction. Region is just a convenient benefit.
>>> It is not an insertion: OP_AnalysisLoad executes SELECT from
>>> stat spaces, and before those SELECTs we start transaction.
>>>>> Mb we always can avoid calling txn_begin_ro_stmt() on system spaces?
>>>>
>>>> It is okay, but for ephemeral spaces too, it is not?
>>> Then, I don’t understand why we can’t avoid calling it for any memtx space?
>>
>> Because when we start a transaction, we expect a kind of read-view,
>> conflict resolving, serialization. But in a case of memtx we can not
>> provide each of these things after a yield. So memtx and vinyl
>> DDL/DML/DQL should not get in touch in a transaction.
>>
>> If a transaction touches vinyl, then yields are possible and memtx
>> is unable to deal with them.
>>
>> For memtx txn_begin_ro_stmt only checks that engine is not changed.
>>
>> For vinyl txn_begin_ro_stmt does more - it creates vinyl-internal
>> transaction object, remembers current read-view LSN etc.
>>
>>> As far as I see (before this patch) we don’t call txn_begin_ro_stmt() at all.
>>
>> And this is why we got the bug.
>>
>>>> On the parsing
>>>> stage we know is the space system one or not. Besides, I think that if
>>>> a user calls explicit "SELECT * FROM _space", we should begin ro,
>>>> but not when we open an internal cursor (ephemeral, or to insert into
>>>> stats or something). It looks like a good question for the server
>>>> team chat - should be start read-only statements on explicit SELECT
>>>> from a system space? Box API now does it (inside box_index_iterator()).
>>>>
>>>>> Quite similar to this issue might be CREATE TABLE AS SELECT
>>>>> when it will be implemented. However, in this case we still can avoid
>>>>> mixing user and system spaces within one transaction:
>>>>> INSERT TO _SPACE/_INDEX etc
>>>>> SELECT FROM source INSERT TO ephemeral
>>>>> START TRANSACTION
>>>>> SELECT FROM ephemeral INSERT TO destination
>>>>> COMMIT TRANSACTION
>>>>
>>>> Then I do not understand why can not we do the same for ANALYZE:
>>>>
>>>> SELECT samples from different engines with no ro stmts;
>>>> START TX;
>>>> INSERT tuples into stat1/4;
>>>> COMMIT;
>>> Look how things happen:
>>> 1. SELECT samples from different engines INSERT INTO ephemeral space
>>> (It doesn’t matter with or without ro stmt since there is no active transaction);
>>> 2. SELECT from ephemeral space INSERT INTO _stat spaces;
>>> 3. START TX;
>>> 4. SELECT FROM _stat space INSERT INTO tarantool data-dictionary
>>> (i.e. fill index->def->opts->stat with current statistics);
>>
>> Then I did not get it. From your words, after step 3 we use only
>> ephemeral spaces and stat1/4, but all of them are memtx. At the
>> same time when I remove id != stat1/4 check from the Kirill's
>> patch, I see errors about multi-engine transactions.
> 
> My initial thought after your explanation concerning txn_begin_ro_stmt() work
> seems to be wrong. However, it really fails on execution sql_analysis_load():
> The reason seems to be call of box_space_id_by_name().
> I set br on diag_set():
> 
>    * frame #0: 0x00000001000d3d00 tarantool`diag_add_error(diag=0x0000000104800250, e=0x0000000104c90c68) at diag.h:172
>      frame #1: 0x00000001000d3eab tarantool`txn_begin_in_engine(engine=0x000000010294ae80, txn=0x000000010480c038) at txn.c:160
>      frame #2: 0x0000000100012e09 tarantool`txn_begin_ro_stmt(space=0x0000000104c85ba0, txn=0x000000010481edf8) at txn.h:264
>      frame #3: 0x0000000100012c67 tarantool`::box_index_get(space_id=281, index_id=2, key="?M\x04\x01", key_end="\x04\x01", result=0x000000010481ee90) at index.cc:247
>      frame #4: 0x00000001000db6f8 tarantool`::box_space_id_by_name(name="M", len=1) at box.cc:959
>      frame #5: 0x000000010036fbde tarantool`analysis_loader(data=0x000000010481f130, argc=3, argv=0x000000011d809020, unused=0x000000011d809008) at analyze.c:1250
>      frame #6: 0x000000010039fbdb tarantool`sqlite3_exec(db=0x0000000102d343f8, zSql="SELECT \"tbl\",\"idx\",\"stat\" FROM \"_sql_stat1\"", xCallback=(tarantool`analysis_loader at analyze.c:1242), pArg=0x000000010481f130, pzErrMsg=0x0000000000000000) at legacy.c:139
>      frame #7: 0x000000010036f50c tarantool`sql_analysis_load(db=0x0000000102d343f8) at analyze.c:1734
>      frame #8: 0x00000001003e7b25 tarantool`sqlite3VdbeExec(p=0x000000011d80c808) at vdbe.c:4778
>      frame #9: 0x00000001003eb75c tarantool`sqlite3Step(p=0x000000011d80c808) at vdbeapi.c:570
> 
> One engine is memtx, another is sys view.
> I guess it would be easy to fix: just use schema_find_id.
> 

Kirill, please, try to fix this in that way. If you succeeded we could defer
this complex cross-engine things on later times and now just always begin ro stmt.

[tarantool-patches] [PATCH v1 1/2] sql: use schema API to get index info in analyze

[Kirill Shcherbatov]

We would like to avoid starting transactions in ANALYZE
so we need to use schema API that is more tolerant.

Part of #3551.
---
 src/box/sql/analyze.c | 82 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 48 insertions(+), 34 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 00d96d2..c7b85dc 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1219,6 +1219,26 @@ decode_stat_string(const char *stat_string, int stat_size, tRowcnt *stat_exact,
 }
 
 /**
+ * Find index with specified @name in specified @space.
+ * @param space to lookup.
+ * @param name to use in comparation.
+ * @param len lenth of @name string.
+ * @retval NULL on nothing found.
+ * @retval not NULL pointer on index AST else.
+ */
+static struct index *
+space_index_by_name(struct space *space, const char *name, uint32_t len)
+{
+	uint32_t idx_cnt = space->index_count;
+	for (uint32_t i = 0; i < idx_cnt; i++) {
+		const char *idx_name = space->index[i]->def->name;
+		if (strlen(idx_name) == len && memcmp(idx_name, name, len) == 0)
+			return space->index[i];
+	}
+	return NULL;
+}
+
+/**
  * This callback is invoked once for each index when reading the
  * _sql_stat1 table.
  *
@@ -1246,20 +1266,20 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
 	struct analysis_index_info *info = (struct analysis_index_info *) data;
 	assert(info->stats != NULL);
 	struct index_stat *stat = &info->stats[info->index_count++];
-	uint32_t space_id = box_space_id_by_name(argv[0], strlen(argv[0]));
-	if (space_id == BOX_ID_NIL)
+	uint32_t space_id = BOX_ID_NIL;
+	int rc = schema_find_id(BOX_SPACE_ID, 2, argv[0], strlen(argv[0]),
+				&space_id);
+	if (rc != 0 || space_id == BOX_ID_NIL)
 		return -1;
 	struct space *space = space_by_id(space_id);
 	assert(space != NULL);
-	struct index *index;
-	uint32_t iid = box_index_id_by_name(space_id, argv[1], strlen(argv[1]));
+	struct index *index =
+		space_index_by_name(space, argv[1], strlen(argv[1]));
 	/*
 	 * Convention is if index's name matches with space's
 	 * one, then it is primary index.
 	 */
-	if (iid != BOX_ID_NIL) {
-		index = space_index(space, iid);
-	} else {
+	if (index == NULL) {
 		if (sqlite3_stricmp(argv[0], argv[1]) != 0)
 			return -1;
 		index = space_index(space, 0);
@@ -1417,19 +1437,17 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 		if (index_name == NULL)
 			continue;
 		uint32_t sample_count = sqlite3_column_int(stmt, 2);
-		uint32_t space_id = box_space_id_by_name(space_name,
-							 strlen(space_name));
-		assert(space_id != BOX_ID_NIL);
+		uint32_t space_id = BOX_ID_NIL;
+		rc = schema_find_id(BOX_SPACE_ID, 2, space_name,
+				    strlen(space_name), &space_id);
+		assert(rc == 0 && space_id != BOX_ID_NIL);
 		struct space *space = space_by_id(space_id);
 		assert(space != NULL);
-		struct index *index;
-		uint32_t iid = box_index_id_by_name(space_id, index_name,
-						    strlen(index_name));
+		struct index *index =
+			space_index_by_name(space, index_name, strlen(index_name));
 		if (sqlite3_stricmp(space_name, index_name) == 0 &&
-		    iid == BOX_ID_NIL)
+		    index == NULL)
 			index = space_index(space, 0);
-		else
-			index = space_index(space, iid);
 		assert(index != NULL);
 		uint32_t column_count = index->def->key_def->part_count;
 		struct index_stat *stat = &stats[current_idx_count];
@@ -1488,17 +1506,15 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 		const char *index_name = (char *)sqlite3_column_text(stmt, 1);
 		if (index_name == NULL)
 			continue;
-		uint32_t space_id = box_space_id_by_name(space_name,
-							 strlen(space_name));
-		assert(space_id != BOX_ID_NIL);
+		uint32_t space_id = BOX_ID_NIL;
+		int rc = schema_find_id(BOX_SPACE_ID, 2, space_name,
+					strlen(space_name), &space_id);
+		assert(rc == 0 && space_id != BOX_ID_NIL);
 		struct space *space = space_by_id(space_id);
 		assert(space != NULL);
-		struct index *index;
-		uint32_t iid = box_index_id_by_name(space_id, index_name,
-						    strlen(index_name));
-		if (iid != BOX_ID_NIL) {
-			index = space_index(space, iid);
-		} else {
+		struct index *index =
+			space_index_by_name(space, index_name, strlen(index_name));
+		if (index == NULL) {
 			if (sqlite3_stricmp(space_name, index_name) != 0)
 				return -1;
 			index = space_index(space, 0);
@@ -1572,18 +1588,16 @@ load_stat_to_index(struct sqlite3 *db, const char *sql_select_load,
 		const char *index_name = (char *)sqlite3_column_text(stmt, 1);
 		if (index_name == NULL)
 			continue;
-		uint32_t space_id = box_space_id_by_name(space_name,
-							 strlen(space_name));
-		if (space_id == BOX_ID_NIL)
+		uint32_t space_id = BOX_ID_NIL;
+		int rc = schema_find_id(BOX_SPACE_ID, 2, space_name, strlen(space_name),
+					&space_id);
+		if (rc != 0 || space_id == BOX_ID_NIL)
 			return -1;
 		struct space *space = space_by_id(space_id);
 		assert(space != NULL);
-		struct index *index;
-		uint32_t iid = box_index_id_by_name(space_id, index_name,
-						    strlen(index_name));
-		if (iid != BOX_ID_NIL) {
-			index = space_index(space, iid);
-		} else {
+		struct index *index =
+			space_index_by_name(space, index_name, strlen(index_name));
+		if (index == NULL) {
 			if (sqlite3_stricmp(space_name, index_name) != 0)
 				return -1;
 			index = space_index(space, 0);
-- 
2.7.4

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

[Kirill Shcherbatov]

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.

Closes #3551
---
 src/box/sql.c              |  11 +++++
 test/sql/triggers.result   | 120 +++++++++++++++++++++++++++++++++++++++++++++
 test/sql/triggers.test.lua |  53 ++++++++++++++++++++
 3 files changed, 184 insertions(+)

diff --git a/src/box/sql.c b/src/box/sql.c
index d48c3cf..a964fcb 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);
 	pCur->iter = it;
 	pCur->eState = CURSOR_VALID;
 
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index dc0a2e5..a692b89 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();
+---
+- 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
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index e019c00..a9df278 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -95,3 +95,56 @@ box.space._trigger:insert(tuple)
 
 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';")
+
+-- 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';")
+
+-- 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 ';'")
+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();
+box.sql.execute("ROLLBACK;");
+box.sql.execute("DROP TABLE test;");
+box.sql.execute("DROP TABLE test2;");
+test_run:cmd("setopt delimiter ''");
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 1/2] sql: use schema API to get index info in analyze

[n.pettik]

> On 24 Jul 2018, at 14:05, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> We would like to avoid starting transactions in ANALYZE
> so we need to use schema API that is more tolerant.

Please, provide more detailed explanation, such as:

“
Previously, ANALYZE routine used to invoke box_space_id_by_name
function, which in turn relies on sysview engine. On the other hand,
ANALYZE processing is surrounded by transaction started in memtx
(_sql_stat1 and _sql_stat4 are system spaces stored in memtx).
Thus, multi-engine transaction error may  occur. To avoid such
situation lets use internal schema_find_id() function.
"

> 
> Part of #3551.
> ---
> src/box/sql/analyze.c | 82 ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 00d96d2..c7b85dc 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1219,6 +1219,26 @@ decode_stat_string(const char *stat_string, int stat_size, tRowcnt *stat_exact,
> }
> 
> /**
> + * Find index with specified @name in specified @space.
> + * @param space to lookup.
> + * @param name to use in comparation.
> + * @param len lenth of @name string.

Typo: length.

> + * @retval NULL on nothing found.

If nothing is found.

> + * @retval not NULL pointer on index AST else.

Index AST? I guess “pointer to index otherwise”.

> + */
> +static struct index *
> +space_index_by_name(struct space *space, const char *name, uint32_t len)
> +{
> +	uint32_t idx_cnt = space->index_count;
> +	for (uint32_t i = 0; i < idx_cnt; i++) {
> +		const char *idx_name = space->index[i]->def->name;
> +		if (strlen(idx_name) == len && memcmp(idx_name, name, len) == 0)
> +			return space->index[i];
> +	}
> +	return NULL;
> +}

Overall it is sad that we can't use schema_find_id for this purpose.
Mb it is worth to refactor this function so that it will take not id but
already encoded key to be found? It is only suggestion tho,
it can be contradictory way..

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

[n.pettik]

> 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

Typo: this means.

> that we shouldn't create triggers on memtex spaces writing
> something in vinyl and so on.

We shouldn’t but you didn’t ban such option in your patch..

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

[Kirill Shcherbatov]

> Typo: this means.
> We shouldn’t but you didn’t ban such option in your patch..

Ok, thanks. Fixed.

[tarantool-patches] Re: [PATCH v1 1/2] sql: use schema API to get index info in analyze

[Kirill Shcherbatov]

> Previously, ANALYZE routine used to invoke box_space_id_by_name
> function, which in turn relies on sysview engine. On the other hand,
> ANALYZE processing is surrounded by transaction started in memtx
> (_sql_stat1 and _sql_stat4 are system spaces stored in memtx).
> Thus, multi-engine transaction error may  occur. To avoid such
> situation lets use internal schema_find_id() function.
Thank, I've used your explanation.

> Typo: length.
> If nothing is found.
> Index AST? I guess “pointer to index otherwise”.
+ * @param len length of @name string.
+ * @retval NULL if nothing is found.
+ * @retval not NULL pointer to index otherwise.


> Overall it is sad that we can't use schema_find_id for this purpose.
> Mb it is worth to refactor this function so that it will take not id but
> already encoded key to be found? It is only suggestion tho,
> it can be contradictory way..
We have already discussed that this is ok. Tnx.

[tarantool-patches] Re: [PATCH v1 1/2] sql: use schema API to get index info in analyze

[Vladislav Shpilevoy]

Hi! Thanks for the patch, but it is not needed anymore
after this commit
https://github.com/tarantool/tarantool/commit/0ecabde8981500886bf5be16cfabe4d1d1a33db4.

Moreover, we should stop use schema_find_id to get a space since
such requests should be filtered through _vspace to prevent
unauthorized access to _space. I've opened an issue:
https://github.com/tarantool/tarantool/issues/3570

On 24/07/2018 14:05, Kirill Shcherbatov wrote:
> We would like to avoid starting transactions in ANALYZE
> so we need to use schema API that is more tolerant.
> 
> Part of #3551.
> ---
>   src/box/sql/analyze.c | 82 ++++++++++++++++++++++++++++++---------------------
>   1 file changed, 48 insertions(+), 34 deletions(-)
> 

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

[Vladislav Shpilevoy]

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
> +...

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

[Kirill Shcherbatov]

I've cherry-picked Vova's path on my branch as it hasn't already merged to 2.0.

> 1. Please, put a new patch version at the end of letter.
Ok.

> 2. crossengine looks like 'междудвижками'. Please, replace with
> cross-engine.
ok.

> 3. 'dissalow', 'dangeros' - no such words.
fixed.
> 4. Both commit_ro and rollback_ro already check for txn != NULL so please,
> remove the redundant checks.
ok.

> 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.
I loks ungly.. ok.  You now, box.sql.execute can't process multiple sql statements

================================

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 disallow such requests.
Since now iterators in SQL start transaction so we cant prevent
such vicious and dangerous things.

Closes #3551
---
 src/box/sql.c              |   8 ++++
 test/sql/triggers.result   | 104 +++++++++++++++++++++++++++++++++++++++++++++
 test/sql/triggers.test.lua |  45 ++++++++++++++++++++
 3 files changed, 157 insertions(+)

diff --git a/src/box/sql.c b/src/box/sql.c
index d48c3cf..db08fcf 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1118,12 +1118,20 @@ cursor_seek(BtCursor *pCur, int *pRes)
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	}
 
+	struct space *space = pCur->space;
+	struct txn *txn = in_txn();
+	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) {
+		txn_rollback_stmt();
 		pCur->eState = CURSOR_INVALID;
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	}
+	txn_commit_ro_stmt(txn);
 	pCur->iter = it;
 	pCur->eState = CURSOR_VALID;
 
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index dc0a2e5..71ade54 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -246,3 +246,107 @@ 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)")
+---
+...
+box.sql.execute("START TRANSACTION") box.sql.execute("INSERT INTO test VALUES (1)") box.sql.execute("SELECT * FROM test2")  box.sql.execute("COMMIT")
+---
+- 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;");
+---
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index e019c00..c7f377c 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -95,3 +95,48 @@ box.space._trigger:insert(tuple)
 
 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';")
+
+-- 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';")
+
+-- 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)")
+box.sql.execute("START TRANSACTION") box.sql.execute("INSERT INTO test VALUES (1)") box.sql.execute("SELECT * FROM test2")  box.sql.execute("COMMIT")
+box.sql.execute("ROLLBACK;");
+box.sql.execute("DROP TABLE test;");
+box.sql.execute("DROP TABLE test2;");
-- 
2.7.4

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

[Vladislav Shpilevoy]

On 26/07/2018 10:08, Kirill Shcherbatov wrote:
> I've cherry-picked Vova's path on my branch as it hasn't already merged to 2.0.
> 
>> 1. Please, put a new patch version at the end of letter.
> Ok.
> 
>> 2. crossengine looks like 'междудвижками'. Please, replace with
>> cross-engine.
> ok.
> 
>> 3. 'dissalow', 'dangeros' - no such words.
> fixed.
>> 4. Both commit_ro and rollback_ro already check for txn != NULL so please,
>> remove the redundant checks.
> ok.
> 
>> 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.
> I loks ungly.. ok.  You now, box.sql.execute can't process multiple sql statements

1. I did not purposed to use single box.sql.execute.

2. I did not insist on the single line. My proposal was to remove f().

> diff --git a/test/sql/triggers.result b/test/sql/triggers.result
> index dc0a2e5..71ade54 100644
> --- a/test/sql/triggers.result
> +++ b/test/sql/triggers.result
> @@ -246,3 +246,107 @@ 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)")
> +---
> +...
> +box.sql.execute("START TRANSACTION") box.sql.execute("INSERT INTO test VALUES (1)") box.sql.execute("SELECT * FROM test2")  box.sql.execute("COMMIT")
> +---
> +- error: A multi-statement transaction can not use multiple storage engines
> +...
> +box.sql.execute("ROLLBACK;");

3. Why do you need ';' after execute()?

> +---
> +...
> +box.sql.execute("DROP TABLE test;");
> +---
> +...
> +box.sql.execute("DROP TABLE test2;");
> +---
> +...

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

[Kirill Shcherbatov]

> 1. I did not purposed to use single box.sql.execute.
> 2. I did not insist on the single line. My proposal was to remove f().
Ok.

> 3. Why do you need ';' after execute()?
Deleted...

===================================

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 disallow such requests.
Since now iterators in SQL start transaction so we cant prevent
such vicious and dangerous things.

Closes #3551
---
 src/box/sql.c              |   8 ++++
 test/sql/triggers.result   | 110 +++++++++++++++++++++++++++++++++++++++++++++
 test/sql/triggers.test.lua |  47 +++++++++++++++++++
 3 files changed, 165 insertions(+)

diff --git a/src/box/sql.c b/src/box/sql.c
index d48c3cf..db08fcf 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1118,12 +1118,20 @@ cursor_seek(BtCursor *pCur, int *pRes)
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	}
 
+	struct space *space = pCur->space;
+	struct txn *txn = in_txn();
+	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) {
+		txn_rollback_stmt();
 		pCur->eState = CURSOR_INVALID;
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	}
+	txn_commit_ro_stmt(txn);
 	pCur->iter = it;
 	pCur->eState = CURSOR_VALID;
 
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index dc0a2e5..658571b 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -246,3 +246,113 @@ 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)")
+---
+...
+box.sql.execute("START TRANSACTION")
+---
+...
+box.sql.execute("INSERT INTO test VALUES (1)")
+---
+...
+box.sql.execute("SELECT * FROM test2")
+---
+- 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;")
+---
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index e019c00..8fd385c 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -95,3 +95,50 @@ box.space._trigger:insert(tuple)
 
 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';")
+
+-- 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';")
+
+-- 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)")
+box.sql.execute("START TRANSACTION")
+box.sql.execute("INSERT INTO test VALUES (1)")
+box.sql.execute("SELECT * FROM test2")
+box.sql.execute("ROLLBACK;")
+box.sql.execute("DROP TABLE test;")
+box.sql.execute("DROP TABLE test2;")
-- 
2.7.4

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

[Vladislav Shpilevoy]

Thanks for the fixes! I have pushed my own on the branch.
Please, squash.

Sorry, I was wrong about txn != NULL omitting. If we omit it,
then for ephemeral spaces we do not start ro stmt, but commit
it. It is not correct.

Your code works only because commit_ro() is no-op when we
have an active transaction. But it can change in future.

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

[Kirill Shcherbatov]

> Thanks for the fixes! I have pushed my own on the branch.
> Please, squash.
> 
> Sorry, I was wrong about txn != NULL omitting. If we omit it,
> then for ephemeral spaces we do not start ro stmt, but commit
> it. It is not correct.
> 
> Your code works only because commit_ro() is no-op when we
> have an active transaction. But it can change in futureThank you for review. I've squashed this changes. 

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

[Vladislav Shpilevoy]

Hi! I do not see your answer. You've just sent empty email with my cite.

On 27/07/2018 10:13, Kirill Shcherbatov wrote:
>> Thanks for the fixes! I have pushed my own on the branch.
>> Please, squash.
>>
>> Sorry, I was wrong about txn != NULL omitting. If we omit it,
>> then for ephemeral spaces we do not start ro stmt, but commit
>> it. It is not correct.
>>
>> Your code works only because commit_ro() is no-op when we
>> have an active transaction. But it can change in futureThank you for review. I've squashed this changes.

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

[Kirill Shcherbatov]

On 27.07.2018 11:55, Vladislav Shpilevoy wrote:
> Hi! I do not see your answer. You've just sent empty email with my cite.
> 
> On 27/07/2018 10:13, Kirill Shcherbatov wrote:
>>> Thanks for the fixes! I have pushed my own on the branch.
>>> Please, squash.
>>>
>>> Sorry, I was wrong about txn != NULL omitting. If we omit it,
>>> then for ephemeral spaces we do not start ro stmt, but commit
>>> it. It is not correct.
>>>
>>> Your code works only because commit_ro() is no-op when we
>>> have an active transaction. But it can change in futureThank you for review. I've squashed this changes.
Oh, sorry. It is kind of misunderstanding. Maybe I've press Enter on selected text before send via Ctrl+Enter.Thank you for review. I've squashed your changes.

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

[Vladislav Shpilevoy]

LGTM.

On 27/07/2018 13:02, Kirill Shcherbatov wrote:
> 
> 
> On 27.07.2018 11:55, Vladislav Shpilevoy wrote:
>> Hi! I do not see your answer. You've just sent empty email with my cite.
>>
>> On 27/07/2018 10:13, Kirill Shcherbatov wrote:
>>>> Thanks for the fixes! I have pushed my own on the branch.
>>>> Please, squash.
>>>>
>>>> Sorry, I was wrong about txn != NULL omitting. If we omit it,
>>>> then for ephemeral spaces we do not start ro stmt, but commit
>>>> it. It is not correct.
>>>>
>>>> Your code works only because commit_ro() is no-op when we
>>>> have an active transaction. But it can change in futureThank you for review. I've squashed this changes.
> Oh, sorry. It is kind of misunderstanding. Maybe I've press Enter on selected text before send via Ctrl+Enter.Thank you for review. I've squashed your changes.
> 

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

[Kirill Yukhin]

Hello,
On 24 июл 14:05, 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.
> 
> Closes #3551
I've checked in the patch into 2.0 branch.

--
Regards, Kirill Yukhin