[tarantool-patches] Re: [PATCH v5 4/6] schema: add new system space for CHECK constraints

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

Thanks for the fixes! See 1 comment below,
review fixes on the branch, and at the end of the email.

> commit cae1ff98e7d190a9fec5c573c84a2ad7c642f1c4
> Author: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date:   Thu Apr 4 14:58:23 2019 +0300
> 
>     schema: add new system space for CHECK constraints
>     
>     This patch introduces a new system space to persist check
>     constraints. The format of the new system space is
>     
>     _ck_constraint (space id = 364)
>     [<space id> UINT, <constraint name> STR,
>      <is_deferred>BOOL, <language>STR], <code>STR

1. '<code> STR' is out of [].

>     
>     A CK constraint is local for a space, so every pair
>     <space id, CK name> is unique
>     (it is also the PK in the _ck_constraint space).
>     
>     After insertion into this space, a new instance describing check
>     constraint is created. Check constraint holds an exspression AST.
>     While space features some check constraints, it isn't allowed to
>     be dropped. The :drop() space method firstly deletes all check
>     constraints and then removes an entry from the _space.
>     
>     Because the space alter, the index alter and the space truncate
>     operations cause space recreation process, a new
>     RebuildCkConstrains object is introduced. This alter object
>     compiles a new ck constraint object, replaces and removes
>     an existent instances atomically (but if the assembly of some
>     ck constraint object fails, nothing is changed).
>     In fact, in scope of this patch we don't really need to recreate
>     a ck_constraint object in such situations (it is enough to patch
>     space_def pointer in AST tree like we did it before, but we are
>     going to recompile a VDBE that represents ck constraint in
>     further patches, and that operation is not safe).
>     
>     The main motivation for these changes is an ability to support
>     ADD CHECK CONSTRAINT operation in the future. CK constraints are
>     easier to manage as self-sustained objects: such change is
>     managed with atomic insertion(unlike the current architecture).
>     
>     Finally, the xfer optimization is disabled now if some space have
>     ck constraints. In following patches this xfer optimisation
>     becomes impossible, so there is no reason to rewrite this code
>     now.
>     
>     Needed for #3691
> 

I've pushed my review fixes in a separate commit. I didn't
fix the commit message, because I can't do it without amendment.
Below are my explanations, and diff.

1) I replaced free() with ck_constraint_def_delete() in one place.

2) I replaced 'char *ck_constraint_def.name' with
'char ck_constraint_def.name[0]' - it simplified several places,
and occupies 8 bytes less memory (yes, I like micro optimizations).

3) I fixed one another typo in the comment on trim_space_snprintf.

==================================================================

diff --git a/src/box/alter.cc b/src/box/alter.cc
index bedeb71cd..f72a2ed25 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4273,7 +4273,9 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 		/* Create or replace check constraint. */
 		struct ck_constraint_def *ck_def =
 			ck_constraint_def_new_from_tuple(new_tuple);
