On 23/07/2018 21:06, n.pettik wrote:
It is not an insertion: OP_AnalysisLoad executes SELECT from2. I strongly do not like these 3 checks.Why do you think so?
* 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.
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.
stat spaces, and before those SELECTs we start transaction.
Then, I don’t understand why we can’t avoid calling it for any memtx space?Mb we always can avoid calling txn_begin_ro_stmt() on system spaces?
It is okay, but for ephemeral spaces too, it is not?
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 parsingLook how things happen:
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;
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.
