[Tarantool-patches] [PATCH v2 2/3] box: make constraint operations transactional
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Dec 29 18:51:32 MSK 2019
>>>> +/**
>>>> + * 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, <constraint type> - more
>> concrete, <name> - even more concrete.
>> 2) Because when I see '<type> constraint <name> already exists ...'
>> for me it looks like someone tried to create constraint of
>> type <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()
================================================================================
More information about the Tarantool-patches
mailing list