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 96BCC248BF for ; Mon, 22 Jul 2019 09:59:09 -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 P--KHA3NVG9O for ; Mon, 22 Jul 2019 09:59:09 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 CBFE324772 for ; Mon, 22 Jul 2019 09:59:08 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL From: "n.pettik" In-Reply-To: <5863bb4286a3e45b3521420273d89347bd331a92.1563576462.git.v.shpilevoy@tarantool.org> Date: Mon, 22 Jul 2019 16:59:05 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <5863bb4286a3e45b3521420273d89347bd331a92.1563576462.git.v.shpilevoy@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 Previous patch seems to be OK to me. > Box recently added support of transactional DDL allowing to do > any number of non-yielding DDL operations atomically. So, in SQL the only yielding operation (except for registered by user custom function) is CREATE INDEX involving non-empty space. Is this statement correct? If so, I guess we should add a few examples demonstrating new functionality to our SQL manual. > This is > really a big relief of one of the biggest pains of SQL. Before > this patch each multirow SQL DDL statement needed to prepare its > own rollback procedure for a case if something would go wrong. >=20 > Now with box support SQL wraps each DDL statement into a > transaction, and doesn't need own escape-routes in a form of > 'struct save_record' and others. >=20 > Closes #4086 > --- >=20 > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 2aefa2a3f..4884a7855 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c >=20 > @@ -3331,10 +3215,7 @@ vdbe_emit_halt_with_presence_test(struct Parse = *parser, int space_id, > } else { > sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, = error, > P4_DYNAMIC); > - if (is_clean_needed) > - save_record(parser, 0, 0, 0, v->nOp - 1, false); > - else > - sqlVdbeAddOp1(v, OP_Halt, -1); > + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); ABORT is set by default, isn=E2=80=99t it? See similar questions below. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 6a4a303b9..a71b331f8 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4373,15 +4373,16 @@ case OP_Update: { > */ > case OP_SInsert: { > assert(pOp->p1 > 0); > - assert(pOp->p2 > 0); > - assert(pOp->p3 >=3D 0); > + assert(pOp->p2 >=3D 0); >=20 > - pIn3 =3D &aMem[pOp->p3]; > + pIn2 =3D &aMem[pOp->p2]; > struct space *space =3D space_by_id(pOp->p1); > assert(space !=3D NULL); > assert(space_is_system(space)); > - if (tarantoolsqlInsert(space, pIn3->z, pIn3->z + pIn3->n) !=3D = 0) > - goto jump_to_p2; > + if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) !=3D = 0) { > + p->errorAction =3D ON_CONFLICT_ACTION_ABORT; > + goto abort_due_to_error; Why do we need set errorAction here? As I see, it is set to _ABORT by default: sqlVdbeRewind() -> sqlVdbeMakeReady() -> sql_finish_coding() > @@ -4404,8 +4405,10 @@ case OP_SDelete: { > struct space *space =3D space_by_id(pOp->p1); > assert(space !=3D NULL); > assert(space_is_system(space)); > - if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) !=3D 0) > + if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) !=3D 0) { > + p->errorAction =3D ON_CONFLICT_ACTION_ABORT; Same question concerning errorAction. > goto abort_due_to_error; > + } > if (pOp->p5 & OPFLAG_NCHANGE) > p->nChange++; > break; > diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua > new file mode 100644 > index 000000000..477158796 > --- /dev/null > +++ b/test/sql/ddl.test.lua > @@ -0,0 +1,187 @@ > +test_run =3D require('test_run').new() > +engine =3D test_run:get_cfg('engine') > +box.execute('pragma sql_default_engine=3D\''..engine..'\'') > + > +-- > +-- gh-4086: SQL transactional DDL. > +-- > +test_run:cmd("setopt delimiter ';'") > +box.begin() > +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);') > +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);') > +box.commit(); > +test_run:cmd("setopt delimiter ''"); > + > +box.space.T1 ~=3D nil > +box.space.T1.index[0] ~=3D nil > +box.space.T2 ~=3D nil > +box.space.T2.index[0] ~=3D nil > + > +test_run:cmd("setopt delimiter ';'") > +box.begin() > +box.execute('DROP TABLE t1;') > +assert(box.space.T1 =3D=3D nil) > +assert(box.space.T2 ~=3D nil) > +box.execute('DROP TABLE t2;') > +assert(box.space.T2 =3D=3D nil) > +box.commit(); > +test_run:cmd("setopt delimiter ''"); > + > +-- > +-- Use all the possible SQL DDL statements. > +-- > +test_run:cmd("setopt delimiter '$'") > +function monster_ddl() Still you have missed ALTER RENAME statement :) Also, AFAIR TRUNCATE is also considered to be DDL operation. Anyway, great tests, I like them. Personally, I=E2=80=99d write them in Lua format as well. IMHO *.sql.test would look a bit weird taking into account number of involved Lua functions. > + local _, err1, err2, err3 > + box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY, > + a INTEGER, > + b INTEGER);]]) > + box.execute([[CREATE TABLE t2(id INTEGER PRIMARY KEY, > + a INTEGER, > + b INTEGER UNIQUE, > + CONSTRAINT ck1 CHECK(b < 100));]]) > + > + box.execute('CREATE INDEX t1a ON t1(a);') > + box.execute('CREATE INDEX t2a ON t2(a);') > + box.execute('DROP INDEX t2a ON t2;') > + > + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);') > + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);') > + box.space.T1.ck_constraint.CK1:drop() > + > + box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY > + (a) REFERENCES t2(b);]]) > + box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;') > + > +-- Try random errors inside this big batch of DDL to ensure, that > +-- they do not affect normal operation. Nit: a bit broken comment's indentation..? > diff --git a/test/sql/view_delayed_wal.test.lua = b/test/sql/view_delayed_wal.test.lua > index 8e73b03f3..0a10d121b 100644 > --- a/test/sql/view_delayed_wal.test.lua > +++ b/test/sql/view_delayed_wal.test.lua > @@ -5,8 +5,8 @@ fiber =3D require('fiber') >=20 > -- View reference counters are incremented before firing > -- on_commit triggers (i.e. before being written into WAL), so > --- it is impossible to create view on dropped (but not written > --- into WAL) space. > +-- it is impossible to drop a space referenced by a created, but > +-- no committed view. Nit: I guess =E2=80=99*still* not yet committed view=E2=80=99 would = sound better.