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

Roman Khabibov roman.habibov at tarantool.org
Wed Dec 4 19:23:41 MSK 2019


Hi! Thanks for the review.

> On Nov 30, 2019, at 04:03, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> 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.
Done.

>> +	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.
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
> 
> 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.

>> +/*
>> + * 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.
+enum constraint_type {
+	CONSTRAINT_TYPE_PK = 0,
+	CONSTRAINT_TYPE_UNIQUE,
+	CONSTRAINT_TYPE_FK,
+	CONSTRAINT_TYPE_CK,
+};

>> +	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?
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) == 0);
+	mh_strnptr_delete(space->constraint_names);
 	assert(space->sql_triggers == NULL);
 	assert(rlist_empty(&space->parent_fk_constraint));
 	assert(rlist_empty(&space->child_fk_constraint));

>> +	 */
>> +	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.
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 = strlen(name);
+	mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name, len);
+	if (pos == 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 = strlen(name);
+	mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name, len);
+	if (pos == mh_end(space->constraint_names))
+		return NULL;
+	struct constraint_def *def = (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 <roman.habibov at 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..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 <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,
+};
+
+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"
 
 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 = mh_strnptr_new();
+	if (space->constraint_names == NULL) {
+		diag_set(OutOfMemory, sizeof(*space->constraint_names),
+			 "malloc", "constraint_names");
+		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->constraint_names) == 0);
+	mh_strnptr_delete(space->constraint_names);
 	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->constraint_names, name, len);
+	if (pos == 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 = 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->constraint_names, &name_node, NULL, NULL) ==
+	    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 = strlen(name);
+	mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name, len);
+	if (pos == mh_end(space->constraint_names))
+		return NULL;
+	struct constraint_def *def = (struct constraint_def *)
+		mh_strnptr_node(space->constraint_names, pos)->val;
+	mh_strnptr_del(space->constraint_names, pos, NULL);
+	return def;
+}
+
 /* {{{ Virtual method stubs */
 
 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;
 
 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;
 };
 
 /** 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.
  */



More information about the Tarantool-patches mailing list