From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL Date: Mon, 22 Jul 2019 16:59:05 +0300 [thread overview] Message-ID: <FE023A3F-AF35-41D8-BC46-490095333C19@tarantool.org> (raw) In-Reply-To: <5863bb4286a3e45b3521420273d89347bd331a92.1563576462.git.v.shpilevoy@tarantool.org> 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. > > 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. > > Closes #4086 > --- > > 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 > > @@ -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’t 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 >= 0); > + assert(pOp->p2 >= 0); > > - pIn3 = &aMem[pOp->p3]; > + pIn2 = &aMem[pOp->p2]; > struct space *space = space_by_id(pOp->p1); > assert(space != NULL); > assert(space_is_system(space)); > - if (tarantoolsqlInsert(space, pIn3->z, pIn3->z + pIn3->n) != 0) > - goto jump_to_p2; > + if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) { > + p->errorAction = 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 = space_by_id(pOp->p1); > assert(space != NULL); > assert(space_is_system(space)); > - if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) > + if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) { > + p->errorAction = 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 = require('test_run').new() > +engine = test_run:get_cfg('engine') > +box.execute('pragma sql_default_engine=\''..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 ~= nil > +box.space.T1.index[0] ~= nil > +box.space.T2 ~= nil > +box.space.T2.index[0] ~= nil > + > +test_run:cmd("setopt delimiter ';'") > +box.begin() > +box.execute('DROP TABLE t1;') > +assert(box.space.T1 == nil) > +assert(box.space.T2 ~= nil) > +box.execute('DROP TABLE t2;') > +assert(box.space.T2 == 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’d 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 = require('fiber') > > -- 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 ’*still* not yet committed view’ would sound better.
next prev parent reply other threads:[~2019-07-22 13:59 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-19 22:49 [tarantool-patches] [PATCH 0/2] SQL TXN DDL Vladislav Shpilevoy 2019-07-19 22:49 ` [tarantool-patches] [PATCH 1/2] alter: do not access space in drop ck commit trigger Vladislav Shpilevoy 2019-07-19 22:49 ` [tarantool-patches] [PATCH 2/2] sql: transactional DDL Vladislav Shpilevoy 2019-07-20 7:42 ` [tarantool-patches] " Konstantin Osipov 2019-07-20 14:02 ` Vladislav Shpilevoy 2019-07-20 15:16 ` Konstantin Osipov 2019-07-22 13:59 ` n.pettik [this message] 2019-07-22 21:35 ` Vladislav Shpilevoy 2019-07-24 13:23 ` n.pettik 2019-07-24 20:58 ` Vladislav Shpilevoy 2019-07-25 12:04 ` n.pettik 2019-07-31 13:19 ` [tarantool-patches] Re: [PATCH 0/2] SQL TXN DDL Kirill Yukhin
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=FE023A3F-AF35-41D8-BC46-490095333C19@tarantool.org \ --to=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL' \ /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