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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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;
>