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

[Vladislav Shpilevoy]

Thanks for the patch!

See 6 comments below!

On 28/11/2019 19:34, Roman Khabibov wrote:
> Add hash table and API for interaction with it to struct space.
> This hash table is needed to keep constraint of a table together
> and check their duplicates by name.
> 
> Part of #3503
> ---
>  src/box/CMakeLists.txt   |  1 +
>  src/box/constraint_def.c | 59 ++++++++++++++++++++++++++++
>  src/box/constraint_def.h | 83 ++++++++++++++++++++++++++++++++++++++++
>  src/box/space.c          | 56 +++++++++++++++++++++++++++
>  src/box/space.h          | 39 +++++++++++++++++++
>  5 files changed, 238 insertions(+)
>  create mode 100755 src/box/constraint_def.c
>  create mode 100755 src/box/constraint_def.h
> 
> diff --git a/src/box/constraint_def.c b/src/box/constraint_def.c
> new file mode 100755
> index 000000000..914b36da3
> --- /dev/null
> +++ b/src/box/constraint_def.c
> +#include "constraint_def.h"
> +#include "assoc.h"
> +#include "errcode.h"
> +#include "diag.h"
> +#include <string.h>
> +#include <stdlib.h>
> +
> +struct constraint_def *
> +constraint_def_new(uint32_t space_id, enum constraint_type type,
> +		   const char *name) {

1. Please, put { on a separate line. In all the other places too.

> +	uint32_t len = strlen(name);
> +	uint32_t size = sizeof(struct constraint_def) + len + 1;
> +	struct constraint_def *ret = malloc(size);
> +	if (ret == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "constraint_def");
> +		return NULL;
> +	}
> +	ret->space_id = space_id;
> +	ret->type = type;
> +	memcpy(ret->name, name, len + 1);
> +	return ret;
> +}
> +
> +void
> +constraint_def_free(struct constraint_def *def) {

2. Please, rename it to _delete(). We always use new/delete() for
constructor/destructor managing memory. Free() is used only by a
few obsolete places, and by subsystem destructors.

> +	free(def);
> +}
> diff --git a/src/box/constraint_def.h b/src/box/constraint_def.h
> new file mode 100755
> index 000000000..ef238eb46
> --- /dev/null
> +++ b/src/box/constraint_def.h
> @@ -0,0 +1,83 @@
> +#ifndef INCLUDES_BOX_CONSTRAINT_DEF_H
> +#define INCLUDES_BOX_CONSTRAINT_DEF_H

3. Please, take a look at other header guards. They
have a certain naming policy. For example, tuple.h in
box/ has a guard TARANTOOL_BOX_TUPLE_H_INCLUDED. I guess
you could use TARANTOOL_BOX_CONSTRAINT_DEF_H_INCLUDED.

But even better would be to use #pragma once. I don't
know whether they are allowed in our code though.

> +/*
> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS
> + * file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include <stdint.h>
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +enum constraint_type {
> +	CONSTRAINT_TYPE_PK = 0,

4. You can omit the assignments, if they does not matter anyway.

> +	CONSTRAINT_TYPE_UNIQUE = 1,
> +	CONSTRAINT_TYPE_FK = 2,
> +	CONSTRAINT_TYPE_CK = 3
> +};
> diff --git a/src/box/space.c b/src/box/space.c
> index 94716a414..91a7d575b 100644
> --- a/src/box/space.c
> +++ b/src/box/space.c
> @@ -257,6 +266,20 @@ space_delete(struct space *space)
>  	trigger_destroy(&space->before_replace);
>  	trigger_destroy(&space->on_replace);
>  	space_def_delete(space->def);
> +
> +	/**
> +	 * Free memory of the constraint hash table. Destroy every
> +	 * constraint def object.

5. There should not be any objects by that time. Why
is not the hash table empty?

> +	 */
> +	struct mh_strnptr_t *mh = space->constraint_names;
> +	while (mh_size(mh) > 0) {
> +		mh_int_t i = mh_first(mh);
> +		struct constraint_def *def =
> +			(struct constraint_def *) mh_strnptr_node(mh, i)->val;
> +		constraint_def_free(def);
> +	}
> +	mh_strnptr_delete(mh);
> +
>  	/*
>  	 * SQL Triggers should be deleted with
>  	 * on_replace_dd_trigger on deletion from _trigger.
> @@ -617,6 +640,39 @@ space_remove_ck_constraint(struct space *space, struct ck_constraint *ck)> diff --git a/src/box/space.h b/src/box/space.h
> index 7926aa65e..96396311e 100644
> --- a/src/box/space.h
> +++ b/src/box/space.h
> @@ -516,6 +519,42 @@ space_add_ck_constraint(struct space *space, struct ck_constraint *ck);
> +struct constraint_def *
> +space_constraint_def_by_name(struct space *space, const char *name);
> +
> +/**
> + * Put node with @a def to the constraint hash table of @a space.
> + *
> + * @param space Space.
> + * @param def   Constraint def.
> + *
> + * @retval  0 Success.
> + * @retval -1 Memory allocation error.
> + */
> +int
> +space_put_constraint(struct space *space, struct constraint_def *def);
> +
> +/**
> + * Remove node with @a name from the constraint hash table of @a
> + * space. But don't destroy the constraint def object binded to
> + * this @a name.
> + *
> + * @param space Space.
> + * @param name  Constraint name.
> + */
> +void
> +space_drop_constraint(struct space *space, const char *name);

6. That function is a bit confusing. I understand, why
do you need to not destroy a constraint right here. But
drop() name really assumes that. Moreover, in most of the
places you do space_constraint_def_by_name() before
space_drop_constraint(). So this is a double search. I
propose you to rename drop to pop, and make it return the
old constraint. It will make clear, that the constraint is
not deleted here, and will eliminate the double search.