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 E8F0522CAC for ; Wed, 24 Jul 2019 16:56:14 -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 CNmXAoClfs0N for ; Wed, 24 Jul 2019 16:56:14 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 87E8F22B4B for ; Wed, 24 Jul 2019 16:56:14 -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: <981da4b5-8c1e-9acb-5983-3ccfe9cee25f@tarantool.org> Date: Wed, 24 Jul 2019 22:58:07 +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 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') -- ----------------------------------------------------------------