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 511B922F30 for ; Mon, 23 Jul 2018 13:09:11 -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 ZCJCr7EYVHsx for ; Mon, 23 Jul 2018 13:09:11 -0400 (EDT) Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 B7F542294B for ; Mon, 23 Jul 2018 13:09:10 -0400 (EDT) From: "n.pettik" Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_B8C473BA-CD64-403C-B046-14CB391F58E6" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: prevent executing crossengine sql Date: Mon, 23 Jul 2018 20:09:08 +0300 In-Reply-To: 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 --Apple-Mail=_B8C473BA-CD64-403C-B046-14CB391F58E6 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 >>> 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? As far as I see (before this patch) we don=E2=80=99t call = txn_begin_ro_stmt() at all. > 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); Now, tarantool fails on last (4th) step due to obvious reason: SELECT results in explicit=20 >=20 >>> 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. >>>=20 >>>=20 >>>> + if (txn_begin_ro_stmt(space, &txn) !=3D 0) >>>> + return SQL_TARANTOOL_ERROR; >>>> + } >>>> struct iterator *it =3D index_create_iterator(pCur->index, = pCur->iter_type, >>>> key, part_count); >>>> if (it =3D=3D NULL) { >>>> + if (txn !=3D NULL) >>>> + txn_rollback_stmt(); >>>> pCur->eState =3D CURSOR_INVALID; >>>> return SQL_TARANTOOL_ITERATOR_FAIL; >>>> } >>>> + if (txn !=3D NULL) >>>> + txn_commit_ro_stmt(txn); >>>> pCur->iter =3D it; >>>> pCur->eState =3D CURSOR_VALID; --Apple-Mail=_B8C473BA-CD64-403C-B046-14CB391F58E6 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

2. I strongly do not like these 3 checks.

* space->id =3D=3D 0 can be skipped, since begin_ro_stmt = needs
only space engine.

* 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?

I am afraid of additional 'if's for such = internal thing.

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()).

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=E2=80=99t understand why we can=E2=80=99t avoid calling it for any = memtx space?
As far as I see (before this patch) we don=E2=80=99= t call txn_begin_ro_stmt() at all.

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=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);

Now, tarantool fails on last (4th) step due to = obvious reason: SELECT
results in explicit 


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) !=3D 0)
+ return = SQL_TARANTOOL_ERROR;
+ }
  struct = iterator *it =3D index_create_iterator(pCur->index, = pCur->iter_type,
      key, = part_count);
  if (it =3D=3D NULL) {
+ = = if (txn !=3D NULL)
+ txn_rollback_stmt();
  pCur->eState =3D CURSOR_INVALID;
  return SQL_TARANTOOL_ITERATOR_FAIL;
  }
+ if (txn !=3D NULL)
+ = = txn_commit_ro_stmt(txn);
  = pCur->iter =3D it;
  = pCur->eState =3D = CURSOR_VALID;

= --Apple-Mail=_B8C473BA-CD64-403C-B046-14CB391F58E6--