From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 7204A46970E for ; Sun, 29 Dec 2019 18:51:35 +0300 (MSK) References: <1b936f83be688cbbc8a1625be61a2841809b5633.1574965971.git.roman.habibov@tarantool.org> <77b2090c-4bfd-c42c-e744-3395eef2e8a3@tarantool.org> <721EA98D-2EC6-4C4C-97B0-6BCEBE889EDA@tarantool.org> <20007117-f560-e06d-16a5-67e811c8f9f8@tarantool.org> <88F2A14C-2D86-4C17-90F1-4B0388CF71C9@tarantool.org> <20191228001833.GA45089@tarantool.org> <20191229000717.GB44517@tarantool.org> From: Vladislav Shpilevoy Message-ID: <741cb846-6f91-1427-9e18-3d4be6855f5e@tarantool.org> Date: Sun, 29 Dec 2019 18:51:32 +0300 MIME-Version: 1.0 In-Reply-To: <20191229000717.GB44517@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 2/3] box: make constraint operations transactional List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org >>>> +/** >>>> + * Check if constraint with @a name exists within @a space. Call >>>> + * diag_set() if it is so. >>>> + */ >>>> +static inline int >>>> +space_check_constraint_existence(struct space *space, const char *name) >>> >>> IMHO this function looks a bit strange (or I'd say - contradictory) >> >> Yes, maybe the name is bad. Check this: >> >> ================================================================================ >> diff --git a/src/box/alter.cc b/src/box/alter.cc >> index 2e4fa3c41..1ce1c3858 100644 >> --- a/src/box/alter.cc >> +++ b/src/box/alter.cc >> @@ -1776,11 +1776,11 @@ MoveCkConstraints::rollback(struct alter_space *alter) >> } >> >> /** >> - * Check if constraint with @a name exists within @a space. Call >> - * diag_set() if it is so. >> + * Check if a constraint name is not occupied in @a space. Treat >> + * existence as an error. >> */ >> static inline int >> -space_check_constraint_existence(struct space *space, const char *name) >> +space_check_constraint_name_is_free(struct space *space, const char *name) > > Now I notice that space_check_constraint_... implies referring to > CHECK constraint (if fact we use _ck_constraint prefix, but anyway > it looks a bit messy). Personally I would better call it: > > space_constraint_name_is_available(...) Agree. But we usually use 'is' without other verb, when it is a simple getter method. Without any side effects such as diag_set. I added 'ensure' verb: space_ensure_constraint_name_is_available() >>>> +{ >>>> + constraint_id_delete(space_pop_constraint_id(space, name)); >>>> +} >>>> + >>>> diff --git a/src/box/errcode.h b/src/box/errcode.h >>>> index c660b1c70..094a63ee1 100644 >>>> --- a/src/box/errcode.h >>>> +++ b/src/box/errcode.h >>>> @@ -222,7 +222,7 @@ struct errcode_record { >>>> /*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \ >>>> /*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \ >>>> /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \ >>>> - /*170 */_(ER_CONSTRAINT_EXISTS, "Constraint %s already exists") \ >>>> + /*170 */_(ER_CONSTRAINT_EXISTS, "Constraint %s '%s' already exists in space '%s'") \ >>> >>> I'd better place type of constraint at the beggining of sentence: >>> >>> Check constraint xxx already exists >>> Foreign key constraint ... >>> Unique constraint ... >>> >>> Sounds way much better, doesn't it? >> >> It is subjective. I changed it to this order on purpose. >> 1) Because I wanted to go from more common term to a more >> concrete: 'Constraint' - common, - more >> concrete, - even more concrete. >> 2) Because when I see ' constraint already exists ...' >> for me it looks like someone tried to create constraint of >> type . But it is not always so. Type is not the most >> important part of the message. > > Well, I think it may turn out to be good argument. Why do > you add type of constraint to message at all? My guess > is to simplify user's life and make finding constraint > with given name easy. If so, does it make sense? Your guess is right. I wanted to make it simpler to determine with what a constraint there is a conflict. So as a user could do rename of index/fk/ck/whatever-else which occupies the needed name. >> ================================================================================ >> >>> What about circle renaming of consraint inside transaction which >>> is rolled back later? >> >> I wasn't sure what do you mean as a circle renaming. I tried this: >> >> ================================================================================ >> diff --git a/test/sql/constraint.result b/test/sql/constraint.result >> index 65ec61e66..32c42897a 100644 >> --- a/test/sql/constraint.result >> +++ b/test/sql/constraint.result >> @@ -148,6 +148,48 @@ test_run:cmd("setopt delimiter ''"); >> | - true >> | ... >> >> +-- >> +-- Circle renaming in a transaction. >> +-- >> +box.begin() \ >> +box.execute('CREATE UNIQUE INDEX d ON t2(i);') \ >> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') > > I'd move constraints creation out of transaction, rename E > constraint to F, D to E, and after transaction is rollbacked > check that F is free and E and D are not. Ok, done. ================================================================================ diff --git a/test/sql/constraint.result b/test/sql/constraint.result index 8ca63fd7a..3000a9244 100644 --- a/test/sql/constraint.result +++ b/test/sql/constraint.result @@ -143,34 +143,61 @@ box.commit() -- -- Circle renaming in a transaction. -- +box.execute('CREATE UNIQUE INDEX d ON t2(i);') + | --- + | - row_count: 1 + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') + | --- + | - row_count: 1 + | ... box.begin() \ -box.execute('CREATE UNIQUE INDEX d ON t2(i);') \ -box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') \ --- D and E constraints exchange their names. Note, CHECK can't be \ --- renamed, because CHECK name is a primary key of _ck_constraint. \ +-- Rename CHECK E to F. Note, CHECK can't be properly renamed, \ +-- because CHECK name is a primary key of _ck_constraint. \ -- It can be only dropped and created again with a new name. \ -box.space.T2.index.D:rename('F') \ box.space.T2.ck_constraint.E:drop() \ -box.space.T2.index.F:rename('E') \ -box.execute('ALTER TABLE t2 ADD CONSTRAINT D CHECK (i > 0);') \ --- Now an attempt to occupy the names will report, that the names \ --- are really swapped. \ +box.execute('ALTER TABLE t2 ADD CONSTRAINT f CHECK (i > 0);') \ +-- Rename UNIQUE D to E. \ +box.space.T2.index.D:rename('E') \ +-- Now E and F are occupied, and D is free. Check this. \ _, err1 = box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') \ -_, err2 = box.execute('CREATE UNIQUE INDEX d ON t2(i);') \ +_, err2 = box.execute('CREATE UNIQUE INDEX f ON t2(i);') \ +_, err3 = box.execute('CREATE UNIQUE INDEX d ON t2(i);') \ box.rollback() | --- | ... +-- Fail. E and F were occupied in the end of the transaction. The +-- error messages say that E was an index, not a CHECK, like it +-- was before the transaction. err1, err2 | --- | - UNIQUE constraint 'E' already exists in space 'T2' - | - CHECK constraint 'D' already exists in space 'T2' + | - CHECK constraint 'F' already exists in space 'T2' | ... --- After rollback ensure, that the names are free again. -box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') +-- Success, D was free in the end. +err3 + | --- + | - null + | ... +-- After rollback ensure, that the names E and D are occupied both +-- again. +box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK (i > 0);') + | --- + | - null + | - UNIQUE constraint 'D' already exists in space 'T2' + | ... +box.execute('CREATE UNIQUE INDEX e ON t2(i);') + | --- + | - null + | - CHECK constraint 'E' already exists in space 'T2' + | ... +-- F is free after rollback. +box.execute('CREATE UNIQUE INDEX f ON t2(i);') | --- | - row_count: 1 | ... -box.execute('CREATE UNIQUE INDEX d ON t2(i);') +-- Cleanup after the test case. +box.execute('DROP INDEX f ON t2;') | --- | - row_count: 1 | ... diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua index 8439bebc1..2f367eafe 100755 --- a/test/sql/constraint.test.lua +++ b/test/sql/constraint.test.lua @@ -64,25 +64,35 @@ box.commit() -- -- Circle renaming in a transaction. -- +box.execute('CREATE UNIQUE INDEX d ON t2(i);') +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') box.begin() \ -box.execute('CREATE UNIQUE INDEX d ON t2(i);') \ -box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') \ --- D and E constraints exchange their names. Note, CHECK can't be \ --- renamed, because CHECK name is a primary key of _ck_constraint. \ +-- Rename CHECK E to F. Note, CHECK can't be properly renamed, \ +-- because CHECK name is a primary key of _ck_constraint. \ -- It can be only dropped and created again with a new name. \ -box.space.T2.index.D:rename('F') \ box.space.T2.ck_constraint.E:drop() \ -box.space.T2.index.F:rename('E') \ -box.execute('ALTER TABLE t2 ADD CONSTRAINT D CHECK (i > 0);') \ --- Now an attempt to occupy the names will report, that the names \ --- are really swapped. \ +box.execute('ALTER TABLE t2 ADD CONSTRAINT f CHECK (i > 0);') \ +-- Rename UNIQUE D to E. \ +box.space.T2.index.D:rename('E') \ +-- Now E and F are occupied, and D is free. Check this. \ _, err1 = box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') \ -_, err2 = box.execute('CREATE UNIQUE INDEX d ON t2(i);') \ +_, err2 = box.execute('CREATE UNIQUE INDEX f ON t2(i);') \ +_, err3 = box.execute('CREATE UNIQUE INDEX d ON t2(i);') \ box.rollback() +-- Fail. E and F were occupied in the end of the transaction. The +-- error messages say that E was an index, not a CHECK, like it +-- was before the transaction. err1, err2 --- After rollback ensure, that the names are free again. -box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);') -box.execute('CREATE UNIQUE INDEX d ON t2(i);') +-- Success, D was free in the end. +err3 +-- After rollback ensure, that the names E and D are occupied both +-- again. +box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK (i > 0);') +box.execute('CREATE UNIQUE INDEX e ON t2(i);') +-- F is free after rollback. +box.execute('CREATE UNIQUE INDEX f ON t2(i);') +-- Cleanup after the test case. +box.execute('DROP INDEX f ON t2;') box.execute('DROP INDEX d ON t2;') box.space.T2.ck_constraint.E:drop() ================================================================================