From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 514DF2E847 for ; Sun, 26 May 2019 08:06:49 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id eMKNCHFOutJT for ; Sun, 26 May 2019 08:06:49 -0400 (EDT) Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DB0CE2C75B for ; Sun, 26 May 2019 08:06:48 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 4/6] schema: add new system space for CHECK constraints References: <788556b70e154103ed1f6131db7ee1f8cd687848.1558605591.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sun, 26 May 2019 15:06:46 +0300 MIME-Version: 1.0 In-Reply-To: <788556b70e154103ed1f6131db7ee1f8cd687848.1558605591.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov Thanks for the patch! See 12 comments below. > @@ -1387,6 +1377,79 @@ UpdateSchemaVersion::alter(struct alter_space *alter) > ++schema_version; > } > > +/** > + * As ck_constraint object depends on space_def we must rebuild > + * all ck constraints on space alter. > + * > + * To perform it transactionally, we create a list of new ck > + * constraints objects in ::prepare method that is fault-tolerant. > + * Finally in ::alter or ::rollback methods we only swap thouse 1. 'thouse' word does not exist, it is clearly visible with a spell checker, which usually highlights unknown words. > + * lists securely. > + */ > +class RebuildCkConstraints: public AlterSpaceOp > +{ > + void space_swap_ck_constraint(struct space *old_space, > + struct space *new_space); > +public: > + RebuildCkConstraints(struct alter_space *alter) : AlterSpaceOp(alter), > + ck_constraint(RLIST_HEAD_INITIALIZER(ck_constraint)) {} > + struct rlist ck_constraint; > + virtual void prepare(struct alter_space *alter); > + virtual void alter(struct alter_space *alter); > + virtual void rollback(struct alter_space *alter); > + virtual ~RebuildCkConstraints(); > +}; > @@ -4040,6 +4111,176 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event) > + > +/** A trigger invoked on replace in the _ck_constraint space. */ > +static void > +on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) > +{ > + struct txn *txn = (struct txn *) event; > + txn_check_singlestatement_xc(txn, "Space _ck_constraint"); > + struct txn_stmt *stmt = txn_current_stmt(txn); > + struct tuple *old_tuple = stmt->old_tuple; > + struct tuple *new_tuple = stmt->new_tuple; > + uint32_t space_id = > + tuple_field_u32_xc(old_tuple != NULL ? old_tuple : new_tuple, > + BOX_CK_CONSTRAINT_FIELD_SPACE_ID); > + struct space *space = space_cache_find_xc(space_id); > + struct trigger *on_rollback = > + txn_alter_trigger_new(on_replace_ck_constraint_rollback, NULL); > + struct trigger *on_commit = > + txn_alter_trigger_new(on_replace_ck_constraint_commit, NULL); > + > + if (new_tuple != NULL) { > + bool is_deferred = > + tuple_field_bool_xc(new_tuple, > + BOX_CK_CONSTRAINT_FIELD_DEFERRED); > + if (is_deferred) { > + tnt_raise(ClientError, ER_UNSUPPORTED, "Tarantool", > + "deferred ck constraints"); > + } > + /* 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); }); 2. Please, implement and use ck_constraint_def_delete. Free() will leak, when ck_constraint will become more complex. > + /* > + * FIXME: Ck constraint creation on non-empty > + * space must be implemented as preparatory > + * step for ALTER SPACE ADD CONSTRAINT feature. > + */ > + struct index *pk = space_index(space, 0); > + if (pk != NULL && index_size(pk) > 0) { > + tnt_raise(ClientError, ER_CREATE_CK_CONSTRAINT, > + ck_def->name, > + "referencing space must be empty"); > + } > + struct ck_constraint *new_ck_constraint = > + ck_constraint_new(ck_def, space->def); > + if (new_ck_constraint == NULL) > + diag_raise(); > + ck_guard.is_active = false; > + const char *name = new_ck_constraint->def->name; > + struct ck_constraint *old_ck_constraint = > + space_ck_constraint_by_name(space, name, strlen(name)); > + if (old_ck_constraint != NULL) > + rlist_del_entry(old_ck_constraint, link); > + rlist_add_entry(&space->ck_constraint, new_ck_constraint, link); > + on_commit->data = old_tuple == NULL ? new_ck_constraint : > + old_ck_constraint; > + on_rollback->data = on_commit->data; > diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c > new file mode 100644 > index 000000000..c74687b31 > --- /dev/null > +++ b/src/box/ck_constraint.c > + > +void > +ck_constraint_delete(struct ck_constraint *ck_constraint) > +{ > + sql_expr_delete(sql_get(), ck_constraint->expr, false); > + free(ck_constraint->def); 3. The same. Use ck_constraint_def_delete. > + TRASH(ck_constraint); > + free(ck_constraint); > +} > diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h > new file mode 100644 > index 000000000..e20203bb5 > --- /dev/null > +++ b/src/box/ck_constraint.h > + > +/** > + * Calculate check constraint definition memory size and fields > + * offsets for given arguments. > + * > + * Alongside with struct ck_constraint_def itself, we reserve > + * memory for string containing its name and expression string. > + * > + * Memory layout: > + * +-----------------------------+ <- Allocated memory starts here > + * | struct ck_constraint_def | > + * |-----------------------------| > + * | name + \0 | > + * |-----------------------------| > + * | expr_str + \0 | > + * +-----------------------------+ > + * > + * @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. > + * @retval The size of the ck constraint definition object for > + * given parameters. 4. I said it probably 2-3 times, but you still ignore. @retval takes one parameter, @return take 0 parameters. Here you wrote, that returned value is 'The'. > + */ > +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) > +{ > + *name_offset = sizeof(struct ck_constraint_def); > + *expr_str_offset = *name_offset + name_len + 1; > + return *expr_str_offset + expr_str_len + 1; > +} > diff --git a/src/box/schema_def.h b/src/box/schema_def.h > index dea3fad19..317e6b51f 100644 > --- a/src/box/schema_def.h > +++ b/src/box/schema_def.h > @@ -108,6 +108,8 @@ enum { > BOX_SPACE_SEQUENCE_ID = 340, > /** Space id of _fk_constraint. */ > BOX_FK_CONSTRAINT_ID = 356, > + /** Space id of _ck_contraint. */ > + BOX_CK_CONSTRAINT_ID = 357, 5. Please, leave a gap after BOX_FK_CONSTRAINT_ID. We keep gaps to be able in future to add a view space, and to choose recovery order. Gap is typically 7 unused IDs. The new ID should be 364. > /** End of the reserved range of system spaces. */ > BOX_SYSTEM_ID_MAX = 511, > BOX_ID_NIL = 2147483647 > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 91b977de2..a3b11531f 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -638,40 +640,116 @@ primary_key_exit: > return; > } > > +/** > + * 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 > + * without any changes until the next (corres) ' or " . 6. What is 'corres'? > + * The wptr buffer is expected to have str_len + 1 bytes > + * (this is the expected scenario where no extra whitespace > + * characters preset in the source string). 7. What is 'characters preset'? > + * @param wptr The destination memory buffer of size > + * @a str_len + 1. > + * @param str The source string to be copied. > + * @param str_len The source string @a str length. > + */ > +static void > +trim_space_snprintf(char *wptr, const char *str, uint32_t str_len) > @@ -1416,6 +1529,37 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name, > sqlReleaseTempRange(parse_context, key_reg, 3); > } > > +/** > + * Generate VDBE program to remove entry from _ck_constraint space. > + * > + * @param parser Parsing context. > + * @param ck_name Name of CK constraint to be dropped. > + * @param child_id Id of table which constraint belongs to. 8. There is no parameter 'child_id'. > + */ > +static void > +vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name, > + uint32_t space_id) > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index f241b8d52..e93dfe751 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -297,7 +297,7 @@ ccons ::= check_constraint_def . > > check_constraint_def ::= cconsname(N) CHECK LP expr(X) RP. { > create_ck_def_init(&pParse->create_ck_def, &N, &X); > - sql_add_check_constraint(pParse); > + sql_create_check_contraint(pParse); 9. What was a motivation of this rename? As I understand, we can add many CHECK constraints in one CREATE TABLE. We do not 'create' one single CHECK, we add multiple ones. Please, keep the old name. > } > > ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R). { > diff --git a/test/sql/checks.result b/test/sql/checks.result > index f7cddec43..5f0098e23 100644 > --- a/test/sql/checks.result > +++ b/test/sql/checks.result > @@ -30,89 +32,216 @@ t = {513, 1, 'test', 'memtx', 0, opts, format} > -s = box.space._space:insert(t) > +-- Pass integer instead of expression. > +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666}) > +--- > +- error: 'Tuple field 5 type does not match one required by operation: expected string' > +... > +-- Defered CK constraints are not supported. 10. 'Defered' word does not exist, use 'deferred'. 'Unexistent' word does not exist as well. Non-existent is better. > +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5'}) > +--- > +- error: Tarantool does not support deferred ck constraints > +... > +-- The only supperted language is SQL. 11. 'Supperted' word does not exist. > +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5'}) > +--- > +- error: Unsupported language 'LUA' specified for function 'CK_CONSTRAINT_01' > +... > ... > +--- > +... > +box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), CONSTRAINT CK1 CHECK(ID < 0))") > +--- > +- error: Duplicate key exists in unique index 'primary' in space '_ck_constraint' 12. The error is not ok. It should not expose any system details like _ck_constraint space. Please, check duplicate names before an insertion. > +... > +box.space._ck_constraint:select() > +--- > +- [] > ...