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

[Vladislav Shpilevoy]

Thanks for the fixes!

See 9 comments below.

>> 14. Talking of the super mess with on_replace_dd_index, and
>> how tight it is now with all these struggling about replacing
>> constraints, and on_commit/on_rollback. I think, that this is
>> a dead end. Too complex.
>>
>> How about introducing a couple of new AlterSpaceOp classes?
>> AddConstraint, and DropConstraint. AlterSpaceOp looks good
>> because
>> - it will allow you to not set on_commit/on_rollback triggers
>> manually - it already provides these methods;
>> - you will be able to move this huge pieces of code out of
>> on_replace_dd_index and other on_replace_*;
>> - it is consistent with MoveIndex/DropIndex/etc classes.
>>
>> All you will need to do is to add to on_replace_*
>> (void) new AddConstraint(), and (void) new DropConstraint.
>> And you will keep (void) new MoveConstraints().
>>
>> Also that should eliminate code duplication between different
>> on_replace_* functions about adding/dropping constraints.
> +/**
> + * CreateConstraintDef - add a new constraint def to the space.
> + */
> +class CreateConstraintDef: public AlterSpaceOp
> +{
> +private:
> +	struct constraint_def *new_constraint_def;
> +public:
> +	CreateConstraintDef(struct alter_space *alter,
> +			    struct constraint_def *def)
> +		:AlterSpaceOp(alter), new_constraint_def(def)
> +	{}
> +	virtual void alter(struct alter_space *alter);
> +	virtual void rollback(struct alter_space *alter);
> +};
> +
> +void
> +CreateConstraintDef::alter(struct alter_space *alter)
> +{
> +	assert(new_constraint_def != NULL);
> +	if (space_put_constraint(alter->old_space, new_constraint_def) != 0)
> +		panic("Can't recover after constraint alter (out of memory)");

1. Wow, why panic? Alter can safely fail. It it is not a commit or
rollback trigger. Maybe it can't return an error, but it can throw
it.

(Although I would like to be able to panic on malloc fails, but it
does not seem to happen in the nearest future.)

> +}
> +
> +void
> +CreateConstraintDef::rollback(struct alter_space *alter)
> +{
> +	assert(space_pop_constraint(alter->new_space,
> +				    new_constraint_def->name) ==
> +	       new_constraint_def);

2. This will fail in Release build mode, because
assert arguments are not calculated in Release. So you
delete the object, but it is still in the hash table.

> +	(void) alter;
> +	constraint_def_delete(new_constraint_def);
> +}
> +
> 
> I know, that I can combine these ifs to the one, but I did so, because I believe that
> it is more readable.

3. Yes, here I agree.

> 
> +		struct index_def *old_def = old_index->def;
> +		/*
> +		 * We put a new name either an index is becoming
> +		 * unique (i.e. constraint), or when an unique
> +		 * index's name is under change.
> +		 */
> +		if (!old_def->opts.is_unique && index_def->opts.is_unique &&
> +		    create_index_as_constraint(alter, index_def) != 0)
> +			return -1;
> +		if (old_def->opts.is_unique && index_def->opts.is_unique &&
> +		    strcmp(index_def->name, old_def->name) != 0 &&
> +		    (create_index_as_constraint(alter, index_def) != 0 ||
> +		     drop_index_as_constraint(alter, old_def) != 0))
> +			return -1;
> +		if (old_def->opts.is_unique && !index_def->opts.is_unique &&
> +		    drop_index_as_constraint(alter, old_def) != 0)
> +			return -1;
> 
> 
> commit c54d2e89d660d2f0b07f1e6917327131362a6568
> Author: Roman Khabibov <roman.habibov at tarantool.org>
> Date:   Wed Oct 23 15:54:16 2019 +0300
> 
>     sql: make constraint operations transactional
>     
>     Put constraint names into the space's hash table and drop them on
>     replace in corresponding system spaces (_index, _fk_constraint,
>     _ck_constraint).
>     
>     Closes #3503
>     
>     @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
>     generated. And these names must be unique within one table/space.
>     Naming in SQL is case-insensitive, so "CONSTRAINT c" and
>     "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.
>     
>     Lua/box:
>     
>     You also can create/drop table constraints from box. See
>     space_object:create_check_constraint() and space_object:create_index()
>     (if an index is unique). Naming in box is case-sensitive, so 'c' and
>     'C' are not the same (see naming policy). For example, an unique
>     index is a constraint, but a non-unique index is not. So, a non-unique
>     index can have the same name with a check/foreign key constraint
>     within one space:
>     
>     box.execute('CREATE TABLE t2 (i INT PRIMARY KEY);');
>     box.execute('CREATE INDEX e ON t2(i);');
>     box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);');
>     
>     But, if you try to:
>     
>     box.space.T2.index.E:alter({unique = true});
>     
>     You will get error, beacuse index 'e' is becoming unique.

