From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CF4304696C0 for ; Wed, 4 Dec 2019 19:23:41 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3594.4.19\)) From: Roman Khabibov In-Reply-To: <11d5344a-0830-9c5c-9e75-30564aa257d0@tarantool.org> Date: Wed, 4 Dec 2019 19:23:41 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <3EB26BFF-D4AA-4AA0-BD3B-83992975C4BD@tarantool.org> References: <8187f6bb57792948fb122d3c2cb3cfc72975fa77.1574965971.git.roman.habibov@tarantool.org> <11d5344a-0830-9c5c-9e75-30564aa257d0@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 1/3] box: introduce constraint names hash table List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: Vladislav Shpilevoy Hi! Thanks for the review. > On Nov 30, 2019, at 04:03, Vladislav Shpilevoy = wrote: >=20 > Thanks for the patch! >=20 > See 6 comments below! >=20 > 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. >>=20 >> 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 >>=20 >> 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 >> +#include >> + >> +struct constraint_def * >> +constraint_def_new(uint32_t space_id, enum constraint_type type, >> + const char *name) { >=20 > 1. Please, put { on a separate line. In all the other places too. Done. >> + uint32_t len =3D strlen(name); >> + uint32_t size =3D sizeof(struct constraint_def) + len + 1; >> + struct constraint_def *ret =3D malloc(size); >> + if (ret =3D=3D NULL) { >> + diag_set(OutOfMemory, size, "malloc", "constraint_def"); >> + return NULL; >> + } >> + ret->space_id =3D space_id; >> + ret->type =3D type; >> + memcpy(ret->name, name, len + 1); >> + return ret; >> +} >> + >> +void >> +constraint_def_free(struct constraint_def *def) { >=20 > 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. Done. >> + 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 >=20 > 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. >=20 > 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=E2=80=99t found it by grep = in the src. >> +/* >> + * 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 ``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 >> + * 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 >> + >> +#if defined(__cplusplus) >> +extern "C" { >> +#endif >> + >> +enum constraint_type { >> + CONSTRAINT_TYPE_PK =3D 0, >=20 > 4. You can omit the assignments, if they does not matter anyway. +enum constraint_type { + CONSTRAINT_TYPE_PK =3D 0, + CONSTRAINT_TYPE_UNIQUE, + CONSTRAINT_TYPE_FK, + CONSTRAINT_TYPE_CK, +}; >> + CONSTRAINT_TYPE_UNIQUE =3D 1, >> + CONSTRAINT_TYPE_FK =3D 2, >> + CONSTRAINT_TYPE_CK =3D 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. >=20 > 5. There should not be any objects by that time. Why > is not the hash table empty? Yes, the table must be empty. @@ -258,9 +267,12 @@ space_delete(struct space *space) trigger_destroy(&space->on_replace); space_def_delete(space->def); /* - * SQL Triggers should be deleted with - * on_replace_dd_trigger on deletion from _trigger. + * SQL triggers and constraints should be deleted with + * on_replace_dd_ triggers on deletion from corresponding + * system space. */ + assert(mh_size(space->constraint_names) =3D=3D 0); + mh_strnptr_delete(space->constraint_names); assert(space->sql_triggers =3D=3D NULL); assert(rlist_empty(&space->parent_fk_constraint)); assert(rlist_empty(&space->child_fk_constraint)); >> + */ >> + struct mh_strnptr_t *mh =3D space->constraint_names; >> + while (mh_size(mh) > 0) { >> + mh_int_t i =3D mh_first(mh); >> + struct constraint_def *def =3D >> + (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); >=20 > 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? +struct constraint_def * +space_constraint_def_by_name(struct space *space, const char *name) +{ + uint32_t len =3D strlen(name); + mh_int_t pos =3D mh_strnptr_find_inp(space->constraint_names, = name, len); + if (pos =3D=3D mh_end(space->constraint_names)) + return NULL; + return (struct constraint_def *) + mh_strnptr_node(space->constraint_names, pos)->val; +} +struct constraint_def * +space_pop_constraint(struct space *space, const char *name) +{ + uint32_t len =3D strlen(name); + mh_int_t pos =3D mh_strnptr_find_inp(space->constraint_names, = name, len); + if (pos =3D=3D mh_end(space->constraint_names)) + return NULL; + struct constraint_def *def =3D (struct constraint_def *) + mh_strnptr_node(space->constraint_names, pos)->val; + mh_strnptr_del(space->constraint_names, pos, NULL); + return def; +} + commit f044d9a99b627e888cef0b935f55c8ad7f1386b1 Author: Roman Khabibov Date: Tue Nov 5 18:06:59 2019 +0300 box: introduce constraint names hash table =20 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. =20 Part of #3503 diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt index 5cd5cba81..bf3895262 100644 --- a/src/box/CMakeLists.txt +++ b/src/box/CMakeLists.txt @@ -102,6 +102,7 @@ add_library(box STATIC sequence.c ck_constraint.c fk_constraint.c + constraint_def.c func.c func_def.c key_list.c diff --git a/src/box/constraint_def.c b/src/box/constraint_def.c new file mode 100755 index 000000000..1d4367532 --- /dev/null +++ b/src/box/constraint_def.c @@ -0,0 +1,61 @@ +/* + * 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 ``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 + * 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 "constraint_def.h" +#include "assoc.h" +#include "errcode.h" +#include "diag.h" +#include +#include + +struct constraint_def * +constraint_def_new(uint32_t space_id, enum constraint_type type, + const char *name) +{ + uint32_t len =3D strlen(name); + uint32_t size =3D sizeof(struct constraint_def) + len + 1; + struct constraint_def *ret =3D malloc(size); + if (ret =3D=3D NULL) { + diag_set(OutOfMemory, size, "malloc", "ret"); + return NULL; + } + ret->space_id =3D space_id; + ret->type =3D type; + memcpy(ret->name, name, len + 1); + return ret; +} + +void +constraint_def_delete(struct constraint_def *def) +{ + free(def); +} diff --git a/src/box/constraint_def.h b/src/box/constraint_def.h new file mode 100755 index 000000000..c740bd2f0 --- /dev/null +++ b/src/box/constraint_def.h @@ -0,0 +1,83 @@ +#ifndef TARANTOOL_BOX_CONSTRAINT_DEF_H_INCLUDED +#define TARANTOOL_BOX_CONSTRAINT_DEF_H_INCLUDED +/* + * 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 ``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 + * 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 + +#if defined(__cplusplus) +extern "C" { +#endif + +enum constraint_type { + CONSTRAINT_TYPE_PK =3D 0, + CONSTRAINT_TYPE_UNIQUE, + CONSTRAINT_TYPE_FK, + CONSTRAINT_TYPE_CK, +}; + +struct constraint_def { + /** Space id. */ + uint32_t space_id; + /** Constraint type. */ + enum constraint_type type; + /** Zero-terminated string with name. */ + char name[0]; +}; + +/** + * Allocate memory and construct constraint def object. + * + * @param space_id Space id. + * @param type Constraint type. + * @param name Constraint name. + * + * @retval ptr Constraint def object. + * @retval NULL Memory allocation error. + */ +struct constraint_def * +constraint_def_new(uint32_t space_id, enum constraint_type type, + const char *name); + +/** + * Free memory of constraint def object. + * + * @param def Constraint def. + */ +void +constraint_def_delete(struct constraint_def *def); + +#if defined(__cplusplus) +} +#endif + +#endif diff --git a/src/box/space.c b/src/box/space.c index 94716a414..7e1a197a1 100644 --- a/src/box/space.c +++ b/src/box/space.c @@ -45,6 +45,8 @@ #include "iproto_constants.h" #include "schema.h" #include "ck_constraint.h" +#include "assoc.h" +#include "constraint_def.h" =20 int access_check_space(struct space *space, user_access_t access) @@ -202,6 +204,13 @@ space_create(struct space *space, struct engine = *engine, } } } + + space->constraint_names =3D mh_strnptr_new(); + if (space->constraint_names =3D=3D NULL) { + diag_set(OutOfMemory, sizeof(*space->constraint_names), + "malloc", "constraint_names"); + goto fail; + } return 0; =20 fail_free_indexes: @@ -258,9 +267,12 @@ space_delete(struct space *space) trigger_destroy(&space->on_replace); space_def_delete(space->def); /* - * SQL Triggers should be deleted with - * on_replace_dd_trigger on deletion from _trigger. + * SQL triggers and constraints should be deleted with + * on_replace_dd_ triggers on deletion from corresponding + * system space. */ + assert(mh_size(space->constraint_names) =3D=3D 0); + mh_strnptr_delete(space->constraint_names); assert(space->sql_triggers =3D=3D NULL); assert(rlist_empty(&space->parent_fk_constraint)); assert(rlist_empty(&space->child_fk_constraint)); @@ -617,6 +629,45 @@ space_remove_ck_constraint(struct space *space, = struct ck_constraint *ck) } } =20 +struct constraint_def * +space_constraint_def_by_name(struct space *space, const char *name) +{ + uint32_t len =3D strlen(name); + mh_int_t pos =3D mh_strnptr_find_inp(space->constraint_names, = name, len); + if (pos =3D=3D mh_end(space->constraint_names)) + return NULL; + return (struct constraint_def *) + mh_strnptr_node(space->constraint_names, pos)->val; +} + +int +space_put_constraint(struct space *space, struct constraint_def *def) +{ + uint32_t len =3D strlen(def->name); + uint32_t hash =3D mh_strn_hash(def->name, len); + const struct mh_strnptr_node_t name_node =3D { def->name, len, = hash, def}; + if (mh_strnptr_put(space->constraint_names, &name_node, NULL, = NULL) =3D=3D + mh_end(space->constraint_names)) { + diag_set(OutOfMemory, sizeof(name_node), "malloc", + "constraint_names"); + return -1; + } + return 0; +} + +struct constraint_def * +space_pop_constraint(struct space *space, const char *name) +{ + uint32_t len =3D strlen(name); + mh_int_t pos =3D mh_strnptr_find_inp(space->constraint_names, = name, len); + if (pos =3D=3D mh_end(space->constraint_names)) + return NULL; + struct constraint_def *def =3D (struct constraint_def *) + mh_strnptr_node(space->constraint_names, pos)->val; + mh_strnptr_del(space->constraint_names, pos, NULL); + return def; +} + /* {{{ Virtual method stubs */ =20 size_t 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 @@ -52,6 +52,7 @@ struct port; struct tuple; struct tuple_format; struct ck_constraint; +struct constraint_def; =20 struct space_vtab { /** Free a space instance. */ @@ -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; }; =20 /** Initialize a base space instance. */ @@ -516,6 +519,45 @@ space_add_ck_constraint(struct space *space, struct = ck_constraint *ck); void space_remove_ck_constraint(struct space *space, struct ck_constraint = *ck); =20 +/** + * Find node with @a name in the constraint hash table of @a + * space. + * + * @param space Space. + * @param name Constraint name. + * + * @retval constraint_def object. + * @retval NULL Constraint doesn't exist. + */ +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. + * + * @retval constraint_def object. + * @retval NULL Constraint doesn't exist. + */ +struct constraint_def * +space_pop_constraint(struct space *space, const char *name); + /* * Virtual method stubs. */