From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 995E02294B for ; Mon, 23 Jul 2018 14:06:16 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id I6DLTQt_j9PP for ; Mon, 23 Jul 2018 14:06:16 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 4DC20226E1 for ; Mon, 23 Jul 2018 14:06:16 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: prevent executing crossengine sql From: "n.pettik" In-Reply-To: Date: Mon, 23 Jul 2018 21:06:13 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <462DD32C-45A7-48FB-8FB1-58D4DA85C553@tarantool.org> References: <6134a0afdcfba13ca289e2d42fdd529a2787070a.1532094680.git.kshcherbatov@tarantool.org> <50ac96b7-d395-f528-f66b-c10847e54670@tarantool.org> <812d64bf-a5a8-805d-d353-32f6733a41b9@tarantool.org> <26691970-9406-1cd2-e2ea-3e78b4d7b145@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy , Kirill Shcherbatov >>>>> 2. I strongly do not like these 3 checks. >>>>>=20 >>>>> * space->id =3D=3D 0 can be skipped, since begin_ro_stmt needs >>>>> only space engine. >>>>>=20 >>>>> * id !=3D 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? >>>=20 >>> I am afraid of additional 'if's for such internal thing. >>>=20 >>>> Actually, I can=E2=80=99t 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()). >>>=20 >>> 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? >>>=20 >>> It is okay, but for ephemeral spaces too, it is not? >> Then, I don=E2=80=99t understand why we can=E2=80=99t avoid calling = it for any memtx space? >=20 > 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. >=20 > If a transaction touches vinyl, then yields are possible and memtx > is unable to deal with them. >=20 > For memtx txn_begin_ro_stmt only checks that engine is not changed. >=20 > For vinyl txn_begin_ro_stmt does more - it creates vinyl-internal > transaction object, remembers current read-view LSN etc. >=20 >> As far as I see (before this patch) we don=E2=80=99t call = txn_begin_ro_stmt() at all. >=20 > And this is why we got the bug. >=20 >>> 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()). >>>=20 >>>> 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 >>>=20 >>> Then I do not understand why can not we do the same for ANALYZE: >>>=20 >>> 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=E2=80=99t 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); >=20 > Then I did not get it. =46rom 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 !=3D 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=3D0x0000000104800250, = e=3D0x0000000104c90c68) at diag.h:172 frame #1: 0x00000001000d3eab = tarantool`txn_begin_in_engine(engine=3D0x000000010294ae80, = txn=3D0x000000010480c038) at txn.c:160 frame #2: 0x0000000100012e09 = tarantool`txn_begin_ro_stmt(space=3D0x0000000104c85ba0, = txn=3D0x000000010481edf8) at txn.h:264 frame #3: 0x0000000100012c67 tarantool`::box_index_get(space_id=3D281,= index_id=3D2, key=3D"?M\x04\x01", key_end=3D"\x04\x01", = result=3D0x000000010481ee90) at index.cc:247 frame #4: 0x00000001000db6f8 = tarantool`::box_space_id_by_name(name=3D"M", len=3D1) at box.cc:959 frame #5: 0x000000010036fbde = tarantool`analysis_loader(data=3D0x000000010481f130, argc=3D3, = argv=3D0x000000011d809020, unused=3D0x000000011d809008) at = analyze.c:1250 frame #6: 0x000000010039fbdb = tarantool`sqlite3_exec(db=3D0x0000000102d343f8, zSql=3D"SELECT = \"tbl\",\"idx\",\"stat\" FROM \"_sql_stat1\"", = xCallback=3D(tarantool`analysis_loader at analyze.c:1242), = pArg=3D0x000000010481f130, pzErrMsg=3D0x0000000000000000) at = legacy.c:139 frame #7: 0x000000010036f50c = tarantool`sql_analysis_load(db=3D0x0000000102d343f8) at analyze.c:1734 frame #8: 0x00000001003e7b25 = tarantool`sqlite3VdbeExec(p=3D0x000000011d80c808) at vdbe.c:4778 frame #9: 0x00000001003eb75c = tarantool`sqlite3Step(p=3D0x000000011d80c808) 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.