Re: [Tarantool-patches] [PATCH v2 2/3] box: make constraint operations transactional

[Vladislav Shpilevoy <v.shpilevoy@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, <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()
================================================================================