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

[Nikita Pettik <korablev@tarantool.org>]

On 28 Dec 14:07, Vladislav Shpilevoy wrote:
> 
> I rewrote the commit message, check this:
> 
> ================================================================================
>     sql: make constraint names unique in scope of table
>     
>     Put constraint names into the space's hash table and drop them on
>     insert/delete in corresponding system spaces (_index,
>     _fk_constraint, _ck_constraint).
>     
>     Closes #3503
>     
>     @TarantoolBot document
>     Title: Constraint names are unique in scope of table
>     
>     SQL:
>     According to ANSI SQL, table constraint is one of the following
>     entities: PRIMARY KEY, UNIQUE, FOREIGN KEY, CHECK. (Also there
>     is NOT NULL, but we don't consider it a constraint.) Every
>     constraint has its own name passed by user or automatically
>     generated. And these names must be unique within one table/space.
>     
>     For example:
>     
>         tarantool> box.execute([[CREATE TABLE test (
>                                      a INTEGER PRIMARY KEY,
>                                      b INTEGER,
>                                      CONSTRAINT cnstr CHECK (a >= 0)
>                                  );]])
>         ---
>         - row_count: 1
>         ...
>     
>         tarantool> box.execute('CREATE UNIQUE INDEX cnstr ON test(b);')
>         ---
>         - null
>         - Constraint CHECK 'CNSTR' already exists in space 'TEST'
>         ...
>     
>     Unique index and CHECK are different constraint types, but they
>     share namespace, and can't have clashing names. The same for all
>     the other constraints.

It's OK, thanks.

> >> +/**
> >> + * 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)
> 
> Yes, maybe the name is bad. Check this:
> 
> ================================================================================
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 2e4fa3c41..1ce1c3858 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1776,11 +1776,11 @@ MoveCkConstraints::rollback(struct alter_space *alter)
>  }
>  
>  /**
> - * Check if constraint with @a name exists within @a space. Call
> - * diag_set() if it is so.
> + * Check if a constraint name is not occupied in @a space. Treat
> + * existence as an error.
>   */
>  static inline int
> -space_check_constraint_existence(struct space *space, const char *name)
> +space_check_constraint_name_is_free(struct space *space, const char *name)

Now I notice that space_check_constraint_... implies referring to
CHECK constraint (if fact we use _ck_constraint prefix, but anyway
it looks a bit messy). Personally I would better call it:

space_constraint_name_is_available(...)

> > Let's simply inline it, only 2 usages tho.
> 
> I would agree if not the long diag_set. Purpose of this function is not to
> write it more than once.

Ok, it is non-functional nit, so let's keep it if you want.
 
> >> +static inline void
> >> +space_delete_constraint_id(struct space *space, const char *name)
> > 
> > Why do we need this wrapper?
> 
> Multiple reasons: 1) it appears 5 times, so I decided to wrap it;
> 2) it is shorter, 3) it is consistent with other
> space_<action>_constraint_id() functions. But is not worth moving
> to space.h/.c IMO.

Nevermind, I missed space_pop_constraint_id() call.
 
> >> +{
> >> +	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?
> 
> 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?
 
> But I don't mind to change it back. Since I have no a
> proof that my point is correct, except personal taste.
> 
> 
> >> new file mode 100755
> >> index 000000000..b41e16dc2
> >> --- /dev/null
> >> +++ b/test/sql/constraint.test.lua
> >> @@ -0,0 +1,93 @@
> 
> >> +--
> >> +-- 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
> 
> I am not sure how that test is related to the patch,
> because uniqueness of index names is a different task,
> and it has many tests in other places. But ok.

Ok, sorry, here you absolutely right. It's very optional test.
Maybe with other constraint type it will be a bit more meaningful.
 
> ================================================================================
> 
> > What about circle renaming of consraint inside transaction which
> > is rolled back later?
> 
> I wasn't sure what do you mean as a circle renaming. I tried this:
> 
> ================================================================================
> diff --git a/test/sql/constraint.result b/test/sql/constraint.result
> index 65ec61e66..32c42897a 100644
> --- a/test/sql/constraint.result
> +++ b/test/sql/constraint.result
> @@ -148,6 +148,48 @@ test_run:cmd("setopt delimiter ''");
>   | - true
>   | ...
>  
> +--
> +-- Circle renaming in a transaction.
> +--
> +box.begin()                                                                 \
> +box.execute('CREATE UNIQUE INDEX d ON t2(i);')                              \
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);')  

I'd move constraints creation out of transaction, rename E
constraint to F, D to E, and after transaction is rollbacked
check that F is free and E and D are not.
  
> +-- D and E constraints exchange their names. Note, CHECK can't be           \
> +-- renamed, because CHECK name is a primary key of _ck_constraint.          \
> +-- It can be only dropped and created again with a new name.                \
> +box.space.T2.index.D:rename('F')                                            \
> +box.space.T2.ck_constraint.E:drop()                                         \
> +box.space.T2.index.F:rename('E')                                            \
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT D CHECK (i > 0);')               \
> +-- Now an attempt to occupy the names will report, that the names           \
> +-- are really swapped.                                                      \
> +_, err1 = box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK (i > 0);')     \
> +_, err2 = box.execute('CREATE UNIQUE INDEX d ON t2(i);')                    \
> +box.rollback()
> + | ---
> + | ...
> Also I did some minor refactoring to the test. See the whole patch below.

It's ok.