Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>,
	tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: prevent executing crossengine sql
Date: Mon, 23 Jul 2018 14:50:03 +0300	[thread overview]
Message-ID: <26691970-9406-1cd2-e2ea-3e78b4d7b145@tarantool.org> (raw)
In-Reply-To: <812d64bf-a5a8-805d-d353-32f6733a41b9@tarantool.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;
>   

  reply	other threads:[~2018-07-23 11:50 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-20 13:52 [tarantool-patches] " Kirill Shcherbatov
2018-07-20 15:03 ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-20 17:50   ` Kirill Shcherbatov
2018-07-23 11:50     ` Vladislav Shpilevoy [this message]
2018-07-23 16:20       ` n.pettik
2018-07-23 16:39         ` Vladislav Shpilevoy
2018-07-23 17:09           ` n.pettik
2018-07-23 17:21             ` Vladislav Shpilevoy
2018-07-23 18:06               ` n.pettik
2018-07-23 18:29                 ` Vladislav Shpilevoy
2018-07-24 11:05                   ` [tarantool-patches] [PATCH v1 1/2] sql: use schema API to get index info in analyze Kirill Shcherbatov
     [not found]                     ` <cover.1532430181.git.kshcherbatov@tarantool.org>
2018-07-24 11:05                       ` [tarantool-patches] [PATCH v1 2/2] sql: prevent executing crossengine sql Kirill Shcherbatov
2018-07-25 13:24                         ` [tarantool-patches] " n.pettik
2018-07-25 17:07                           ` Kirill Shcherbatov
2018-07-25 21:05                             ` Vladislav Shpilevoy
2018-07-26  7:08                               ` Kirill Shcherbatov
2018-07-26  8:54                                 ` Vladislav Shpilevoy
2018-07-26 11:22                                   ` Kirill Shcherbatov
2018-07-26 21:26                                     ` Vladislav Shpilevoy
2018-07-27  7:13                                       ` Kirill Shcherbatov
2018-07-27  8:55                                         ` Vladislav Shpilevoy
2018-07-27 10:02                                           ` Kirill Shcherbatov
2018-07-27 10:14                                             ` Vladislav Shpilevoy
2018-07-31  7:54                         ` Kirill Yukhin
2018-07-25 13:22                     ` [tarantool-patches] Re: [PATCH v1 1/2] sql: use schema API to get index info in analyze n.pettik
2018-07-25 17:07                       ` Kirill Shcherbatov
2018-07-25 20:52                     ` Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=26691970-9406-1cd2-e2ea-3e78b4d7b145@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: prevent executing crossengine sql' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox