From: Roman Khabibov <roman.habibov@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 1/3] box: introduce constraint names hash table Date: Tue, 10 Dec 2019 15:48:54 +0300 [thread overview] Message-ID: <E416D2C4-8ABF-4791-A057-55139BD1C184@tarantool.org> (raw) In-Reply-To: <e752658b-1145-f8e4-fbcd-4266735a2f85@tarantool.org> Hi! Thanks for the review. > On Dec 7, 2019, at 19:34, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > 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. +++ b/src/box/constraint_def.h @@ -0,0 +1,83 @@ +#pragma once + >>>> --- 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. Done. commit aa06c0665af78a66036da88aad8001ffbd773551 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Tue Nov 5 18:06:59 2019 +0300 box: introduce constraint names hash table 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 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 <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 "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) +{ + 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", "ret"); + return NULL; + } + ret->space_id = space_id; + ret->type = 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..d1b1a725a --- /dev/null +++ b/src/box/constraint_def.h @@ -0,0 +1,83 @@ +#pragma once + +/* + * 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, + CONSTRAINT_TYPE_UNIQUE, + CONSTRAINT_TYPE_FK, + CONSTRAINT_TYPE_CK, + + CONSTRAINT_TYPE_MAX, +}; + +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 diff --git a/src/box/space.c b/src/box/space.c index 94716a414..cfae93cf1 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" int access_check_space(struct space *space, user_access_t access) @@ -202,6 +204,13 @@ space_create(struct space *space, struct engine *engine, } } } + + space->constraints = mh_strnptr_new(); + if (space->constraints == NULL) { + diag_set(OutOfMemory, sizeof(*space->constraints), "malloc", + "constraints"); + goto fail; + } return 0; 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->constraints) == 0); + mh_strnptr_delete(space->constraints); assert(space->sql_triggers == 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) } } +struct constraint_def * +space_constraint_def_by_name(struct space *space, const char *name) +{ + uint32_t len = strlen(name); + mh_int_t pos = mh_strnptr_find_inp(space->constraints, name, len); + if (pos == mh_end(space->constraints)) + return NULL; + return (struct constraint_def *) + mh_strnptr_node(space->constraints, pos)->val; +} + +int +space_put_constraint(struct space *space, struct constraint_def *def) +{ + uint32_t len = strlen(def->name); + uint32_t hash = mh_strn_hash(def->name, len); + const struct mh_strnptr_node_t name_node = { def->name, len, hash, def}; + if (mh_strnptr_put(space->constraints, &name_node, NULL, NULL) == + mh_end(space->constraints)) { + diag_set(OutOfMemory, sizeof(name_node), "malloc", + "constraints"); + return -1; + } + return 0; +} + +struct constraint_def * +space_pop_constraint(struct space *space, const char *name) +{ + uint32_t len = strlen(name); + mh_int_t pos = mh_strnptr_find_inp(space->constraints, name, len); + if (pos == mh_end(space->constraints)) + return NULL; + struct constraint_def *def = (struct constraint_def *) + mh_strnptr_node(space->constraints, pos)->val; + mh_strnptr_del(space->constraints, pos, NULL); + return def; +} + /* {{{ Virtual method stubs */ size_t diff --git a/src/box/space.h b/src/box/space.h index 7926aa65e..08b5b4777 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; 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 def objects by name. */ + struct mh_strnptr_t *constraints; }; /** 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); +/** + * 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. */
next prev parent reply other threads:[~2019-12-10 12:48 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-28 18:34 [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space Roman Khabibov 2019-11-28 18:34 ` [Tarantool-patches] [PATCH v2 1/2] box: introduce constraint names hash table Roman Khabibov 2019-11-30 1:03 ` Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 1/3] " Roman Khabibov 2019-12-07 16:34 ` Vladislav Shpilevoy 2019-12-10 12:48 ` Roman Khabibov [this message] 2019-11-28 18:34 ` [Tarantool-patches] [PATCH v2 2/2] sql: make constraint operations transactional Roman Khabibov 2019-11-29 7:38 ` Roman Khabibov 2019-11-30 1:03 ` Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 2/3] " Roman Khabibov 2019-12-05 18:43 ` Roman Khabibov 2019-12-07 16:35 ` Vladislav Shpilevoy 2019-12-10 12:49 ` [Tarantool-patches] [PATCH v2 2/3] box: " Roman Khabibov 2019-12-15 22:26 ` Vladislav Shpilevoy 2019-12-17 15:03 ` Roman Khabibov 2019-12-28 0:18 ` Nikita Pettik 2019-12-28 11:07 ` Vladislav Shpilevoy 2019-12-29 0:07 ` Nikita Pettik 2019-12-29 15:51 ` Vladislav Shpilevoy 2019-12-29 22:28 ` Nikita Pettik 2019-12-29 22:35 ` Vladislav Shpilevoy 2019-12-30 11:12 ` Sergey Ostanevich 2019-12-30 12:05 ` Nikita Pettik 2019-12-21 20:54 ` Sergey Ostanevich 2019-12-22 14:59 ` Vladislav Shpilevoy 2019-12-24 12:06 ` Roman Khabibov 2019-11-30 1:03 ` [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 0/3] " Roman Khabibov 2019-12-07 16:34 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=E416D2C4-8ABF-4791-A057-55139BD1C184@tarantool.org \ --to=roman.habibov@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 1/3] box: introduce constraint names hash table' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox