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

[Nikita Pettik <korablev@tarantool.org>]

On 29 Dec 18:51, Vladislav Shpilevoy wrote:
> >>>> 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.

I mean if user finds out that name is already occupied,
one is likely to use another name rather than drop/rename
existing constraint. In this case type of constraint is not
vital.

Anyway, patch LGTM. This is second one approve so it can be pushed,
but Sergos wrote that he is concerned about memory management
during constraint id creation/drop. Sergos, did you find any
issues, or we can push this patch?