From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 663D346970E for ; Mon, 30 Dec 2019 14:12:20 +0300 (MSK) Date: Mon, 30 Dec 2019 14:12:19 +0300 From: Sergey Ostanevich Message-ID: <20191230111219.GE1222@tarantool.org> References: <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> <741cb846-6f91-1427-9e18-3d4be6855f5e@tarantool.org> <20191229222839.GB44588@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191229222839.GB44588@tarantool.org> 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, Vladislav Shpilevoy Hi! I didn't find any issues, let's push it. Regards, Sergos On 30 Dec 00:28, Nikita Pettik wrote: > 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, - 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. > > 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? >