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

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

Thanks for the fixes!

>>> 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.
> I think #pragma is disallowed, because I didn’t found it by grep in the
> src.
> 

I asked Kirill. He said that pragma is a great idea. Please,
use it in all new files, including this one.

>>> --- 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.
> But the first one is almost copy of the second one. Is it ok?

Yes. These two functions are small enough. I tried to make
a third one which they would use, but it did not make the
code smaller.

> diff --git a/src/box/space.h b/src/box/space.h
> index 7926aa65e..0c23c224c 100644
> --- a/src/box/space.h
> +++ b/src/box/space.h
> @@ -234,6 +235,8 @@ struct space {
>  	 * of parent constraints as well as child ones.
>  	 */
>  	uint64_t fk_constraint_mask;
> +	/** Hash table with constraint names. */
> +	struct mh_strnptr_t *constraint_names;

Please, rename to 'constraints'. These are not just 'names'
anymore. Sorry for the nit.