* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-22 13:59 ` n.pettik
@ 2019-07-22 21:35 ` Vladislav Shpilevoy
2019-07-24 13:23 ` n.pettik
0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2019-07-22 21:35 UTC (permalink / raw)
To: n.pettik, tarantool-patches
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
^ permalink raw reply [flat|nested] 12+ messages in thread