[tarantool-patches] Re: [PATCH v5 4/6] schema: add new system space for CHECK constraints
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Jun 4 00:15:19 MSK 2019
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 at 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;
More information about the Tarantool-patches
mailing list