[tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the comments!

On 24/07/2019 15:23, n.pettik wrote:
> 
> Thanks, the only dubious nit I can spot: should we
> set initiateTTrans for DDL operations which consist
> of one box operations?

Yeah, I thought about it, and even made it and reverted back
several times having some doubts 'code consistency vs
transaction necessity'.

After all I decided, that each DDL op should be transactional,
even if consists of one statement. Because 1) it is consistent
with other SQL DDL, 2) it protects us from the case when SQL
DDL will complicate. I think, the latter matters the most, taking
into account incoming information schema, when even a simple
index creation will insert into _index, into something like
_constraint_name to ensure name uniqueness, etc.

> Like: create index, drop index,
> create trigger, drop trigger, drop constraint etc
> For instance, we don’t set it for TRUNCATE stmt.

Good catch, now I will.

----------------------------------------------------------------
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 06eab7f2d..c5e41cbf3 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -779,6 +779,7 @@ cmd ::= with(C) DELETE FROM fullname(X) indexed_opt(I) where_opt(W). {
 /////////////////////////// The TRUNCATE statement /////////////////////////////
 //
 cmd ::= TRUNCATE TABLE fullname(X). {
+  pParse->initiateTTrans = true;
   sql_table_truncate(pParse, X);
 }
----------------------------------------------------------------

> The only two operations which require starting transactions
> are CREATE TABLE and RENAME.

Do not forget about DROP TABLE.

> Btw, when checking
> results of RENAME I’d also check that operation (update of
> ‘CREATE TRIGGER …' string) performed on _trigger has
> been committed/rollbacked.
> 

Agree.

----------------------------------------------------------------
diff --git a/test/sql/ddl.result b/test/sql/ddl.result
index 834d9757f..a47e15bb4 100644
--- a/test/sql/ddl.result
+++ b/test/sql/ddl.result
@@ -187,7 +187,58 @@ box.space.T1:count() == 0 and box.space.T2:count() == 0
  | - true
  | ...
 
-box.execute('DROP TABLE t1')
+--
+-- Rename transactionally changes name of the table and its
+-- mentioning in trigger bodies.
+--
+_trigger_index = box.space._trigger.index.space_id
+ | ---
+ | ...
+test_run:cmd("setopt delimiter '$'")
+ | ---
+ | - true
+ | ...
+box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
+              BEGIN
+                  INSERT INTO t2 VALUES(1);
+              END; ]])$
+ | ---
+ | - row_count: 1
+ | ...
+
+box.begin()
+box.execute('ALTER TABLE t1 RENAME TO t1_new;')
+sql = _trigger_index:select(box.space.T1_NEW.id)[1].opts.sql
+assert(sql:find('T1_NEW'))
+box.rollback()$
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''")$
+ | ---
+ | - true
+ | ...
+
+sql = _trigger_index:select(box.space.T1.id)[1].opts.sql
+ | ---
+ | ...
+not sql:find('T1_NEW') and sql:find('t1') ~= nil
+ | ---
+ | - true
+ | ...
+
+box.execute('ALTER TABLE t1 RENAME TO t1_new;')
+ | ---
+ | - row_count: 0
+ | ...
+sql = _trigger_index:select(box.space.T1_NEW.id)[1].opts.sql
+ | ---
+ | ...
+sql:find('T1_NEW') ~= nil
+ | ---
+ | - true
+ | ...
+
+box.execute('DROP TABLE t1_new')
  | ---
  | - row_count: 1
  | ...
diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua
index 300f8b20c..371f0339e 100644
--- a/test/sql/ddl.test.lua
+++ b/test/sql/ddl.test.lua
@@ -85,7 +85,32 @@ box.begin() truncate_both() box.commit()
 
 box.space.T1:count() == 0 and box.space.T2:count() == 0
 
-box.execute('DROP TABLE t1')
+--
+-- Rename transactionally changes name of the table and its
+-- mentioning in trigger bodies.
+--
+_trigger_index = box.space._trigger.index.space_id
+test_run:cmd("setopt delimiter '$'")
+box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
+              BEGIN
+                  INSERT INTO t2 VALUES(1);
+              END; ]])$
+
+box.begin()
+box.execute('ALTER TABLE t1 RENAME TO t1_new;')
+sql = _trigger_index:select(box.space.T1_NEW.id)[1].opts.sql
+assert(sql:find('T1_NEW'))
+box.rollback()$
+test_run:cmd("setopt delimiter ''")$
+
+sql = _trigger_index:select(box.space.T1.id)[1].opts.sql
+not sql:find('T1_NEW') and sql:find('t1') ~= nil
+
+box.execute('ALTER TABLE t1 RENAME TO t1_new;')
+sql = _trigger_index:select(box.space.T1_NEW.id)[1].opts.sql
+sql:find('T1_NEW') ~= nil
+
+box.execute('DROP TABLE t1_new')
 box.execute('DROP TABLE t2')
 
 --
----------------------------------------------------------------