-		auto ck_guard = make_scoped_guard([=] { free(ck_def); });
+		auto ck_guard = make_scoped_guard([=] {
+			ck_constraint_def_delete(ck_def);
+		});
 		/*
 		 * FIXME: Ck constraint creation on non-empty
 		 * space is not implemented yet.
diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
index 69b9793ea..e25f4506e 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -41,17 +41,15 @@ ck_constraint_def_new(const char *name, uint32_t name_len, const char *expr_str,
 		      uint32_t expr_str_len, uint32_t space_id,
 		      enum ck_constraint_language language)
 {
-	uint32_t name_offset, expr_str_offset;
-	uint32_t ck_def_sz =
-		ck_constraint_def_sizeof(name_len, expr_str_len, &name_offset,
-					 &expr_str_offset);
+	uint32_t expr_str_offset;
+	uint32_t ck_def_sz = ck_constraint_def_sizeof(name_len, expr_str_len,
+						      &expr_str_offset);
 	struct ck_constraint_def *ck_def =
 		(struct ck_constraint_def *) malloc(ck_def_sz);
 	if (ck_def == NULL) {
 		diag_set(OutOfMemory, ck_def_sz, "malloc", "ck_def");
 		return NULL;
 	}
-	ck_def->name = (char *)ck_def + name_offset;
 	ck_def->expr_str = (char *)ck_def + expr_str_offset;
 	ck_def->language = language;
 	ck_def->space_id = space_id;
diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
index 3ae3d5c91..a5b7b2c71 100644
--- a/src/box/ck_constraint.h
+++ b/src/box/ck_constraint.h
@@ -57,11 +57,6 @@ extern const char *ck_constraint_language_strs[];
  * details and memory layout.
  */
 struct ck_constraint_def {
-	/**
-	 * The 0-terminated string, a name of the check
-	 * constraint. Must be unique for a given space.
-	 */
-	char *name;
 	/**
 	 * The 0-terminated string that defines check constraint
 	 * expression.
@@ -76,6 +71,11 @@ struct ck_constraint_def {
 	uint32_t space_id;
 	/** The language of ck constraint. */
 	enum ck_constraint_language language;
+	/**
+	 * The 0-terminated string, a name of the check
+	 * constraint. Must be unique for a given space.
+	 */
+	char name[0];
 };
 
 /**
@@ -117,17 +117,15 @@ struct ck_constraint {
  *
  * @param name_len The length of the name.
  * @param expr_str_len The length of the expr_str.
- * @param[out] name_offset The offset of the name string.
  * @param[out] expr_str_offset The offset of the expr_str string.
  * @return The size of the ck constraint definition object for
  *         given parameters.
  */
 static inline uint32_t
 ck_constraint_def_sizeof(uint32_t name_len, uint32_t expr_str_len,
-			 uint32_t *name_offset, uint32_t *expr_str_offset)
+			 uint32_t *expr_str_offset)
 {
-	*name_offset = sizeof(struct ck_constraint_def);
-	*expr_str_offset = *name_offset + name_len + 1;
+	*expr_str_offset = sizeof(struct ck_constraint_def) + name_len + 1;
 	return *expr_str_offset + expr_str_len + 1;
 }
 
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 09a15cb18..b28e7b66f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -644,7 +644,7 @@ primary_key_exit:
  * Prepare a 0-terminated string in the wptr memory buffer that
  * does not contain a sequence of more than one whatespace
  * character. Routine enforces ' ' (space) as whitespace
- * delimiter. When character ' or " was met, the sting is copied
+ * delimiter. When character ' or " was met, the string is copied
  * without any changes until the next ' or " sign.
  * The wptr buffer is expected to have str_len + 1 bytes
  * (this is the expected scenario where no extra whitespace
@@ -721,13 +721,11 @@ sql_create_check_contraint(struct Parse *parser)
 	 * region:
 	 *
 	 *    [ck_parse][ck_def[name][expr_str]]
-	 *         |_____^  |___^     ^
-	 *                  |_________|
+	 *         |_____^  |_________^
 	 */
-	uint32_t name_offset, expr_str_offset;
-	uint32_t ck_def_sz =
-		ck_constraint_def_sizeof(name_len, expr_str_len, &name_offset,
-					 &expr_str_offset);
+	uint32_t expr_str_offset;
+	uint32_t ck_def_sz = ck_constraint_def_sizeof(name_len, expr_str_len,
+						      &expr_str_offset);
 	struct ck_constraint_parse *ck_parse =
 		region_alloc(region, sizeof(*ck_parse) + ck_def_sz);
 	if (ck_parse == NULL) {
@@ -742,7 +740,6 @@ sql_create_check_contraint(struct Parse *parser)
 	ck_parse->ck_def = ck_def;
 	rlist_create(&ck_parse->link);
 
-	ck_def->name = (char *)ck_def + name_offset;
 	ck_def->expr_str = (char *)ck_def + expr_str_offset;
 	ck_def->language = CK_CONSTRAINT_LANGUAGE_SQL;
 	ck_def->space_id = BOX_ID_NIL;