From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 0990046971A for ; Sat, 7 Dec 2019 19:36:02 +0300 (MSK) References: <0218889f41f954a10bbe36c6d0c43f32fa7dd4b4.1575468493.git.roman.habibov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <58178e55-ec83-5a70-ffba-087730d636ed@tarantool.org> Date: Sat, 7 Dec 2019 17:35:59 +0100 MIME-Version: 1.0 In-Reply-To: <0218889f41f954a10bbe36c6d0c43f32fa7dd4b4.1575468493.git.roman.habibov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov , tarantool-patches@dev.tarantool.org Thanks for the fixes! See 4 comments below. > diff --git a/src/box/alter.cc b/src/box/alter.cc > index c12279e65..e60c2827b 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -2449,6 +2449,47 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid) > return false; > } > > +/** > + * Just return string with constraint type to print it in error > + * message. > + */ > +static inline const char * > +cosntraint_type_str(struct constraint_def *def) { > + switch (def->type) { > + case CONSTRAINT_TYPE_PK: > + return "PRIMARY KEY"; > + case CONSTRAINT_TYPE_UNIQUE: > + return "UNIQUE"; > + case CONSTRAINT_TYPE_FK: > + return "FOREIGN KEY"; > + case CONSTRAINT_TYPE_CK: > + return "CHECK"; > + } > +} 1. Please, use the way it is implemented for other enums - add const char *constraint_type_strs[] array and get a string via constraint_type_strs[type]. > + > +/** > + * Check if constraint with @a name exists within @a space. Call > + * diag_set() if it is so. > + * > + * @param space Space to find constraint within. > + * @param name Constraint name. > + * > + * @retval true Constraint exists. > + * @retval false Doesn't exist. > + */ > +static inline bool > +space_constraint_exists(struct space *space, const char *name) 2. Please, rename to 'check_existence()'. When you write 'exists', it looks like a simple getter method, which does not change anything. Your method does - it changes diagnostics area. And then it will return int: 0 or -1, as usual. > +{ > + struct constraint_def *def = > + space_constraint_def_by_name(space, name); > + if (def != NULL) { > + diag_set(ClientError, ER_CONSTRAINT_EXISTS, > + cosntraint_type_str(def), name, space_name(space)); > + return true; > + } > + return false; > +} > @@ -5348,13 +5387,8 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event) > fk->def = fk_def; > fk->index_id = fk_index->def->iid; > if (old_tuple == NULL) { > - if (space_constraint_def_by_name(child_space, > - fk_def->name) > - != NULL) { > - diag_set(ClientError, ER_CONSTRAINT_EXISTS, > - fk_def->name); > + if (space_constraint_exists(child_space, fk_def->name)) > return -1; > - } 3. I propose you to introduce space_constraint_exists in the previous commit so as here the diff will be smaller. > rlist_add_entry(&child_space->child_fk_constraint, > fk, in_child_space); > rlist_add_entry(&parent_space->parent_fk_constraint,> diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua > index e0bd60727..e4603e579 100755 > --- a/test/sql-tap/alter2.test.lua > +++ b/test/sql-tap/alter2.test.lua > @@ -246,7 +246,7 @@ test:do_catchsql_test( > ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES child; > ]], { > -- > - 1, "Constraint FK already exists" > + 1, "Duplicate key exists in unique index 'primary' in space '_fk_constraint'" 4. Well, maybe I was wrong, and we should keep vdbe_emit_halt_with_presence_test. It was added exactly to avoid such ugly error messages. But the problem is that ER_CONSTRAINT_EXISTS expects multiple arguments, which can't be passed through vdbe_emit_halt_with_presence_test. I propose you to turn ER_CONSTRAINT_EXISTS into ER_FK_EXISTS in vdbe_emit_fk_constraint_create, and into ER_CK_EXISTS in vdbe_emit_ck_constraint_create. ER_CK/FK_EXISTS will have only one parameter. > -- > }) >