From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6548B440F3C for ; Wed, 13 Nov 2019 01:58:22 +0300 (MSK) References: <792e035b34839371b2fae03d91c782ebbe04c27c.1573566885.git.roman.habibov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 13 Nov 2019 00:04:30 +0100 MIME-Version: 1.0 In-Reply-To: <792e035b34839371b2fae03d91c782ebbe04c27c.1573566885.git.roman.habibov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/2] sql: make constraint operations transactional List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov , tarantool-patches@dev.tarantool.org Thanks for the patch! See 22 comments below. On 12/11/2019 15:02, Roman Khabibov wrote: > Put constraint names into the space's hash table and drop them on > commit, replace and drop (in the alter.cc triggers). > > Closes #3503 1. Please, write a documentation request describing what is changed in the public behaviour. > --- > src/box/alter.cc | 254 +++++++++++++++++++++++++++++-- > test/sql-tap/constraint.test.lua | 244 +++++++++++++++++++++++++++++ > 2 files changed, 488 insertions(+), 10 deletions(-) > create mode 100755 test/sql-tap/constraint.test.lua > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 272d7cd32..8255f2f83 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1690,6 +1690,38 @@ MoveCkConstraints::rollback(struct alter_space *alter) > space_swap_ck_constraint(alter->new_space, alter->old_space); > } > > +/** > + * Move constraint names hash table from old space to the new one. > + */ > +class MoveConstraints: public AlterSpaceOp > +{ > + void space_swap_constraint(struct space *old_space, > + struct space *new_space); > +public: > + MoveConstraints(struct alter_space *alter) : AlterSpaceOp(alter) {} > + virtual void alter(struct alter_space *alter); > + virtual void rollback(struct alter_space *alter); > +}; > + > +void > +MoveConstraints::space_swap_constraint(struct space *old_space, > + struct space *new_space) > +{ > + SWAP(new_space->constraint_names, old_space->constraint_names); > +} 2. I think this one-liner method can be inlined. > + > +void > +MoveConstraints::alter(struct alter_space *alter) > +{ > + space_swap_constraint(alter->old_space, alter->new_space); > +} > + > +void > +MoveConstraints::rollback(struct alter_space *alter) > +{ > + space_swap_constraint(alter->new_space, alter->old_space); > +} > + > /* }}} */ > @@ -2258,6 +2291,73 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid) > return false; > } > > +/** > + * Bring back the name of a unique index to the hash table of a > + * space. > + * > + * Rollback DROP index. > + * > + * @param trigger Trigger. > + * @param event Event. 3. These @param are totally useless here. I propose to drop them. In other similar places too. > + */ > +static int > +on_drop_index_rollback(struct trigger *trigger, void * /* event */) > +{ > + struct index_def *def = (struct index_def *)trigger->data; > + struct space *space = space_by_id(def->space_id); > + if (space_put_constraint_name(space, def->name) != 0) { > + panic("Can't recover after index drop rollback (out of " > + "memory)"); > + } > + index_def_delete(def); > + return 0; > +} > @@ -2393,17 +2493,46 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) > } > alter_space_move_indexes(alter, 0, iid); > (void) new DropIndex(alter, old_index); > + if (old_index->def->opts.is_unique && > + !space_is_system(old_space)) { > + space_drop_constraint_name(old_space, > + old_index->def->name); > + struct trigger *on_rollback = > + txn_alter_trigger_new(NULL, NULL); > + if (on_rollback == NULL) > + return -1; > + on_rollback->data = index_def_dup(old_index->def); 4. This index def leaks in case of commit. 5. You don't check for NULL. 6. Why do you need the whole def, why a name is not enough? 7. Why can't you use region memory? 8. Why can't you take a pointer at old_index->def->name instead of copying it? The same about new index. All the same questions about each other similar place. > + on_rollback->run = on_drop_index_rollback; > + txn_stmt_on_rollback(stmt, on_rollback); > + } > } > /* Case 2: create an index, if it is simply created. */ > if (old_index == NULL && new_tuple != NULL) { > + struct index_def *def = NULL; 9. Why do you nullify it, if you assign this variable on a next line? > + def = index_def_new_from_tuple(new_tuple, old_space); > + if (def == NULL) > + return -1; > + if (def->opts.is_unique && !space_is_system(old_space)) { > + if (space_has_constraint(old_space, def->name)) { > + diag_set(ClientError, ER_CONSTRAINT_EXISTS, > + def->name); > + return -1; > + } > + if (space_put_constraint_name(old_space, > + def->name) != 0) > + return -1; > + struct trigger *on_rollback = > + txn_alter_trigger_new(NULL, NULL); > + if (on_rollback == NULL) > + return -1; > + on_rollback->data = index_def_dup(def); > + on_rollback->run = on_create_index_rollback; > + txn_stmt_on_rollback(stmt, on_rollback); > + } > alter_space_move_indexes(alter, 0, iid); > CreateIndex *create_index = new CreateIndex(alter); > - create_index->new_index_def = > - index_def_new_from_tuple(new_tuple, old_space); > - if (create_index->new_index_def == NULL) > - return -1; > - index_def_update_optionality(create_index->new_index_def, > - alter->new_min_field_count); > + create_index->new_index_def = def; > + index_def_update_optionality(def, alter->new_min_field_count); > } > /* Case 3 and 4: check if we need to rebuild index data. */ > if (old_index != NULL && new_tuple != NULL) { > @@ -2411,6 +2540,82 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) > index_def = index_def_new_from_tuple(new_tuple, old_space); > if (index_def == NULL) > return -1; > + struct index_def *old_def = old_index->def; > + if (!space_is_system(old_space)) { > + /** > + * We put a new name either the an index 10. 'the an index'? - don't understand. Please, rephrase. > + * is becoming unique (i.e. constraint), > + * or when an unique index's name is under > + * change. > + */ > + if (!old_def->opts.is_unique && > + index_def->opts.is_unique) { > + if (space_has_constraint(old_space, > + index_def->name) != 0) { > + diag_set(ClientError, > + ER_CONSTRAINT_EXISTS, > + index_def->name); > + return -1; > + }> @@ -5121,6 +5343,9 @@ on_drop_ck_constraint_commit(struct trigger *trigger, void * /* event */) > { > struct ck_constraint *ck = (struct ck_constraint *)trigger->data; > assert(ck != NULL); > + struct space *space = space_by_id(ck->def->space_id); > + if (space != NULL) > + space_drop_constraint_name(space, ck->def->name); 11. How is it possible, that space == NULL? > ck_constraint_delete(ck); > return 0; > } > diff --git a/test/sql-tap/constraint.test.lua b/test/sql-tap/constraint.test.lua > new file mode 100755 > index 000000000..6444c06e9 > --- /dev/null > +++ b/test/sql-tap/constraint.test.lua > @@ -0,0 +1,244 @@ > +#!/usr/bin/env tarantool > +test = require("sqltester") > +test:plan(20) > + > +--!./tcltestrunner.lua > +-- 2001 September 15 12. I don't think you created this file in 2001. Please, don't blindly copy code, and read it before submission. This whole comment is garbage from SQLite. > +-- > +-- The author disclaims copyright to this source code. In place > +-- of a legal notice, here is a blessing: > +-- > +-- May you do good and not evil. > +-- May you find forgiveness for yourself and forgive others. > +-- May you share freely, never taking more than you give. > +-- > +------------------------------------------------------------------ > +-- This file implements regression tests for sql library. The > +-- focus of this file is testing the constraint creation, alter > +-- and drop. > +-- > + > +-- > +-- gh-3503 Check constraint name for duplicate. > +-- > +test:do_execsql_test( > + "constraint-1.1", > + [[ > + CREATE TABLE t1 (i INT PRIMARY KEY); > + DROP TABLE IF EXISTS t2; > + ]], { > + -- > + -- 13. We don't write these tags. They are artifacts from auto conversion from TCL for Lua. > + }) > + > +test:do_catchsql_test( > + "constraint-1.2", > + [[ > + CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0), > + CONSTRAINT c PRIMARY KEY (i)); > + ]], { > + -- > + 1,"Constraint C already exists" 14. I think you need to add more information. At least space name, and the existing constraint type. > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.3", > + [[ > + CREATE TABLE t2 (i INT, > + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), > + CONSTRAINT c PRIMARY KEY (i)); > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.4", > + [[ > + CREATE TABLE t2 (i INT PRIMARY KEY, > + CONSTRAINT c CHECK (i > 0), > + CONSTRAINT c UNIQUE (i)); > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.5", > + [[ > + CREATE TABLE t2 (i INT PRIMARY KEY, > + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), > + CONSTRAINT c UNIQUE (i)); > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.6", > + [[ > + CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY, > + CONSTRAINT c CHECK (i > 0)); 15. You have exactly the same test in 1.2. > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.7", > + [[ > + CREATE TABLE t2 (i INT PRIMARY KEY, > + CONSTRAINT c UNIQUE (i), > + CONSTRAINT c CHECK (i > 0)); 16. 1.4 is the same. > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.8", > + [[ > + CREATE TABLE t2 (i INT PRIMARY KEY, > + CONSTRAINT c CHECK (i > 0), > + CONSTRAINT c CHECK (i < 0)); > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.9", > + [[ > + CREATE TABLE t2 (i INT PRIMARY KEY, > + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), > + CONSTRAINT c CHECK (i > 0));> + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.10", > + [[ > + CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY, > + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i)); 17. 1.3 is the same. > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.11", > + [[ > + CREATE TABLE t2 (i INT PRIMARY KEY, > + CONSTRAINT c UNIQUE (i), > + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i)); 18. 1.5 is the same. > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.12", > + [[ > + CREATE TABLE t2 (i INT PRIMARY KEY, > + CONSTRAINT c CHECK (i > 0), > + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i)); 19. 1.9 is the same. > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-1.13", > + [[ > + CREATE TABLE t2 (i INT PRIMARY KEY CONSTRAINT c REFERENCES t1(i), > + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i)); > + ]], { > + -- > + 1,"Constraint C already exists" > + -- > + }) > + > +-- > +-- gh-3503 Make sure that _constraint works. 20. What is _constraint? > +-- > +test:do_execsql_test( > + "constraint-2.1", > + [[ > + DROP TABLE IF EXISTS t2; > + CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY); > + ]], { > + -- > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-2.2", > + [[ > + ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0); > + ]], { > + -- > + 1, "Constraint C already exists" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-2.3", > + [[ > + ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i); > + ]], { > + -- > + 1, "Index 'C' already exists in space 'T2'" > + -- > + }) > + > +test:do_catchsql_test( > + "constraint-2.4", > + [[ > + ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i); > + ]], { > + -- > + 1, "Constraint C already exists" > + -- > + }) > + > +test:do_execsql_test( > + "constraint-2.5", > + [[ > + ALTER TABLE t2 ADD CONSTRAINT f FOREIGN KEY(i) REFERENCES t1(i); > + ]], { > + -- > + -- > + }) > + > +test:do_execsql_test( > + "constraint-2.6", > + [[ > + ALTER TABLE t2 DROP CONSTRAINT f; > + ]], { > + -- > + -- > + }) > + > +test:do_execsql_test( > + "constraint-2.7", > + [[ > + ALTER TABLE t2 ADD CONSTRAINT f FOREIGN KEY(i) REFERENCES t1(i); > + ]], { > + -- > + -- > + }) > + > +test:finish_test() > \ No newline at end of file > 21. Please, append an empty line to the file. 22. You've used tap test file, but as far as I know they are obsolete for SQL. It is better to use normal tarantool tests (sql/*.test.lua) or SQL tests (sql/*.test.sql). But I may be wrong here.