Re: [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space

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

Hi! Thanks for the patch!

See 2 comments below!

On 28/11/2019 19:34, Roman Khabibov wrote:
> I have made essential changes to the previous patch, so I decided
> to send it as v2.
> 
> 1) I still don't understand, why we need to store constraint id. If we have the
> query "ALTER TABLE T DROP CONSTRAINT C", we just get struct space by its name,
> then find the corresponding constraint_def node by the name and emit opcode on
> replace in _index/_fk_constraint/_ck_constraint depending on
> constraint_def->type.

1. ID is a primary index. You can delete by ID. Is it always
possible to delete by name? Do all the constraint spaces have a
unique index over name consisting of one column?

> 
> 2) space_delete()
> I don't know how to check, if the hash table is empty, because it jsut freed.

2. Mhash provides mh_size() method. Add an assert, that it
is it == 0.

> 
> salad/mhash.h
> void
> _mh(delete)(struct _mh(t) *h)
> {
> 	if (h->shadow->p) {
> 		free(h->shadow->p);
> 		free(h->shadow->b);
> 		memset(h->shadow, 0, sizeof(*h->shadow));
> 	}
> 	free(h->shadow);
> 	free(h->b);
> 	free(h->p);
> 	free(h);
> }
> Roman Khabibov (2):
>   box: introduce constraint names hash table
>   sql: make constraint operations transactional
> 
>  src/box/CMakeLists.txt       |   1 +
>  src/box/alter.cc             | 427 +++++++++++++++++++++++++++++++----
>  src/box/constraint_def.c     |  59 +++++
>  src/box/constraint_def.h     |  83 +++++++
>  src/box/space.c              |  56 +++++
>  src/box/space.h              |  39 ++++
>  test/sql/constraint.result   | 190 ++++++++++++++++
>  test/sql/constraint.test.lua |  84 +++++++
>  8 files changed, 892 insertions(+), 47 deletions(-)
>  create mode 100755 src/box/constraint_def.c
>  create mode 100755 src/box/constraint_def.h
>  create mode 100644 test/sql/constraint.result
>  create mode 100755 test/sql/constraint.test.lua
>