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 6866E22E1C for ; Mon, 22 Jul 2019 17:33:59 -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 Y1k3BIZk_kHU for ; Mon, 22 Jul 2019 17:33:59 -0400 (EDT) Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 52A0E22D46 for ; Mon, 22 Jul 2019 17:33:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL References: <5863bb4286a3e45b3521420273d89347bd331a92.1563576462.git.v.shpilevoy@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 22 Jul 2019 23:35:44 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org Hi! Thanks for the comments. On 22/07/2019 15:59, n.pettik wrote: > 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. Yes, CREATE INDEX should be the only one. I forgot to test it, and here is diff. Note, that I omitted *.test.lua file fixes, because otherwise the email gets too big. ================================================================================ diff --git a/test/sql/ddl.result b/test/sql/ddl.result index 8f7f91151..f96f7de9e 100644 --- a/test/sql/ddl.result +++ b/test/sql/ddl.result @@ -63,6 +63,93 @@ test_run:cmd("setopt delimiter ''"); | - true | ... +-- +-- Try to build an index transactionally. +-- +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER, b INTEGER)') + | --- + | - row_count: 1 + | ... +box.space.T1:replace{1, 1, 1} + | --- + | - [1, 1, 1] + | ... +box.space.T1:replace{2, 2, 2} + | --- + | - [2, 2, 2] + | ... +box.space.T1:replace{3, 3, 3} + | --- + | - [3, 3, 3] + | ... +box.space.T1:replace{4, 4, 4} + | --- + | - [4, 4, 4] + | ... +box.space.T1:replace{5, 5, 5} + | --- + | - [5, 5, 5] + | ... +-- Snapshot to dump Vinyl memory level, and force reading from a +-- disk, if someday create index will support transactions. +box.snapshot() + | --- + | - ok + | ... + +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +box.begin() +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)') +box.execute('CREATE INDEX t1a ON t1(a)') +box.execute('CREATE INDEX t1b ON t1(b)') +box.commit(); + | --- + | - error: Can not perform index build in a multi-statement transaction + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... +box.rollback() + | --- + | ... + +-- +-- Index drop does not yield, and is being done in background +-- later. So it is transactional. +-- +box.execute('CREATE INDEX t1a ON t1(a)') + | --- + | - row_count: 1 + | ... +box.execute('CREATE INDEX t1b ON t1(b)') + | --- + | - row_count: 1 + | ... + +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)') +box.execute('DROP INDEX t1a ON t1') +box.execute('DROP INDEX t1b ON t1') +test_run:cmd("setopt delimiter ''"); + | --- + | ... + +box.execute('DROP TABLE t1') + | --- + | - row_count: 1 + | ... +box.execute('DROP TABLE t2') + | --- + | - row_count: 1 + | ... + -- -- Use all the possible SQL DDL statements. -- ================================================================================ I also added a couple of tests on SQL transactions: ================================================================================ diff --git a/test/sql/ddl.result b/test/sql/ddl.result index f96f7de9e..73abc0c77 100644 --- a/test/sql/ddl.result +++ b/test/sql/ddl.result @@ -361,6 +361,45 @@ box.begin() monster_ddl_clear() box.commit() | --- | ... +-- Try SQL transactions. +box.execute('START TRANSACTION') res = {monster_ddl()} box.execute('COMMIT') + | --- + | ... +res + | --- + | - - 'Finished ok, errors in the middle: ' + | - Space 'T1' already exists + | - Space 'T3' does not exist + | - Index 'T1A' already exists in space 'T1' + | ... +monster_ddl_check() + | --- + | - 'Finished ok, errors and trigger catcher content: ' + | - 'Check constraint failed ''CK1'': b < 100' + | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2' + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed' + | - 'Check constraint failed ''CK2'': a > 0' + | - metadata: + | - name: ID + | type: integer + | rows: + | - [1] + | ... +box.execute('START TRANSACTION') monster_ddl_clear() box.execute('COMMIT') + | --- + | ... + +box.execute('START TRANSACTION') res = {monster_ddl()} box.execute('ROLLBACK') + | --- + | ... +res + | --- + | - - 'Finished ok, errors in the middle: ' + | - Space 'T1' already exists + | - Space 'T3' does not exist + | - Index 'T1A' already exists in space 'T1' + | ... + -- -- Voluntary rollback. -- ================================================================================ Talking of documentation - I added a docbot request. See at the end of the email for the whole patch and the commit message. > >> 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. In this place it was not set. OP_Halt takes an error action from p2, which was 0 here. 0 means rollback of the whole transaction, but we need a rollback of the current statement only - this is ON_CONFLICT_ACTION_ABORT. > >> 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() > Yes, here you are right. These gotos bypass OP_Halt, and terminate the execution directly, so p2 here does not matter, and p->errorAction is already set correctly. Here is diff: ================================================================================ diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index a71b331f8..e63d50382 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4379,10 +4379,9 @@ case OP_SInsert: { struct space *space = space_by_id(pOp->p1); assert(space != NULL); assert(space_is_system(space)); - if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) { - p->errorAction = ON_CONFLICT_ACTION_ABORT; + assert(p->errorAction == ON_CONFLICT_ACTION_ABORT); + if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) goto abort_due_to_error; - } if (pOp->p5 & OPFLAG_NCHANGE) p->nChange++; break; @@ -4405,10 +4404,9 @@ 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) { - p->errorAction = ON_CONFLICT_ACTION_ABORT; + assert(p->errorAction == ON_CONFLICT_ACTION_ABORT); + if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) goto abort_due_to_error; - } if (pOp->p5 & OPFLAG_NCHANGE) p->nChange++; break; ================================================================================ > >> @@ -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. The same, you are right. See diff and the explanation above. > >> 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 :) Indeed. Here it is: ================================================================================ diff --git a/test/sql/ddl.result b/test/sql/ddl.result index 73abc0c77..f233470f2 100644 --- a/test/sql/ddl.result +++ b/test/sql/ddl.result @@ -158,7 +158,9 @@ test_run:cmd("setopt delimiter '$'") | - true | ... function monster_ddl() - local _, err1, err2, err3 +-- Try random errors inside this big batch of DDL to ensure, that +-- they do not affect normal operation. + local _, err1, err2, err3, err4 box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER, b INTEGER);]]) @@ -169,9 +171,17 @@ function monster_ddl() box.execute('CREATE INDEX t1a ON t1(a);') box.execute('CREATE INDEX t2a ON t2(a);') + + box.execute('CREATE TABLE t_to_rename(id INTEGER PRIMARY KEY, a INTEGER);') + box.execute('DROP INDEX t2a ON t2;') + box.execute('CREATE INDEX t_to_rename_a ON t_to_rename(a);') + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);') + + _, err1 = pcall(box.execute, 'ALTER TABLE t_to_rename RENAME TO t1;') + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);') box.space.T1.ck_constraint.CK1:drop() @@ -179,9 +189,7 @@ function monster_ddl() (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. - _, err1 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);') + _, err2 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);') box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY (a) REFERENCES t2(b);]]) @@ -189,23 +197,27 @@ function monster_ddl() box.execute([[CREATE TABLE trigger_catcher(id INTEGER PRIMARY KEY AUTOINCREMENT);]]) + box.execute('ALTER TABLE t_to_rename RENAME TO t_renamed;') + + box.execute('DROP INDEX t_to_rename_a ON t_renamed;') + box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW BEGIN INSERT INTO trigger_catcher VALUES(1); END; ]]) - _, err2 = pcall(box.execute, 'DROP TABLE t3;') + _, err3 = pcall(box.execute, 'DROP TABLE t3;') box.execute([[CREATE TRIGGER t2t AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO trigger_catcher VALUES(1); END; ]]) - _, err3 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);') + _, err4 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);') box.execute('DROP TRIGGER t2t;') - return 'Finished ok, errors in the middle: ', err1, err2, err3 + return 'Finished ok, errors in the middle: ', err1, err2, err3, err4 end$ | --- | ... @@ -215,6 +227,8 @@ function monster_ddl_is_clean() assert(box.space._trigger:count() == 0) assert(box.space._fk_constraint:count() == 0) assert(box.space._ck_constraint:count() == 0) + assert(box.space.T_RENAMED == nil) + assert(box.space.T_TO_RENAME == nil) end$ | --- | ... @@ -227,6 +241,9 @@ function monster_ddl_check() _, err4 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, -1, 1)') box.execute('INSERT INTO t1 VALUES (1, 1, 1)') res = box.execute('SELECT * FROM trigger_catcher') + assert(box.space.T_RENAMED ~= nil) + assert(box.space.T_RENAMED.index.T_TO_RENAME_A == nil) + assert(box.space.T_TO_RENAME == nil) return 'Finished ok, errors and trigger catcher content: ', err1, err2, err3, err4, res end$ @@ -238,6 +255,7 @@ function monster_ddl_clear() pcall(box.execute, 'ALTER TABLE t1 DROP CONSTRAINT fk1;') box.execute('DROP TABLE IF EXISTS t2') box.execute('DROP TABLE IF EXISTS t1') + box.execute('DROP TABLE IF EXISTS t_renamed') monster_ddl_is_clean() end$ | --- @@ -252,6 +270,7 @@ monster_ddl() | --- | - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists + | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' | ... @@ -283,6 +302,7 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists + | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' | ... @@ -295,6 +315,7 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists + | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' | ... @@ -320,6 +341,7 @@ monster_ddl() | --- | - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists + | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' | ... @@ -369,6 +391,7 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists + | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' | ... @@ -396,6 +419,7 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists + | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' | ... @@ -467,6 +491,7 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists + | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' | ... ================================================================================ > Also, AFAIR TRUNCATE is also considered to be DDL > operation. Fair, added: ================================================================================ diff --git a/test/sql/ddl.result b/test/sql/ddl.result index 73abc0c77..f1f72e9d5 100644 --- a/test/sql/ddl.result +++ b/test/sql/ddl.result @@ -141,6 +141,47 @@ test_run:cmd("setopt delimiter ''"); | --- | ... +-- +-- Truncate should not be different from index drop in terms of +-- yields and atomicity. +-- +box.space.T2:replace{1} + | --- + | - [1] + | ... +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +function truncate_both() + box.execute('TRUNCATE TABLE t1;') + box.execute('TRUNCATE TABLE t2;') +end; + | --- + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... + +box.begin() truncate_both() box.rollback() + | --- + | ... + +box.space.T1:count() > 0 and box.space.T2:count() > 0 + | --- + | - true + | ... + +box.begin() truncate_both() box.commit() + | --- + | ... + +box.space.T1:count() == 0 and box.space.T2:count() == 0 + | --- + | - true + | ... + box.execute('DROP TABLE t1') | --- | - row_count: 1 @@ -158,7 +199,9 @@ test_run:cmd("setopt delimiter '$'") | - true | ... function monster_ddl() -- Try random errors inside this big batch of DDL to ensure, that -- they do not affect normal operation. - local _, err1, err2, err3, err4 + local _, err1, err2, err3, err4, err5, err6 box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER, b INTEGER);]]) @@ -189,23 +238,32 @@ function monster_ddl() _, err4 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);') + + box.execute('TRUNCATE TABLE t1;') + _, err5 = pcall(box.execute, 'TRUNCATE TABLE t2;') + _, err6 = pcall(box.execute, 'TRUNCATE TABLE t_does_not_exist;') box.execute('DROP TRIGGER t2t;') - return 'Finished ok, errors in the middle: ', err1, err2, err3, err4 + return 'Finished ok, errors in the middle: ', err1, err2, err3, err4, + err5, err6 end$ | --- | ... @@ -252,8 +316,12 @@ monster_ddl() | --- | - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' + | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other objects + | depend on it' + | - Space 'T_DOES_NOT_EXIST' does not exist | ... monster_ddl_check() | --- @@ -283,8 +351,12 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' + | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other + | objects depend on it' + | - Space 'T_DOES_NOT_EXIST' does not exist | ... -- DDL in txn, cleanup is not. @@ -295,8 +367,12 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' + | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other + | objects depend on it' + | - Space 'T_DOES_NOT_EXIST' does not exist | ... monster_ddl_check() | --- @@ -320,8 +396,12 @@ monster_ddl() | --- | - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' + | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other objects + | depend on it' + | - Space 'T_DOES_NOT_EXIST' does not exist | ... monster_ddl_check() | --- @@ -369,8 +449,12 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' + | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other + | objects depend on it' + | - Space 'T_DOES_NOT_EXIST' does not exist | ... monster_ddl_check() | --- @@ -396,8 +480,12 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' + | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other + | objects depend on it' + | - Space 'T_DOES_NOT_EXIST' does not exist | ... -- @@ -467,8 +555,12 @@ res | --- | - - 'Finished ok, errors in the middle: ' | - Space 'T1' already exists | - Space 'T1' already exists | - Space 'T3' does not exist | - Index 'T1A' already exists in space 'T1' + | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other + | objects depend on it' + | - Space 'T_DOES_NOT_EXIST' does not exist | ... box.commit() | --- ================================================================================ > 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..? No, it is a problem with test-run (perhaps, or probably with the console - don't know). If you set a delimiter, then comments can't have indentation. Otherwise you will get weird errors. > >> 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. > Perhaps. ================================================================================ diff --git a/test/sql/view_delayed_wal.result b/test/sql/view_delayed_wal.result index 3faaf07b8..d518e7d8c 100644 --- a/test/sql/view_delayed_wal.result +++ b/test/sql/view_delayed_wal.result @@ -14,7 +14,7 @@ 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 drop a space referenced by a created, but --- no committed view. +-- *still* not yet committed view. -- box.execute('CREATE TABLE t1(id INT PRIMARY KEY)') --- ================================================================================ Additionally, after the fixes there are too many repeating results of monster_ddl() and monster_ddl_check() calls. I moved their comparison into a function monster_ddl_cmp_res(). See the new patch: ================================================================================ sql: transactional DDL Box recently added support of transactional DDL allowing to do any number of non-yielding DDL operations atomically. 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 @TarantoolBot document Title: SQL DDL is transactional SQL DDL operations are atomic now. For example, if a CREATE TABLE request fails somewhere in the middle, it won't leave any garbage. Like a space without indexes, or unused sequences. Even if the instance is powered off during the request. Also, SQL DDL can be manually included into transactions, with certain limitations - such a transaction can't yield. For example, this is legal: START TRANSACTION; CREATE TABLE test(a INTEGER PRIMARY KEY, b INTEGER); CREATE INDEX test_a ON test(a); COMMIT; If you want to test it in the console, then wrap it into a function to do not get a rollback by yield, because the console yields after each command: function create() box.execute('START TRANSACTION;') box.execute('CREATE TABLE test(a INTEGER PRIMARY KEY, b INTEGER);') box.execute('CREATE INDEX test_a ON test(a);') box.execute('COMMIT;') end create() But the following example is illegal and you will get an error: box.execute('CREATE TABLE test(a INTEGER PRIMARY KEY, b INTEGER, c INTEGER);') box.execute('INSERT INTO test VALUES (1, 1, 1), (2, 2, 2), (3, 3, 3);') function index() box.execute('START TRANSACTION;') box.execute('CREATE INDEX test_b ON test(b);') box.execute('CREATE INDEX test_c ON test(c);') box.execute('COMMIT;') end tarantool> index() --- - error: Can not perform index build in a multi-statement transaction ... The error is because an attempt to build an index on a non-empty space leads to immediate yield. 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 @@ -57,64 +57,6 @@ #include "box/tuple_format.h" #include "box/coll_id_cache.h" -/** - * Structure that contains information about record that was - * inserted into system space. - */ -struct saved_record -{ - /** A link in a record list. */ - struct rlist link; - /** Id of space in which the record was inserted. */ - uint32_t space_id; - /** First register of the key of the record. */ - int reg_key; - /** Number of registers the key consists of. */ - int reg_key_count; - /** The address of the opcode. */ - int op_addr; - /** Flag to show that operation is SInsert. */ - bool is_insert; -}; - -/** - * Save inserted in system space record in list. This procedure is - * called after generation of either OP_SInsert or OP_NoColflict + - * OP_SetDiag. In the first case, record inserted to the system - * space is supposed to be deleted on error; in the latter - jump - * target specified in OP_SetDiag should be adjusted to the start - * of clean-up routines (current entry isn't inserted to the space - * yet, so there's no need to delete it). - * - * @param parser SQL Parser object. - * @param space_id Id of table in which record is inserted. - * @param reg_key Register that contains first field of the key. - * @param reg_key_count Exact number of fields of the key. - * @param op_addr Address of opcode (OP_SetDiag or OP_SInsert). - * Used to fix jump target (see - * sql_finish_coding()). - * @param is_insert_op Whether opcode is OP_SInsert or not. - */ -static inline void -save_record(struct Parse *parser, uint32_t space_id, int reg_key, - int reg_key_count, int op_addr, bool is_insert_op) -{ - struct saved_record *record = - region_alloc(&parser->region, sizeof(*record)); - if (record == NULL) { - diag_set(OutOfMemory, sizeof(*record), "region_alloc", - "record"); - parser->is_aborted = true; - return; - } - record->space_id = space_id; - record->reg_key = reg_key; - record->reg_key_count = reg_key_count; - record->op_addr = op_addr; - record->is_insert = is_insert_op; - rlist_add_entry(&parser->record_list, record, link); -} - void sql_finish_coding(struct Parse *parse_context) { @@ -122,52 +64,6 @@ sql_finish_coding(struct Parse *parse_context) struct sql *db = parse_context->db; struct Vdbe *v = sqlGetVdbe(parse_context); sqlVdbeAddOp0(v, OP_Halt); - /* - * In case statement "CREATE TABLE ..." fails it can - * left some records in system spaces that shouldn't be - * there. To clean-up properly this code is added. Last - * record isn't deleted because if statement fails than - * it won't be created. This code works the same way for - * other "CREATE ..." statements but it won't delete - * anything as these statements create no more than one - * record. Hence for processed insertions we should remove - * entries from corresponding system spaces alongside - * with fixing jump address for OP_SInsert opcode in - * case it fails during VDBE runtime; for OP_SetDiag only - * adjust jump target to the start of clean-up program - * for already inserted entries. - */ - if (!rlist_empty(&parse_context->record_list)) { - struct saved_record *record = - rlist_shift_entry(&parse_context->record_list, - struct saved_record, link); - /* - * Set jump target for OP_SetDiag and OP_SInsert. - */ - sqlVdbeChangeP2(v, record->op_addr, v->nOp); - MAYBE_UNUSED const char *comment = - "Delete entry from %s if CREATE TABLE fails"; - rlist_foreach_entry(record, &parse_context->record_list, link) { - if (record->is_insert) { - int rec_reg = ++parse_context->nMem; - sqlVdbeAddOp3(v, OP_MakeRecord, record->reg_key, - record->reg_key_count, rec_reg); - sqlVdbeAddOp2(v, OP_SDelete, record->space_id, - rec_reg); - MAYBE_UNUSED struct space *space = - space_by_id(record->space_id); - VdbeComment((v, comment, space_name(space))); - } - /* - * Set jump target for OP_SetDiag and - * OP_SInsert. - */ - sqlVdbeChangeP2(v, record->op_addr, v->nOp); - } - sqlVdbeAddOp1(v, OP_Halt, -1); - VdbeComment((v, - "Exit with an error if CREATE statement fails")); - } if (db->mallocFailed) parse_context->is_aborted = true; @@ -933,8 +829,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def, sqlVdbeAddOp4(v, OP_Blob, index_parts_sz, entry_reg + 5, SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC); sqlVdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg); - sqlVdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg); - save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1, true); + sqlVdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, tuple_reg); return; error: parse->is_aborted = true; @@ -991,9 +886,8 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg, sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6, SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC); sqlVdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, tuple_reg); - sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, tuple_reg); + sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, tuple_reg); sqlVdbeChangeP5(v, OPFLAG_NCHANGE); - save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1, true); return; error: pParse->is_aborted = true; @@ -1102,8 +996,7 @@ vdbe_emit_ck_constraint_create(struct Parse *parser, * Occupy registers for 5 fields: each member in * _ck_constraint space plus one for final msgpack tuple. */ - int ck_constraint_reg = parser->nMem + 1; - parser->nMem += 6; + int ck_constraint_reg = sqlGetTempRange(parser, 6); sqlVdbeAddOp2(v, OP_SCopy, reg_space_id, ck_constraint_reg); sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 1, 0, sqlDbStrDup(db, ck_def->name), P4_DYNAMIC); @@ -1120,13 +1013,12 @@ vdbe_emit_ck_constraint_create(struct Parse *parser, if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0, ck_constraint_reg, 2, ER_CONSTRAINT_EXISTS, error_msg, - false, OP_NoConflict, true) != 0) + false, OP_NoConflict) != 0) return; - sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0, + sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, ck_constraint_reg + 5); - save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2, - v->nOp - 1, true); VdbeComment((v, "Create CK constraint %s", ck_def->name)); + sqlReleaseTempRange(parser, ck_constraint_reg, 6); } /** @@ -1148,8 +1040,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context, * Occupy registers for 9 fields: each member in * _fk_constraint space plus one for final msgpack tuple. */ - int constr_tuple_reg = parse_context->nMem + 1; - parse_context->nMem += 10; + int constr_tuple_reg = sqlGetTempRange(parse_context, 10); char *name_copy = sqlDbStrDup(parse_context->db, fk->name); if (name_copy == NULL) return; @@ -1185,7 +1076,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context, BOX_FK_CONSTRAINT_ID, 0, constr_tuple_reg, 2, ER_CONSTRAINT_EXISTS, error_msg, - false, OP_NoConflict, true) != 0) + false, OP_NoConflict) != 0) return; sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3); sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0, @@ -1228,14 +1119,13 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context, parent_links, P4_DYNAMIC); sqlVdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9, constr_tuple_reg + 9); - sqlVdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0, - constr_tuple_reg + 9); + sqlVdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, + constr_tuple_reg + 9); if (parse_context->create_table_def.new_space == NULL) { sqlVdbeCountChanges(vdbe); sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE); } - save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2, - vdbe->nOp - 1, true); + sqlReleaseTempRange(parse_context, constr_tuple_reg, 10); return; error: parse_context->is_aborted = true; @@ -1331,7 +1221,7 @@ sqlEndTable(struct Parse *pParse) if (vdbe_emit_halt_with_presence_test(pParse, BOX_SPACE_ID, 2, name_reg, 1, ER_SPACE_EXISTS, error_msg, (no_err != 0), - OP_NoConflict, false) != 0) + OP_NoConflict) != 0) return; int reg_space_id = getNewSpaceId(pParse); @@ -1354,18 +1244,13 @@ sqlEndTable(struct Parse *pParse) int reg_seq_record = emitNewSysSequenceRecord(pParse, reg_seq_id, new_space->def->name); - sqlVdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0, - reg_seq_record); - save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1, - v->nOp - 1, true); + sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record); /* Do an insertion into _space_sequence. */ int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse, reg_space_id, reg_seq_id, new_space->index[0]->def); - sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, 0, - reg_space_seq_record); - save_record(pParse, BOX_SPACE_SEQUENCE_ID, - reg_space_seq_record + 1, 1, v->nOp - 1, true); + sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, + reg_space_seq_record); } /* Code creation of FK constraints, if any. */ struct fk_constraint_parse *fk_parse; @@ -1492,7 +1377,7 @@ sql_create_view(struct Parse *parse_context) if (vdbe_emit_halt_with_presence_test(parse_context, BOX_SPACE_ID, 2, name_reg, 1, ER_SPACE_EXISTS, error_msg, (no_err != 0), - OP_NoConflict, false) != 0) + OP_NoConflict) != 0) goto create_view_fail; vdbe_emit_space_create(parse_context, getNewSpaceId(parse_context), @@ -1611,7 +1496,7 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name, BOX_FK_CONSTRAINT_ID, 0, key_reg, 2, ER_NO_SUCH_CONSTRAINT, error_msg, false, - OP_Found, false) != 0) { + OP_Found) != 0) { sqlDbFree(parse_context->db, constraint_name); return; } @@ -1643,8 +1528,7 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name, tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name); if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0, key_reg, 2, ER_NO_SUCH_CONSTRAINT, - error_msg, false, - OP_Found, false) != 0) + error_msg, false, OP_Found) != 0) return; sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2); sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2); @@ -3310,7 +3194,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id, int index_id, int key_reg, uint32_t key_len, int tarantool_error_code, const char *error_src, bool no_error, - int cond_opcode, bool is_clean_needed) + int cond_opcode) { assert(cond_opcode == OP_NoConflict || cond_opcode == OP_Found); struct Vdbe *v = sqlGetVdbe(parser); @@ -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); } sqlVdbeJumpHere(v, addr); sqlVdbeAddOp1(v, OP_Close, cursor); diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 2a60ad25b..06eab7f2d 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -172,6 +172,7 @@ cmd ::= create_table create_table_args. create_table ::= CREATE TABLE ifnotexists(E) nm(Y). { create_table_def_init(&pParse->create_table_def, &Y, E); pParse->create_table_def.new_space = sqlStartTable(pParse, &Y); + pParse->initiateTTrans = true; } %type ifnotexists {int} @@ -380,12 +381,14 @@ resolvetype(A) ::= REPLACE. {A = ON_CONFLICT_ACTION_REPLACE;} cmd ::= DROP TABLE ifexists(E) fullname(X) . { struct Token t = Token_nil; drop_table_def_init(&pParse->drop_table_def, X, &t, E); + pParse->initiateTTrans = true; sql_drop_table(pParse); } cmd ::= DROP VIEW ifexists(E) fullname(X) . { struct Token t = Token_nil; drop_view_def_init(&pParse->drop_view_def, X, &t, E); + pParse->initiateTTrans = true; sql_drop_table(pParse); } @@ -399,6 +402,7 @@ cmd ::= CREATE(X) VIEW ifnotexists(E) nm(Y) eidlist_opt(C) AS select(S). { if (!pParse->parse_only) { create_view_def_init(&pParse->create_view_def, &Y, &X, C, S, E); + pParse->initiateTTrans = true; sql_create_view(pParse); } else { sql_store_select(pParse, S); @@ -1404,6 +1408,7 @@ cmd ::= CREATE uniqueflag(U) INDEX ifnotexists(NE) nm(X) } create_index_def_init(&pParse->create_index_def, src_list, &X, Z, U, SORT_ORDER_ASC, NE); + pParse->initiateTTrans = true; sql_create_index(pParse); } @@ -1456,6 +1461,7 @@ eidlist(A) ::= nm(Y). { // cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). { drop_index_def_init(&pParse->drop_index_def, Y, &X, E); + pParse->initiateTTrans = true; sql_drop_index(pParse); } @@ -1502,7 +1508,7 @@ cmd ::= CREATE trigger_decl(A) BEGIN trigger_cmd_list(S) END(Z). { Token all; all.z = A.z; all.n = (int)(Z.z - A.z) + Z.n; - pParse->initiateTTrans = false; + pParse->initiateTTrans = true; sql_trigger_finish(pParse, S, &all); } @@ -1652,6 +1658,7 @@ raisetype(A) ::= FAIL. {A = ON_CONFLICT_ACTION_FAIL;} cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). { struct Token t = Token_nil; drop_trigger_def_init(&pParse->drop_trigger_def, X, &t, NOERR); + pParse->initiateTTrans = true; sql_drop_trigger(pParse); } @@ -1671,6 +1678,7 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; } alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). { A.table_name = T; A.name = N; + pParse->initiateTTrans = true; } cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES @@ -1697,11 +1705,13 @@ unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; } cmd ::= alter_table_start(A) RENAME TO nm(N). { rename_entity_def_init(&pParse->rename_entity_def, A, &N); + pParse->initiateTTrans = true; sql_alter_table_rename(pParse); } cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false); + pParse->initiateTTrans = true; sql_drop_foreign_key(pParse); } diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index 84fb31bcd..36c21a221 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -243,7 +243,6 @@ sql_parser_create(struct Parse *parser, struct sql *db, uint32_t sql_flags) memset(parser, 0, sizeof(struct Parse)); parser->db = db; parser->sql_flags = sql_flags; - rlist_create(&parser->record_list); region_create(&parser->region, &cord()->slabc); } diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 4f5bad287..99296b4b3 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2329,11 +2329,6 @@ struct Parse { * sqlEndTable() function). */ struct create_table_def create_table_def; - /** - * List of all records that were inserted in system spaces - * in current statement. - */ - struct rlist record_list; bool initiateTTrans; /* Initiate Tarantool transaction */ /** If set - do not emit byte code at all, just parse. */ bool parse_only; @@ -4542,7 +4537,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id, int index_id, int key_reg, uint32_t key_len, int tarantool_error_code, const char *error_src, bool no_error, - int cond_opcode, bool is_clean_needed); + int cond_opcode); /** * Generate VDBE code to delete records from system _sql_stat1 or diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index 562723959..d746ef893 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -112,8 +112,7 @@ sql_trigger_begin(struct Parse *parse) name_reg, 1, ER_TRIGGER_EXISTS, error_msg, (no_err != 0), - OP_NoConflict, - false) != 0) + OP_NoConflict) != 0) goto trigger_cleanup; } @@ -421,8 +420,7 @@ sql_drop_trigger(struct Parse *parser) sqlVdbeAddOp4(v, OP_String8, 0, name_reg, 0, name_copy, P4_DYNAMIC); if (vdbe_emit_halt_with_presence_test(parser, BOX_TRIGGER_ID, 0, name_reg, 1, ER_NO_SUCH_TRIGGER, - error_msg, no_err, OP_Found, - false) != 0) + error_msg, no_err, OP_Found) != 0) goto drop_trigger_cleanup; vdbe_code_drop_trigger(parser, trigger_name, true); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 6a4a303b9..e63d50382 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4359,8 +4359,8 @@ case OP_Update: { break; } -/* Opcode: SInsert P1 P2 P3 * P5 - * Synopsis: space id = P1, key = r[P3], on error goto P2 +/* Opcode: SInsert P1 P2 * * P5 + * Synopsis: space id = P1, key = r[P2] * * This opcode is used only during DDL routine. * In contrast to ordinary insertion, insertion to system spaces @@ -4373,15 +4373,15 @@ 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; + assert(p->errorAction == ON_CONFLICT_ACTION_ABORT); + if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) + goto abort_due_to_error; if (pOp->p5 & OPFLAG_NCHANGE) p->nChange++; break; @@ -4404,6 +4404,7 @@ case OP_SDelete: { struct space *space = space_by_id(pOp->p1); assert(space != NULL); assert(space_is_system(space)); + assert(p->errorAction == ON_CONFLICT_ACTION_ABORT); if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) goto abort_due_to_error; if (pOp->p5 & OPFLAG_NCHANGE) diff --git a/test/sql/ddl.result b/test/sql/ddl.result new file mode 100644 index 000000000..70fb2222e --- /dev/null +++ b/test/sql/ddl.result @@ -0,0 +1,542 @@ +-- test-run result file version 2 +test_run = require('test_run').new() + | --- + | ... +json = require('json') + | --- + | ... +engine = test_run:get_cfg('engine') + | --- + | ... +box.execute('pragma sql_default_engine=\''..engine..'\'') + | --- + | - row_count: 0 + | ... + +-- +-- gh-4086: SQL transactional DDL. +-- +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +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 ''"); + | --- + | - true + | ... + +box.space.T1 ~= nil + | --- + | - true + | ... +box.space.T1.index[0] ~= nil + | --- + | - true + | ... +box.space.T2 ~= nil + | --- + | - true + | ... +box.space.T2.index[0] ~= nil + | --- + | - true + | ... + +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +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 ''"); + | --- + | - true + | ... + +-- +-- Try to build an index transactionally. +-- +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER, b INTEGER)') + | --- + | - row_count: 1 + | ... +box.space.T1:replace{1, 1, 1} + | --- + | - [1, 1, 1] + | ... +box.space.T1:replace{2, 2, 2} + | --- + | - [2, 2, 2] + | ... +box.space.T1:replace{3, 3, 3} + | --- + | - [3, 3, 3] + | ... +box.space.T1:replace{4, 4, 4} + | --- + | - [4, 4, 4] + | ... +box.space.T1:replace{5, 5, 5} + | --- + | - [5, 5, 5] + | ... +-- Snapshot to dump Vinyl memory level, and force reading from a +-- disk, if someday create index will support transactions. +box.snapshot() + | --- + | - ok + | ... + +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +box.begin() +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)') +box.execute('CREATE INDEX t1a ON t1(a)') +box.execute('CREATE INDEX t1b ON t1(b)') +box.commit(); + | --- + | - error: Can not perform index build in a multi-statement transaction + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... +box.rollback() + | --- + | ... + +-- +-- Index drop does not yield, and is being done in background +-- later. So it is transactional. +-- +box.execute('CREATE INDEX t1a ON t1(a)') + | --- + | - row_count: 1 + | ... +box.execute('CREATE INDEX t1b ON t1(b)') + | --- + | - row_count: 1 + | ... + +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)') +box.execute('DROP INDEX t1a ON t1') +box.execute('DROP INDEX t1b ON t1') +test_run:cmd("setopt delimiter ''"); + | --- + | ... + +-- +-- Truncate should not be different from index drop in terms of +-- yields and atomicity. +-- +box.space.T2:replace{1} + | --- + | - [1] + | ... +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +function truncate_both() + box.execute('TRUNCATE TABLE t1;') + box.execute('TRUNCATE TABLE t2;') +end; + | --- + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... + +box.begin() truncate_both() box.rollback() + | --- + | ... + +box.space.T1:count() > 0 and box.space.T2:count() > 0 + | --- + | - true + | ... + +box.begin() truncate_both() box.commit() + | --- + | ... + +box.space.T1:count() == 0 and box.space.T2:count() == 0 + | --- + | - true + | ... + +box.execute('DROP TABLE t1') + | --- + | - row_count: 1 + | ... +box.execute('DROP TABLE t2') + | --- + | - row_count: 1 + | ... + +-- +-- Use all the possible SQL DDL statements. +-- +test_run:cmd("setopt delimiter '$'") + | --- + | - true + | ... +function monster_ddl() +-- Try random errors inside this big batch of DDL to ensure, that +-- they do not affect normal operation. + local _, err1, err2, err3, err4, err5, err6 + 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('CREATE TABLE t_to_rename(id INTEGER PRIMARY KEY, a INTEGER);') + + box.execute('DROP INDEX t2a ON t2;') + + box.execute('CREATE INDEX t_to_rename_a ON t_to_rename(a);') + + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);') + + _, err1 = pcall(box.execute, 'ALTER TABLE t_to_rename RENAME TO t1;') + + 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;') + + _, err2 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);') + + box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY + (a) REFERENCES t2(b);]]) + + box.execute([[CREATE TABLE trigger_catcher(id INTEGER PRIMARY + KEY AUTOINCREMENT);]]) + + box.execute('ALTER TABLE t_to_rename RENAME TO t_renamed;') + + box.execute('DROP INDEX t_to_rename_a ON t_renamed;') + + box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW + BEGIN + INSERT INTO trigger_catcher VALUES(1); + END; ]]) + + _, err3 = pcall(box.execute, 'DROP TABLE t3;') + + box.execute([[CREATE TRIGGER t2t AFTER INSERT ON t2 FOR EACH ROW + BEGIN + INSERT INTO trigger_catcher VALUES(1); + END; ]]) + + _, err4 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);') + + box.execute('TRUNCATE TABLE t1;') + _, err5 = pcall(box.execute, 'TRUNCATE TABLE t2;') + _, err6 = pcall(box.execute, 'TRUNCATE TABLE t_does_not_exist;') + + box.execute('DROP TRIGGER t2t;') + + return {'Finished ok, errors in the middle: ', err1, err2, err3, err4, + err5, err6} +end$ + | --- + | ... +function monster_ddl_cmp_res(res1, res2) + if json.encode(res1) == json.encode(res2) then + return true + end + return res1, res2 +end$ + | --- + | ... +function monster_ddl_is_clean() + assert(box.space.T1 == nil) + assert(box.space.T2 == nil) + assert(box.space._trigger:count() == 0) + assert(box.space._fk_constraint:count() == 0) + assert(box.space._ck_constraint:count() == 0) + assert(box.space.T_RENAMED == nil) + assert(box.space.T_TO_RENAME == nil) +end$ + | --- + | ... +function monster_ddl_check() + local _, err1, err2, err3, err4, res + _, err1 = pcall(box.execute, 'INSERT INTO t2 VALUES (1, 1, 101)') + box.execute('INSERT INTO t2 VALUES (1, 1, 1)') + _, err2 = pcall(box.execute, 'INSERT INTO t2 VALUES(2, 2, 1)') + _, err3 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, 20, 1)') + _, err4 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, -1, 1)') + box.execute('INSERT INTO t1 VALUES (1, 1, 1)') + res = box.execute('SELECT * FROM trigger_catcher') + assert(box.space.T_RENAMED ~= nil) + assert(box.space.T_RENAMED.index.T_TO_RENAME_A == nil) + assert(box.space.T_TO_RENAME == nil) + return {'Finished ok, errors and trigger catcher content: ', err1, err2, + err3, err4, res} +end$ + | --- + | ... +function monster_ddl_clear() + box.execute('DROP TRIGGER IF EXISTS t1t;') + box.execute('DROP TABLE IF EXISTS trigger_catcher;') + pcall(box.execute, 'ALTER TABLE t1 DROP CONSTRAINT fk1;') + box.execute('DROP TABLE IF EXISTS t2') + box.execute('DROP TABLE IF EXISTS t1') + box.execute('DROP TABLE IF EXISTS t_renamed') + monster_ddl_is_clean() +end$ + | --- + | ... +test_run:cmd("setopt delimiter ''")$ + | --- + | - true + | ... + +-- No txn. +true_ddl_res = monster_ddl() + | --- + | ... +true_ddl_res + | --- + | - - 'Finished ok, errors in the middle: ' + | - Space 'T1' already exists + | - Space 'T1' already exists + | - Space 'T3' does not exist + | - Index 'T1A' already exists in space 'T1' + | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other + | objects depend on it' + | - Space 'T_DOES_NOT_EXIST' does not exist + | ... + +true_check_res = monster_ddl_check() + | --- + | ... +true_check_res + | --- + | - - 'Finished ok, errors and trigger catcher content: ' + | - 'Check constraint failed ''CK1'': b < 100' + | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2' + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed' + | - 'Check constraint failed ''CK2'': a > 0' + | - metadata: + | - name: ID + | type: integer + | rows: + | - [1] + | ... + +monster_ddl_clear() + | --- + | ... + +-- Both DDL and cleanup in one txn. +ddl_res = nil + | --- + | ... +box.begin() ddl_res = monster_ddl() monster_ddl_clear() box.commit() + | --- + | ... +monster_ddl_cmp_res(ddl_res, true_ddl_res) + | --- + | - true + | ... + +-- DDL in txn, cleanup is not. +box.begin() ddl_res = monster_ddl() box.commit() + | --- + | ... +monster_ddl_cmp_res(ddl_res, true_ddl_res) + | --- + | - true + | ... + +check_res = monster_ddl_check() + | --- + | ... +monster_ddl_cmp_res(check_res, true_check_res) + | --- + | - true + | ... + +monster_ddl_clear() + | --- + | ... + +-- DDL is not in txn, cleanup is. +ddl_res = monster_ddl() + | --- + | ... +monster_ddl_cmp_res(ddl_res, true_ddl_res) + | --- + | - true + | ... + +check_res = monster_ddl_check() + | --- + | ... +monster_ddl_cmp_res(check_res, true_check_res) + | --- + | - true + | ... + +box.begin() monster_ddl_clear() box.commit() + | --- + | ... + +-- DDL and cleanup in separate txns. +box.begin() ddl_res = monster_ddl() box.commit() + | --- + | ... +monster_ddl_cmp_res(ddl_res, true_ddl_res) + | --- + | - true + | ... + +check_res = monster_ddl_check() + | --- + | ... +monster_ddl_cmp_res(check_res, true_check_res) + | --- + | - true + | ... + +box.begin() monster_ddl_clear() box.commit() + | --- + | ... + +-- Try SQL transactions. +box.execute('START TRANSACTION') ddl_res = monster_ddl() box.execute('COMMIT') + | --- + | ... +monster_ddl_cmp_res(ddl_res, true_ddl_res) + | --- + | - true + | ... + +check_res = monster_ddl_check() + | --- + | ... +monster_ddl_cmp_res(check_res, true_check_res) + | --- + | - true + | ... + +box.execute('START TRANSACTION') monster_ddl_clear() box.execute('COMMIT') + | --- + | ... + +box.execute('START TRANSACTION') ddl_res = monster_ddl() box.execute('ROLLBACK') + | --- + | ... +monster_ddl_cmp_res(ddl_res, true_ddl_res) + | --- + | - true + | ... + +-- +-- Voluntary rollback. +-- +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +box.begin() +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);') +assert(box.space.T1 ~= nil) +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);') +assert(box.space.T2 ~= nil) +box.rollback(); + | --- + | ... + +box.space.T1 == nil and box.space.T2 == nil; + | --- + | - true + | ... + +box.begin() +save1 = box.savepoint() +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY)') +save2 = box.savepoint() +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER)') +box.execute('CREATE INDEX t2a ON t2(a)') +save3 = box.savepoint() +assert(box.space.T1 ~= nil) +assert(box.space.T2 ~= nil) +assert(box.space.T2.index.T2A ~= nil) +box.execute('DROP TABLE t2') +assert(box.space.T2 == nil) +box.rollback_to_savepoint(save3) +assert(box.space.T2 ~= nil) +assert(box.space.T2.index.T2A ~= nil) +save3 = box.savepoint() +box.execute('DROP TABLE t2') +assert(box.space.T2 == nil) +box.rollback_to_savepoint(save2) +assert(box.space.T2 == nil) +assert(box.space.T1 ~= nil) +box.rollback_to_savepoint(save1) +box.commit(); + | --- + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... + +box.space.T1 == nil and box.space.T2 == nil + | --- + | - true + | ... + +-- +-- Unexpected rollback. +-- + +box.begin() ddl_res = monster_ddl() require('fiber').yield() + | --- + | ... +monster_ddl_cmp_res(ddl_res, true_ddl_res) + | --- + | - true + | ... +box.commit() + | --- + | - error: Transaction has been aborted by a fiber yield + | ... +box.rollback() + | --- + | ... +monster_ddl_clear() + | --- + | ... diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua new file mode 100644 index 000000000..74fc20ea9 --- /dev/null +++ b/test/sql/ddl.test.lua @@ -0,0 +1,302 @@ +test_run = require('test_run').new() +json = require('json') +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 ''"); + +-- +-- Try to build an index transactionally. +-- +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER, b INTEGER)') +box.space.T1:replace{1, 1, 1} +box.space.T1:replace{2, 2, 2} +box.space.T1:replace{3, 3, 3} +box.space.T1:replace{4, 4, 4} +box.space.T1:replace{5, 5, 5} +-- Snapshot to dump Vinyl memory level, and force reading from a +-- disk, if someday create index will support transactions. +box.snapshot() + +test_run:cmd("setopt delimiter ';'") +box.begin() +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)') +box.execute('CREATE INDEX t1a ON t1(a)') +box.execute('CREATE INDEX t1b ON t1(b)') +box.commit(); +test_run:cmd("setopt delimiter ''"); +box.rollback() + +-- +-- Index drop does not yield, and is being done in background +-- later. So it is transactional. +-- +box.execute('CREATE INDEX t1a ON t1(a)') +box.execute('CREATE INDEX t1b ON t1(b)') + +test_run:cmd("setopt delimiter ';'") +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)') +box.execute('DROP INDEX t1a ON t1') +box.execute('DROP INDEX t1b ON t1') +test_run:cmd("setopt delimiter ''"); + +-- +-- Truncate should not be different from index drop in terms of +-- yields and atomicity. +-- +box.space.T2:replace{1} +test_run:cmd("setopt delimiter ';'") +function truncate_both() + box.execute('TRUNCATE TABLE t1;') + box.execute('TRUNCATE TABLE t2;') +end; +test_run:cmd("setopt delimiter ''"); + +box.begin() truncate_both() box.rollback() + +box.space.T1:count() > 0 and box.space.T2:count() > 0 + +box.begin() truncate_both() box.commit() + +box.space.T1:count() == 0 and box.space.T2:count() == 0 + +box.execute('DROP TABLE t1') +box.execute('DROP TABLE t2') + +-- +-- Use all the possible SQL DDL statements. +-- +test_run:cmd("setopt delimiter '$'") +function monster_ddl() +-- Try random errors inside this big batch of DDL to ensure, that +-- they do not affect normal operation. + local _, err1, err2, err3, err4, err5, err6 + 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('CREATE TABLE t_to_rename(id INTEGER PRIMARY KEY, a INTEGER);') + + box.execute('DROP INDEX t2a ON t2;') + + box.execute('CREATE INDEX t_to_rename_a ON t_to_rename(a);') + + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);') + + _, err1 = pcall(box.execute, 'ALTER TABLE t_to_rename RENAME TO t1;') + + 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;') + + _, err2 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);') + + box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY + (a) REFERENCES t2(b);]]) + + box.execute([[CREATE TABLE trigger_catcher(id INTEGER PRIMARY + KEY AUTOINCREMENT);]]) + + box.execute('ALTER TABLE t_to_rename RENAME TO t_renamed;') + + box.execute('DROP INDEX t_to_rename_a ON t_renamed;') + + box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW + BEGIN + INSERT INTO trigger_catcher VALUES(1); + END; ]]) + + _, err3 = pcall(box.execute, 'DROP TABLE t3;') + + box.execute([[CREATE TRIGGER t2t AFTER INSERT ON t2 FOR EACH ROW + BEGIN + INSERT INTO trigger_catcher VALUES(1); + END; ]]) + + _, err4 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);') + + box.execute('TRUNCATE TABLE t1;') + _, err5 = pcall(box.execute, 'TRUNCATE TABLE t2;') + _, err6 = pcall(box.execute, 'TRUNCATE TABLE t_does_not_exist;') + + box.execute('DROP TRIGGER t2t;') + + return {'Finished ok, errors in the middle: ', err1, err2, err3, err4, + err5, err6} +end$ +function monster_ddl_cmp_res(res1, res2) + if json.encode(res1) == json.encode(res2) then + return true + end + return res1, res2 +end$ +function monster_ddl_is_clean() + assert(box.space.T1 == nil) + assert(box.space.T2 == nil) + assert(box.space._trigger:count() == 0) + assert(box.space._fk_constraint:count() == 0) + assert(box.space._ck_constraint:count() == 0) + assert(box.space.T_RENAMED == nil) + assert(box.space.T_TO_RENAME == nil) +end$ +function monster_ddl_check() + local _, err1, err2, err3, err4, res + _, err1 = pcall(box.execute, 'INSERT INTO t2 VALUES (1, 1, 101)') + box.execute('INSERT INTO t2 VALUES (1, 1, 1)') + _, err2 = pcall(box.execute, 'INSERT INTO t2 VALUES(2, 2, 1)') + _, err3 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, 20, 1)') + _, err4 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, -1, 1)') + box.execute('INSERT INTO t1 VALUES (1, 1, 1)') + res = box.execute('SELECT * FROM trigger_catcher') + assert(box.space.T_RENAMED ~= nil) + assert(box.space.T_RENAMED.index.T_TO_RENAME_A == nil) + assert(box.space.T_TO_RENAME == nil) + return {'Finished ok, errors and trigger catcher content: ', err1, err2, + err3, err4, res} +end$ +function monster_ddl_clear() + box.execute('DROP TRIGGER IF EXISTS t1t;') + box.execute('DROP TABLE IF EXISTS trigger_catcher;') + pcall(box.execute, 'ALTER TABLE t1 DROP CONSTRAINT fk1;') + box.execute('DROP TABLE IF EXISTS t2') + box.execute('DROP TABLE IF EXISTS t1') + box.execute('DROP TABLE IF EXISTS t_renamed') + monster_ddl_is_clean() +end$ +test_run:cmd("setopt delimiter ''")$ + +-- No txn. +true_ddl_res = monster_ddl() +true_ddl_res + +true_check_res = monster_ddl_check() +true_check_res + +monster_ddl_clear() + +-- Both DDL and cleanup in one txn. +ddl_res = nil +box.begin() ddl_res = monster_ddl() monster_ddl_clear() box.commit() +monster_ddl_cmp_res(ddl_res, true_ddl_res) + +-- DDL in txn, cleanup is not. +box.begin() ddl_res = monster_ddl() box.commit() +monster_ddl_cmp_res(ddl_res, true_ddl_res) + +check_res = monster_ddl_check() +monster_ddl_cmp_res(check_res, true_check_res) + +monster_ddl_clear() + +-- DDL is not in txn, cleanup is. +ddl_res = monster_ddl() +monster_ddl_cmp_res(ddl_res, true_ddl_res) + +check_res = monster_ddl_check() +monster_ddl_cmp_res(check_res, true_check_res) + +box.begin() monster_ddl_clear() box.commit() + +-- DDL and cleanup in separate txns. +box.begin() ddl_res = monster_ddl() box.commit() +monster_ddl_cmp_res(ddl_res, true_ddl_res) + +check_res = monster_ddl_check() +monster_ddl_cmp_res(check_res, true_check_res) + +box.begin() monster_ddl_clear() box.commit() + +-- Try SQL transactions. +box.execute('START TRANSACTION') ddl_res = monster_ddl() box.execute('COMMIT') +monster_ddl_cmp_res(ddl_res, true_ddl_res) + +check_res = monster_ddl_check() +monster_ddl_cmp_res(check_res, true_check_res) + +box.execute('START TRANSACTION') monster_ddl_clear() box.execute('COMMIT') + +box.execute('START TRANSACTION') ddl_res = monster_ddl() box.execute('ROLLBACK') +monster_ddl_cmp_res(ddl_res, true_ddl_res) + +-- +-- Voluntary rollback. +-- +test_run:cmd("setopt delimiter ';'") +box.begin() +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);') +assert(box.space.T1 ~= nil) +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);') +assert(box.space.T2 ~= nil) +box.rollback(); + +box.space.T1 == nil and box.space.T2 == nil; + +box.begin() +save1 = box.savepoint() +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY)') +save2 = box.savepoint() +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER)') +box.execute('CREATE INDEX t2a ON t2(a)') +save3 = box.savepoint() +assert(box.space.T1 ~= nil) +assert(box.space.T2 ~= nil) +assert(box.space.T2.index.T2A ~= nil) +box.execute('DROP TABLE t2') +assert(box.space.T2 == nil) +box.rollback_to_savepoint(save3) +assert(box.space.T2 ~= nil) +assert(box.space.T2.index.T2A ~= nil) +save3 = box.savepoint() +box.execute('DROP TABLE t2') +assert(box.space.T2 == nil) +box.rollback_to_savepoint(save2) +assert(box.space.T2 == nil) +assert(box.space.T1 ~= nil) +box.rollback_to_savepoint(save1) +box.commit(); +test_run:cmd("setopt delimiter ''"); + +box.space.T1 == nil and box.space.T2 == nil + +-- +-- Unexpected rollback. +-- + +box.begin() ddl_res = monster_ddl() require('fiber').yield() +monster_ddl_cmp_res(ddl_res, true_ddl_res) +box.commit() +box.rollback() +monster_ddl_clear() diff --git a/test/sql/errinj.result b/test/sql/errinj.result index 8846e5ee8..257dbafde 100644 --- a/test/sql/errinj.result +++ b/test/sql/errinj.result @@ -388,56 +388,6 @@ box.execute("DROP TABLE t3;") --- - row_count: 1 ... --- gh-3780: space without PK raises error if --- it is used in SQL queries. --- -errinj = box.error.injection ---- -... -fiber = require('fiber') ---- -... -box.execute("CREATE TABLE t (id INT PRIMARY KEY);") ---- -- row_count: 1 -... -box.execute("INSERT INTO t VALUES (1);") ---- -- row_count: 1 -... -errinj.set("ERRINJ_WAL_DELAY", true) ---- -- ok -... --- DROP TABLE consists of several steps: firstly indexes --- are deleted, then space itself. Lets make sure that if --- first part of drop is successfully finished, but resulted --- in yield, all operations on space will be blocked due to --- absence of primary key. --- -function drop_table_yield() box.execute("DROP TABLE t;") end ---- -... -f = fiber.create(drop_table_yield) ---- -... -box.execute("SELECT * FROM t;") ---- -- error: SQL does not support spaces without primary key -... -box.execute("INSERT INTO t VALUES (2);") ---- -- error: SQL does not support spaces without primary key -... -box.execute("UPDATE t SET id = 2;") ---- -- error: SQL does not support spaces without primary key -... --- Finish drop space. -errinj.set("ERRINJ_WAL_DELAY", false) ---- -- ok -... -- -- gh-3931: Store regular identifiers in case-normal form -- diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua index 48b80a443..3bc1b684d 100644 --- a/test/sql/errinj.test.lua +++ b/test/sql/errinj.test.lua @@ -118,28 +118,6 @@ box.execute("INSERT INTO t3 VALUES(1, 1, 3);") errinj.set("ERRINJ_WAL_IO", false) box.execute("DROP TABLE t3;") --- gh-3780: space without PK raises error if --- it is used in SQL queries. --- -errinj = box.error.injection -fiber = require('fiber') -box.execute("CREATE TABLE t (id INT PRIMARY KEY);") -box.execute("INSERT INTO t VALUES (1);") -errinj.set("ERRINJ_WAL_DELAY", true) --- DROP TABLE consists of several steps: firstly indexes --- are deleted, then space itself. Lets make sure that if --- first part of drop is successfully finished, but resulted --- in yield, all operations on space will be blocked due to --- absence of primary key. --- -function drop_table_yield() box.execute("DROP TABLE t;") end -f = fiber.create(drop_table_yield) -box.execute("SELECT * FROM t;") -box.execute("INSERT INTO t VALUES (2);") -box.execute("UPDATE t SET id = 2;") --- Finish drop space. -errinj.set("ERRINJ_WAL_DELAY", false) - -- -- gh-3931: Store regular identifiers in case-normal form -- diff --git a/test/sql/view_delayed_wal.result b/test/sql/view_delayed_wal.result index 519794931..d518e7d8c 100644 --- a/test/sql/view_delayed_wal.result +++ b/test/sql/view_delayed_wal.result @@ -13,8 +13,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 +-- *still* not yet committed view. -- box.execute('CREATE TABLE t1(id INT PRIMARY KEY)') --- @@ -49,13 +49,13 @@ box.error.injection.set("ERRINJ_WAL_DELAY", false) fiber.sleep(0.1) --- ... -box.space.T1 +box.space.T1 ~= nil --- -- null +- true ... -box.space.V1 +box.space.V1 ~= nil --- -- null +- true ... -- -- In the same way, we have to drop all referenced spaces before diff --git a/test/sql/view_delayed_wal.test.lua b/test/sql/view_delayed_wal.test.lua index 8e73b03f3..7e6fce6ca 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 +-- *still* not yet committed view. -- box.execute('CREATE TABLE t1(id INT PRIMARY KEY)') function create_view() box.execute('CREATE VIEW v1 AS SELECT * FROM t1') end @@ -18,8 +18,8 @@ f2 = fiber.create(drop_index_t1) f3 = fiber.create(drop_space_t1) box.error.injection.set("ERRINJ_WAL_DELAY", false) fiber.sleep(0.1) -box.space.T1 -box.space.V1 +box.space.T1 ~= nil +box.space.V1 ~= nil -- -- In the same way, we have to drop all referenced spaces before