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

[Nikita Pettik <korablev@tarantool.org>]

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?