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 7C736200F2 for ; Mon, 23 Jul 2018 13:21:32 -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 bWfkvKKtvHHL for ; Mon, 23 Jul 2018 13:21:32 -0400 (EDT) Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 CADDA200EB for ; Mon, 23 Jul 2018 13:21:31 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: prevent executing crossengine sql 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> From: Vladislav Shpilevoy Message-ID: Date: Mon, 23 Jul 2018 20:21:29 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org Cc: Kirill Shcherbatov 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; >