Re: [Tarantool-patches] [PATCH 1/2] box: introduce constraint names hash table

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

Thanks for the patch!

See 9 comments below.

On 12/11/2019 15:02, Roman Khabibov wrote:
> Add hash table and API for interaction with it to struct space. This
> hash table is needed to keep constraint names of a table together and check
> their duplicates.
> 
> Needed for #3503

1. This is rather a part of 3503. Not something self sufficient.

> ---
>  src/box/space.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/box/space.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 86 insertions(+)
> 
> diff --git a/src/box/space.c b/src/box/space.c
> index 94716a414..c15b9f5b4 100644
> --- a/src/box/space.c
> +++ b/src/box/space.c
> @@ -202,6 +203,13 @@ space_create(struct space *space, struct engine *engine,
>  			}
>  		}
>  	}
> +
> +	space->constraint_names = mh_strnptr_new();

2. I think you need to store more information than just a name.
You need to store constraint type and its ID to be able to
drop a constraint knowing only its name.

> +	if (space->constraint_names == NULL) {
> +		diag_set(OutOfMemory, sizeof(*space->constraint_names), "malloc",
> +			 "constraint_names");
> +		goto fail;
> +	}
>  	return 0;
>  
>  fail_free_indexes:
> @@ -257,6 +265,7 @@ space_delete(struct space *space)
>  	trigger_destroy(&space->before_replace);
>  	trigger_destroy(&space->on_replace);
>  	space_def_delete(space->def);
> +	mh_strnptr_delete(space->constraint_names);

3. Please, add an assertion, that the hash is empty.

>  	/*
>  	 * SQL Triggers should be deleted with
>  	 * on_replace_dd_trigger on deletion from _trigger.
> @@ -617,6 +626,43 @@ space_remove_ck_constraint(struct space *space, struct ck_constraint *ck)
>  	}
>  }
>  
> +bool
> +space_has_constraint(struct space *space, const char *name)
> +{
> +	uint32_t len = strlen(name);
> +	mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name, len);
> +	if (pos != mh_end(space->constraint_names))
> +		return true;
> +	return false;

4.

--- a/src/box/space.c
+++ b/src/box/space.c
@@ -631,9 +631,7 @@ space_has_constraint(struct space *space, const char *name)
 {
 	uint32_t len = strlen(name);
 	mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name, len);
-	if (pos != mh_end(space->constraint_names))
-		return true;
-	return false;
+	return pos != mh_end(space->constraint_names);
 }

> +}
> +
> +int
> +space_put_constraint_name(struct space *space, const char *name)
> +{
> +	uint32_t len = strlen(name);
> +	uint32_t hash = mh_strn_hash(name, len);
> +	char *name_copy = malloc(len);

5. You need to check for malloc result.

> +	memcpy(name_copy, name, len);
> +	const struct mh_strnptr_node_t name_node = { name_copy, len, hash,
> +						     NULL };
> +	if (mh_strnptr_put(space->constraint_names, &name_node, NULL,
> +			   NULL) == mh_end(space->constraint_names)) {
> +		diag_set(OutOfMemory, sizeof(name_node), "malloc",
> +			 "constraint_names");
> +		return -1;

6. name_copy leaks here.

> +	}
> +
> +	return 0;
> +}
> +
> +void
> +space_drop_constraint_name(struct space *space, const char *name)
> +{
> +	mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name,
> +					   strlen(name));
> +	mh_strnptr_del(space->constraint_names, pos, NULL);

7. name_copy leaks here as well.

> +}
> +
>  /* {{{ Virtual method stubs */
>  
>  size_t
> diff --git a/src/box/space.h b/src/box/space.h
> index 7926aa65e..36a767421 100644
> --- a/src/box/space.h
> +++ b/src/box/space.h
> @@ -234,6 +234,11 @@ struct space {
>  	 * of parent constraints as well as child ones.
>  	 */
>  	uint64_t fk_constraint_mask;
> +	/**
> +	  * Hash table with constraint names. Used for non-system
> +	  * spaces only.

8. Why for non-system spaces only?

> +	  */
> +	struct mh_strnptr_t *constraint_names;
>  };
>  
>  /** Initialize a base space instance. */
> @@ -516,6 +521,41 @@ space_add_ck_constraint(struct space *space, struct ck_constraint *ck);
>  void
>  space_remove_ck_constraint(struct space *space, struct ck_constraint *ck);
>  
> +/**
> + * Check if a constraint with @a name exists within @a space.
> + *
> + * @param space Space.
> + * @param name  Constraint name.
> + *
> + * @retval true  The constraint with @a name exists.
> + * @retval false Doesn't exist respectively.
> + */
> +bool
> +space_has_constraint(struct space *space, const char *name);
> +
> +/**
> + * Put node with @a name of a constraint the the hash table of @a

9. 'the the' ?

> + * space.
> + *
> + * @param space Space.
> + * @param name  Constraint name.
> + *
> + * @retval  0 Success.
> + * @retval -1 Memory allocation error.
> + */
> +int
> +space_put_constraint_name(struct space *space, const char *name);