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