4. That is a good comment. (But typo: beacuse -> because.)

> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index bef25b605..c03bad589 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc> @@ -2510,18 +2649,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>  	if (old_index == NULL && new_tuple != NULL) {
>  		if (alter_space_move_indexes(alter, 0, iid))
>  			return -1;
> +		struct index_def *def =
> +			index_def_new_from_tuple(new_tuple, old_space);
> +		if (def == NULL)
> +			return -1;
> +		if (def->opts.is_unique &&
> +		    create_index_as_constraint(alter, def) != 0) {
> +			index_def_delete(def);
> +			return -1;
> +		}
>  		CreateIndex *create_index;
>  		try {
>  			create_index = new CreateIndex(alter);
>  		} catch (Exception *e) {
>  			return -1;

5. 'def' leaks here.

>  		}
> -		create_index->new_index_def =
> -			index_def_new_from_tuple(new_tuple, old_space);
> -		if (create_index->new_index_def == NULL)
> -			return -1;
> -		index_def_update_optionality(create_index->new_index_def,
> -					     alter->new_min_field_count);
> +		create_index->new_index_def = def;
> +		index_def_update_optionality(def, alter->new_min_field_count);
>  	}
>  	/* Case 3 and 4: check if we need to rebuild index data. */
>  	if (old_index != NULL && new_tuple != NULL) {
> @@ -2531,6 +2675,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>  			return -1;
>  		auto index_def_guard =
>  			make_scoped_guard([=] { index_def_delete(index_def); });
> +		struct index_def *old_def = old_index->def;
> +		/*
> +		 * We put a new name either an index is becoming

6. 'either' -> 'when either'.

> +		 * unique (i.e. constraint), or when an unique
> +		 * index's name is under change.
> +		 */
> +		if (!old_def->opts.is_unique && index_def->opts.is_unique &&
> +		    create_index_as_constraint(alter, index_def) != 0)
> +			return -1;
> +		if (old_def->opts.is_unique && index_def->opts.is_unique &&
> +		    strcmp(index_def->name, old_def->name) != 0 &&
> +		    (create_index_as_constraint(alter, index_def) != 0 ||
> +		     drop_index_as_constraint(alter, old_def) != 0))
> +			return -1;
> +		if (old_def->opts.is_unique && !index_def->opts.is_unique &&
> +		    drop_index_as_constraint(alter, old_def) != 0)
> +			return -1;
>  		/*
>  		 * To detect which key parts are optional,
>  		 * min_field_count is required. But> @@ -5219,6 +5407,16 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
>  						      fk);
>  			if (on_rollback == NULL)
>  				return -1;
> +			struct constraint_def *constr_def =
> +				constraint_def_new(child_space->def->id,
> +						   CONSTRAINT_TYPE_FK,
> +						   fk_def->name);
> +			if (constr_def == NULL)
> +				return -1;
> +			if (space_put_constraint(child_space, constr_def) != 0) {
> +				constraint_def_delete(constr_def);
> +				return -1;
> +			}

7. Sequence constraint_def_new -> space_put_constraint is repeated 4
times. I think this is worth wrapping into a function. The same about
space_constraint_def_by_name -> space_pop_constraint ->
constraint_def_delete.

>  			txn_stmt_on_rollback(stmt, on_rollback);
>  			fk_constraint_set_mask(fk,
>  					       &parent_space->fk_constraint_mask,
> diff --git a/test/sql/constraint.result b/test/sql/constraint.result
> new file mode 100644
> index 000000000..ba262182b
> --- /dev/null
> +++ b/test/sql/constraint.result
> @@ -0,0 +1,190 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +engine = test_run:get_cfg('engine')
> + | ---
> + | ...
> +box.execute('pragma sql_default_engine=\''..engine..'\'')
> + | ---
> + | - row_count: 0
> + | ...
> +test_run:cmd("setopt delimiter ';'")

8. Please, don't. There is only one small place where you need
multiple lines. You can either use ';' just for it, or use /.

> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
> new file mode 100755
> index 000000000..50162e47d
> --- /dev/null
> +++ b/test/sql/constraint.test.lua

9. Your test leaves lots of not deleted data. Please, do
cleanup.