[Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Dec 7 19:35:59 MSK 2019
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>
> })
>
More information about the Tarantool-patches
mailing list