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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

The commit below (or at least the commit message) was outdated.
I see a different version on the branch. Below I copypaste the
branch's version, and review it.

See 14 comments below.
    
>     Put constraint names into the space's hash table and drop them on
>     replace in correspondig system spaces (_index, _fk_constraint,

1. correspondig -> corresponding

>     _ck_constraint).
>     
>     Closes #3503
>     
>     @TarantoolBot document
>     Title: Constraints
>     Now, names of constraints must be unique within one space.

2. Now, imagine that you are a technical writer. You don't
run and test Tarantool every day. And you don't know what,
how, when, and where do we support. What are the problems,
features, plans. Your task is simple - you take a ticket
from tarantool/doc, and transform it into something that is
ok to be placed on the site. Into a good English text, with
rich details, with omission of not needed information. You
may fix some of the old documentation affected by that ticket.
You may add pictures, diagrams.

And then imagine you got such a ticket as you provided here.
Sorry, but this is just nothing. That is like if I would ask
you to 'fix an assertion in box'. What assertion? Where in
the box? What is a reproducer?

You need to explain, what do you mean as constraints; what
if uniqueness is violated; does letter case matter; what are
examples; does language matter (Lua, SQL, binary) etc.

> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 4a3241a79..4bb8b8556 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1765,6 +1766,38 @@ MoveCkConstraints::rollback(struct alter_space *alter)
>  	space_swap_ck_constraint(alter->new_space, alter->old_space);
>  }
>  
> +/**
> + * Move constraint names hash table from old space to a new one.
> + */
> +class MoveConstraints: public AlterSpaceOp
> +{
> +	inline void space_swap_constraint(struct space *old_space,
> +				   struct space *new_space);
> +public:
> +	MoveConstraints(struct alter_space *alter) : AlterSpaceOp(alter) {}
> +	virtual void alter(struct alter_space *alter);
> +	virtual void rollback(struct alter_space *alter);
> +};
> +
> +inline void
> +MoveConstraints::space_swap_constraint(struct space *old_space,
> +				       struct space *new_space)
> +{
> +	SWAP(new_space->constraint_names, old_space->constraint_names);

3. So this is not inlined as a I asked. Perhaps that was
misunderstanding. I meant literally inline it. Not add
'inline' keyword. That is a one-liner function, which calls
another one liner. Even with the same number of arguments.
What is a point of having it? It will never even be extended,
and does not make the code more compact.

> +}
> +
> +void
> +MoveConstraints::alter(struct alter_space *alter)
> +{
> +	space_swap_constraint(alter->old_space, alter->new_space);
> +}
> +
> +void
> +MoveConstraints::rollback(struct alter_space *alter)
> +{
> +	space_swap_constraint(alter->new_space, alter->old_space);
> +}
> @@ -2341,6 +2375,150 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
> +
> +/**
> + * Put the node of an unique index to the constraint hash table of
> + * @a space and create trigger on rollback.
> + *
> + * This function is needed to wrap the duplicated piece of code
> + * inside on_replace_dd_index().
> + *
> + * @param stmt  Statement.
> + * @param space Space.
> + * @param def   Index def.
> + *
> + * @retval 0  Success.
> + * @retval -1 Constraint already exists or out of memory.

4. Once again - please, don't write comments just for
comments. 'stmt Statement.' really does not make anything
clearer or easier to understand. Either drop these
useless formal @param @retval, or provide more *useful*
details about these arguments. Although I doubt, that
it is needed. The function is trivial and is not public
anyway. The same about other similar places.

The comment's part before first @param is good, keep it.

> + */
> +static int
> +create_index_as_constraint(struct txn_stmt *stmt, struct space *space,
> +			   struct index_def *def) {

5. I propose you to make it `const struct index_def *`.

> +	assert(def->opts.is_unique);
> +	if (space_constraint_def_by_name(space, def->name) != NULL) {
> +		diag_set(ClientError, ER_CONSTRAINT_EXISTS, def->name);

6. AFAIR, I asked to add constraint type into the error message, didn't I?
Why didn't you add it?

> +		return -1;
> +	}
> +	struct constraint_def *constr_def =
> +		constraint_def_new(space->def->id, def->iid == 0 ?
> +				   CONSTRAINT_TYPE_PK : CONSTRAINT_TYPE_UNIQUE,
> +				   def->name);
> +	if (constr_def == NULL)
> +		return -1;
> +	struct trigger *on_rollback = txn_alter_trigger_new(NULL, NULL);
> +	if (space_put_constraint(space, constr_def) != 0 ||
> +	    on_rollback == NULL) {
> +		constraint_def_free(constr_def);
> +		return -1;
> +	}
> +	on_rollback->data = constr_def;
> +	on_rollback->run = on_create_index_rollback;
> +	txn_stmt_on_rollback(stmt, on_rollback);
> +	return 0;
> +}
> +
> +/**
> + * Drop the node of an unique index from the constraint hash table
> + * of @a space and create trigger on rollback.
> + *
> + * This function is needed to wrap the duplicated piece of code
> + * inside on_replace_dd_index().
> + *
> + * @param stmt  Statement.
> + * @param space Space.
> + * @param def   Index def.
> + *
> + * @retval 0  Success.
> + * @retval -1 Out of memory.
> + */
> +static int
> +drop_index_as_constraint(struct txn_stmt *stmt, struct space *space,
> +			 struct index_def *def) {
> +	assert(def->opts.is_unique);
> +	struct constraint_def *constr_def =
> +		space_constraint_def_by_name(space, def->name);
> +	assert(constr_def != NULL);
> +	space_drop_constraint(space, def->name);
> +	struct trigger *on_rollback = txn_alter_trigger_new(NULL, NULL);
> +	if (on_rollback == NULL)
> +		return -1;

7. In case on_rollback creation is failed, you don't restore
the constraint back.

> +	on_rollback->data = constr_def;
> +	on_rollback->run = on_drop_index_rollback;
> +	txn_stmt_on_rollback(stmt, on_rollback);
> +	return 0;
> +}
> +
>  /**
>   * Just like with _space, 3 major cases:
>   *> @@ -2469,35 +2645,111 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>  		 * Can't drop index if foreign key constraints
>  		 * references this index.
>  		 */
> -		if (index_is_used_by_fk_constraint(&old_space->parent_fk_constraint,
> +		if (index_is_used_by_fk_constraint(&space->parent_fk_constraint,
>  						   iid)) {
> -			diag_set(ClientError, ER_ALTER_SPACE,
> -				  space_name(old_space),
> +			diag_set(ClientError, ER_ALTER_SPACE, space_name(space),
>  				  "can not drop a referenced index");
>  			return -1;
>  		}
>  		alter_space_move_indexes(alter, 0, iid);
>  		(void) new DropIndex(alter, old_index);
> +		if (old_index->def->opts.is_unique &&
> +		    drop_index_as_constraint(stmt, space, old_index->def) != 0)
> +			return -1;
>  	}
>  	/* Case 2: create an index, if it is simply created. */
>  	if (old_index == NULL && new_tuple != NULL) {
> +		struct index_def *def =
> +			index_def_new_from_tuple(new_tuple, space);
> +		if (def == NULL)
> +			return -1;
> +		if (def->opts.is_unique) {
> +			if (create_index_as_constraint(stmt, space, def) != 0) {
> +				index_def_delete(def);
> +				return -1;
> +			}
> +		}
>  		alter_space_move_indexes(alter, 0, iid);
>  		CreateIndex *create_index = new CreateIndex(alter);
> -		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) {
>  		struct index_def *index_def;
> -		index_def = index_def_new_from_tuple(new_tuple, old_space);
> +		index_def = index_def_new_from_tuple(new_tuple, space);
>  		if (index_def == NULL)
>  			return -1;
>  		auto index_def_guard =
>  			make_scoped_guard([=] { index_def_delete(index_def); });
> +		struct index_def *old_def = old_index->def;
> +		/**

8. We don't use /** inside functions.

> +		 * 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) {
> +			if (create_index_as_constraint(stmt, space, index_def)

9. When you have multiple conditions, it is shorter
and easier to check them all via && instead of 'if if if ... '.

> +			    != 0)
> +				return -1;
> +		}
> +		if (old_def->opts.is_unique && index_def->opts.is_unique &&
> +		    strcmp(index_def->name, old_def->name) != 0) {
> +			if (space_constraint_def_by_name(space, index_def->name)
> +			    != NULL) {
> +				diag_set(ClientError, ER_CONSTRAINT_EXISTS,
> +					 index_def->name);
> +				return -1;
> +			}
> +			struct constraint_def *old_constr_def =
> +				space_constraint_def_by_name(space,
> +							     old_def->name);
> +			assert(old_constr_def != NULL);
> +			struct constraint_def *new_constr_def =
> +				constraint_def_new(old_constr_def->space_id,
> +						   old_constr_def->type,
> +						   index_def->name);
> +			if (new_constr_def == NULL)
> +				return -1;
> +			auto new_constraint_def_guard =
> +				make_scoped_guard([=] { constraint_def_free(new_constr_def); });
> +			if (space_put_constraint(space, new_constr_def) != 0)
> +				return -1;
> +			space_drop_constraint(space, old_def->name);
> +			/**
> +			 * Array with the pair of 2
> +			 * index_def structures: old and
> +			 * new.
> +			 */
> +			uint32_t size = sizeof(struct constraint_def *) * 2;
> +			struct region *r = &fiber()->gc;
> +			struct constraint_def **defs =
> +				(struct constraint_def **)region_alloc(r, size);
> +			if (defs == NULL) {
> +				diag_set(OutOfMemory, size, "region",
> +					 "new slab");

10. Please, take a look at what OutOfMemory expects, and how is
it used in other places. It takes size, allocator function name,
and result variable name. So both 'region' and 'new slab' are
wrong here. You should use 'region_alloc' and 'defs'.

11. old_constr_def leaks here. Moreover, it is dropped already,
and is it not added back.

> +				return -1;
> +			}
> +			defs[0] = old_constr_def;
> +			defs[1] = new_constr_def;
> +			struct trigger *on_commit =
> +				txn_alter_trigger_new(NULL, NULL);
> +			struct trigger *on_rollback =
> +				txn_alter_trigger_new(NULL, NULL);
> +			if (on_commit == NULL || on_rollback == NULL)
> +				return -1;

12. The same here.

> +			on_commit->data = defs;
> +			on_commit->run = on_replace_index_commit;
> +			on_rollback->data = defs;
> +			on_rollback->run = on_replace_index_rollback;
> +			txn_stmt_on_commit(stmt, on_commit);
> +			txn_stmt_on_rollback(stmt, on_rollback);
> +			new_constraint_def_guard.is_active = false;
> +		}
> +		if (old_def->opts.is_unique && !index_def->opts.is_unique) {
> +			if (drop_index_as_constraint(stmt, space, old_def) != 0)
> +				return -1;
> +		}
>  		/*
>  		 * To detect which key parts are optional,
>  		 * min_field_count is required. But
> @@ -2570,8 +2821,9 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>  	 * Create MoveIndex ops for the remaining indexes in the
>  	 * old space.
>  	 */
> -	alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1);
> +	alter_space_move_indexes(alter, iid + 1, space->index_id_max + 1);

13. I remember, you said that 'old_space' -> 'space' is going
to save some lines, but man. This rename diff is huge. This is
really comparable with the functional diff in this function. I
didn't think, that this rename will affect so much code. Please,
return old_space back. It is not worth introducing such a big
diff.


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.