Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

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

Thanks for the fixes!

See 4 comments below.

> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index c12279e65..e60c2827b 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2449,6 +2449,47 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
>  	return false;
>  }
>  
> +/**
> + * Just return string with constraint type to print it in error
> + * message.
> + */
> +static inline const char *
> +cosntraint_type_str(struct constraint_def *def) {
> +	switch (def->type) {
> +	case CONSTRAINT_TYPE_PK:
> +		return "PRIMARY KEY";
> +	case CONSTRAINT_TYPE_UNIQUE:
> +		return "UNIQUE";
> +	case CONSTRAINT_TYPE_FK:
> +		return "FOREIGN KEY";
> +	case CONSTRAINT_TYPE_CK:
> +		return "CHECK";
> +	}
> +}

1. Please, use the way it is implemented for other
enums - add const char *constraint_type_strs[] array
and get a string via constraint_type_strs[type].

> +
> +/**
> + * Check if constraint with @a name exists within @a space. Call
> + * diag_set() if it is so.
> + *
> + * @param space Space to find constraint within.
> + * @param name  Constraint name.
> + *
> + * @retval true  Constraint exists.
> + * @retval false Doesn't exist.
> + */
> +static inline bool
> +space_constraint_exists(struct space *space, const char *name)

2. Please, rename to 'check_existence()'. When you write 'exists',
it looks like a simple getter method, which does not change anything.
Your method does - it changes diagnostics area. And then it will
return int: 0 or -1, as usual.

> +{
> +	struct constraint_def *def =
> +		space_constraint_def_by_name(space, name);
> +	if (def != NULL) {
> +		diag_set(ClientError, ER_CONSTRAINT_EXISTS,
> +			 cosntraint_type_str(def), name, space_name(space));
> +		return true;
> +	}
> +	return false;
> +}
> @@ -5348,13 +5387,8 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
>  		fk->def = fk_def;
>  		fk->index_id = fk_index->def->iid;
>  		if (old_tuple == NULL) {
> -			if (space_constraint_def_by_name(child_space,
> -							 fk_def->name)
> -			    != NULL) {
> -				diag_set(ClientError, ER_CONSTRAINT_EXISTS,
> -					 fk_def->name);
> +			if (space_constraint_exists(child_space, fk_def->name))
>  				return -1;
> -			}

3. I propose you to introduce space_constraint_exists in the previous
commit so as here the diff will be smaller.

>  			rlist_add_entry(&child_space->child_fk_constraint,
>  					fk, in_child_space);
>  			rlist_add_entry(&parent_space->parent_fk_constraint,> diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
> index e0bd60727..e4603e579 100755
> --- a/test/sql-tap/alter2.test.lua
> +++ b/test/sql-tap/alter2.test.lua
> @@ -246,7 +246,7 @@ test:do_catchsql_test(
>          ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES child;
>      ]], {
>          -- <alter2-5.1>
> -        1, "Constraint FK already exists"
> +        1, "Duplicate key exists in unique index 'primary' in space '_fk_constraint'"

4. Well, maybe I was wrong, and we should keep vdbe_emit_halt_with_presence_test.
It was added exactly to avoid such ugly error messages.

But the problem is that ER_CONSTRAINT_EXISTS expects multiple arguments, which
can't be passed through vdbe_emit_halt_with_presence_test.

I propose you to turn ER_CONSTRAINT_EXISTS into ER_FK_EXISTS in
vdbe_emit_fk_constraint_create, and into ER_CK_EXISTS in
vdbe_emit_ck_constraint_create. ER_CK/FK_EXISTS will have only
one parameter.

>          -- </alter2-5.1>
>      })
>