From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (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 8BD6246970E for ; Sat, 28 Dec 2019 03:18:37 +0300 (MSK) Date: Sat, 28 Dec 2019 02:18:33 +0200 From: Nikita Pettik Message-ID: <20191228001833.GA45089@tarantool.org> References: <1b936f83be688cbbc8a1625be61a2841809b5633.1574965971.git.roman.habibov@tarantool.org> <77b2090c-4bfd-c42c-e744-3395eef2e8a3@tarantool.org> <721EA98D-2EC6-4C4C-97B0-6BCEBE889EDA@tarantool.org> <20007117-f560-e06d-16a5-67e811c8f9f8@tarantool.org> <88F2A14C-2D86-4C17-90F1-4B0388CF71C9@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <88F2A14C-2D86-4C17-90F1-4B0388CF71C9@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: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy On 17 Dec 18:03, Roman Khabibov wrote: > >> > >> @TarantoolBot document > >> Title: Table constraints in SQL > >> > >> SQL: > >> According to ANSI SQL, table constraint is one of the following > >> entities: PRIMARY KEY, UNIQUE, FOREIGN KEY, CHECK. Every > >> constraint have its own name passed by user or automatically have -> has > >> generated. And these names must be unique within one table/space. > >> Naming in SQL is case-insensitive, so "CONSTRAINT c" and False. Naming in SQL is case-sensitive. It's all about uppercasing unquoted identifiers: CREATE TABLE t2 (i INT, CONSTRAINT "c" CHECK (i > 0), CONSTRAINT c PRIMARY KEY (i)); --- - row_count: 1 ... > >> "CONSTRAINT C" are the same. For example, you will get error, if > >> you try to: > >> > >> CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0), > >> CONSTRAINT c PRIMARY KEY (i)); > >> > >> or > >> > >> CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY); > >> ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);'); > >> The same is for any other constraint types. > >> > > +/** > + * 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). Let's simply inline it, only 2 usages tho. > +{ > + struct constraint_id *id = space_find_constraint_id(space, name); > + if (id == NULL) > + return 0; > + diag_set(ClientError, ER_CONSTRAINT_EXISTS, > + constraint_type_strs[id->type], name, space_name(space)); > + return -1; > +} > + > + > +static inline void > +space_delete_constraint_id(struct space *space, const char *name) Why do we need this wrapper? > +{ > + 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? > new file mode 100755 > index 000000000..b41e16dc2 > --- /dev/null > +++ b/test/sql/constraint.test.lua > @@ -0,0 +1,93 @@ > +test_run = require('test_run').new() > +engine = test_run:get_cfg('engine') > +box.execute('pragma sql_default_engine=\''..engine..'\'') > + > +-- Make sure, that altering of an index name affects its record > +-- in the space's constraint hash table. > +-- > +box.execute('CREATE UNIQUE INDEX d ON t2(i);') > +box.space.T2.index.D:alter({name = 'E'}) > +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') As far as I see, you missed case of altering index (or other constraint) name to already occupied by another constraint. > +-- > +-- Make sure, that altering of an index uniqueness puts/drops > +-- its name to/from the space's constraint hash table. > +-- > +box.space.T2.index.E:alter({unique = false}) > +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') > +box.space.T2.index.E:alter({unique = true}) > +box.space.T2.ck_constraint.E:drop() > +box.space.T2.index.E:alter({unique = true}) > +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') > + > +-- Alter name and uniqueness of an unique index simultaneously. > +box.space.T2.index.E:alter({name = 'D', unique = false}) > +box.execute('CREATE UNIQUE INDEX e ON t2(i);') And make sure that CREATE UNIQUE INDEX d ON t2(i); fails What about circle renaming of consraint inside transaction which is rolled back later?