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 6E06623A74 for ; Mon, 23 Jul 2018 07:50:06 -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 98J12AjqGF9k for ; Mon, 23 Jul 2018 07:50:06 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 C0F442033A for ; Mon, 23 Jul 2018 07:50:05 -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> From: Vladislav Shpilevoy Message-ID: <26691970-9406-1cd2-e2ea-3e78b4d7b145@tarantool.org> Date: Mon, 23 Jul 2018 14:50:03 +0300 MIME-Version: 1.0 In-Reply-To: <812d64bf-a5a8-805d-d353-32f6733a41b9@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Kirill Shcherbatov , tarantool-patches@freelists.org Hi! Thanks for the patch! On 20/07/2018 20:50, Kirill Shcherbatov wrote: > Hi! Thank you for participating. I've reworked this patch to open transaction before > index_create_iterator. > ========================================= > > diff --git a/src/box/sql.c b/src/box/sql.c > index d2cc0a9..0440edf 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -585,6 +585,7 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur) > assert(pCur); > assert(pCur->curFlags & BTCF_TEphemCursor); > > + assert(space_is_memtx(pCur->space)); 1. I do not think this assertion is related to the patch. As well as the next one about (id != 0 || is_memtx). > struct iterator *it = index_create_iterator(*pCur->space->index, > ITER_ALL, nil_key, > 0 /* part_count */); > @@ -1134,12 +1135,26 @@ cursor_seek(BtCursor *pCur, int *pRes) > return SQL_TARANTOOL_ITERATOR_FAIL; > } > > + struct space *space = pCur->space; > + struct txn *txn = NULL;> + > + assert(space->def->id != 0 || space_is_memtx(space)); > + if (space->def->id != 0 &&> + space->def->id != BOX_SQL_STAT4_ID && > + space->def->id != BOX_SQL_STAT1_ID) { 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. 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; >