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 C09642AD3C for ; Fri, 22 Mar 2019 05:29:04 -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 98IG275kQWmJ for ; Fri, 22 Mar 2019 05:29:04 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 4D1222AD3A for ; Fri, 22 Mar 2019 05:29:04 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 5/9] schema: add new system space for CHECK constraints References: <186b109d1f6049b1413fe0cdded43e5fad29a895.1548838034.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 22 Mar 2019 12:29:02 +0300 MIME-Version: 1.0 In-Reply-To: <186b109d1f6049b1413fe0cdded43e5fad29a895.1548838034.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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 , korablev@tarantool.org Thanks for the patch! See 9 comments below. It is not a complete review, because Kirill Y. said, that the patchset is not finished (then why is it in the patches mailing list?). On 30/01/2019 11:59, Kirill Shcherbatov wrote: > This patch introduces new system space to persist check > constraints. Format of the space: > > _ck_constraint (space id = 357) > [ STR, UINT, STR] > > CK constraint is local to space, so every pair > is unique (and it is PK in _ck_constraint space). > > After insertion into this space, a new instance describing check > constraint is created. Check constraint held Expr tree. 1. held -> hold > Until space features check constraints, it isn't allowed to > be dropped. 2. Sorry, the sentence is invalid. In Russian you said literally this: "Пока спейс не поддерживает check ограничение, он не может быть удален". But why? If a space does not have a check, it is ok to drop the space. Probably you thought that 'Until' == 'While'? Vice versa, rather 'While' == 'Until not'. > The :drop() space method firstly deletes all check > constraints and than removes entry from _space. 3. than -> then. My piece of advice - go to English courses. Fortunately, Kirill and Kostja appreciate such activity and can reimburse your expenses (likely that they will). > > We use BuildCkConstraints Alter operation object because space > definition may be modified and check AST must be recreated. > > Needed for #3691 > --- > diff --git a/src/box/alter.cc b/src/box/alter.cc > index eff3524cf..3ba604ca6 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1351,6 +1341,56 @@ TruncateIndex::commit(struct alter_space *alter, int64_t signature) > index_commit_create(new_index, signature); > } > > +/** BuildCkConstraints - rebuild ck_constraints on alter. */ 4. Instead of the comments like you wrote below, it would be much better to focus your efforts on the code like this. Here I do not understand what is happening, especially in prepare(). > +class BuildCkConstraints: public AlterSpaceOp > +{ > +public: > + BuildCkConstraints(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 ~BuildCkConstraints(); > +}; > + > +void > +BuildCkConstraints::prepare(struct alter_space *alter) > +{ > + struct ck_constraint *old_ck_constraint; > + rlist_foreach_entry(old_ck_constraint, &alter->old_space->ck_constraint, > + link) { > + struct ck_constraint *new_ck_constraint = > + ck_constraint_new(old_ck_constraint->def, > + alter->new_space->def); > + if (new_ck_constraint == NULL) > + diag_raise(); > + rlist_add_entry(&ck_constraint, new_ck_constraint, link); > + } > +} > + > +void > +BuildCkConstraints::alter(struct alter_space *alter) > +{ > + rlist_swap(&alter->new_space->ck_constraint, &ck_constraint); > + rlist_swap(&ck_constraint, &alter->old_space->ck_constraint); > +} > + > +void > +BuildCkConstraints::rollback(struct alter_space *alter) > +{ > + rlist_swap(&alter->old_space->ck_constraint, &ck_constraint); > + rlist_swap(&ck_constraint, &alter->new_space->ck_constraint); > +} > + > +BuildCkConstraints::~BuildCkConstraints() > +{ > + struct ck_constraint *old_ck_constraint, *tmp; > + rlist_foreach_entry_safe(old_ck_constraint, &ck_constraint, link, tmp) > + ck_constraint_delete(old_ck_constraint); > +} > + > /** > * UpdateSchemaVersion - increment schema_version. Used on > * in alter_space_do(), i.e. when creating or dropping > @@ -1757,6 +1797,12 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) > space_name(old_space), > "the space has foreign key constraints"); > } > + /* Can't drop space having check constraints. */ > + if (!rlist_empty(&old_space->ck_constraint)) { > + tnt_raise(ClientError, ER_DROP_SPACE, > + space_name(old_space), > + "the space has check constraints"); > + } > /** > * The space must be deleted from the space > * cache right away to achieve linearisable > @@ -1854,6 +1900,8 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) > def->field_count); > (void) new CheckSpaceFormat(alter); > (void) new ModifySpace(alter, def); > + /* Add an op to rebuild check constraints. */ > + (void) new BuildCkConstraints(alter); > def_guard.is_active = false; > /* Create MoveIndex ops for all space indexes. */ > alter_space_move_indexes(alter, 0, old_space->index_id_max + 1); > @@ -2096,6 +2144,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) > * old space. > */ > alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1); > + /* Add an op to rebuild check constraints. */ > + (void) new BuildCkConstraints(alter); 5. Why do you need checks rebuild on an index update? As I know, checks depend on space format only. Strictly speaking, even on space alter you do not need to rebuild checks, if the space format is unchanged. However, I am not sure, if checks AST does not contain space pointers. > /* Add an op to update schema_version on commit. */ > (void) new UpdateSchemaVersion(alter); > alter_space_do(txn, alter); > @@ -2163,7 +2213,8 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event) > struct index *old_index = old_space->index[i]; > (void) new TruncateIndex(alter, old_index->def->iid); > } > - > + /* Add an op to rebuild check constraints. */ > + (void) new BuildCkConstraints(alter); 6. Comments like that, and the 2 same ones above looks like this, sorry: https://github.com/Gerold103/tarantool-memes/blob/master/CAPTAIN.png How is such a comment supposed to help a reader? > alter_space_do(txn, alter); > scoped_guard.is_active = false; > } > @@ -4051,6 +4102,144 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event) > +/** > + * Trigger invoked on commit in the _ck_constraint space. > + * Drop useless old check constraint object if exists. > + */ > +static void > +on_replace_ck_constraint_commit(struct trigger *trigger, void * /* event */) > +{ > + struct ck_constraint *old_ck_constraint = > + (struct ck_constraint *)trigger->data; > + if (old_ck_constraint != NULL) > + ck_constraint_delete(old_ck_constraint); > + ++schema_version; 7. We have UpdateSchemaVersion for that. It can be touched direcrtly only by _space triggers. > +} > + > +/** 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); > + access_check_ddl(space->def->name, space->def->id, space->def->uid, > + SC_SPACE, PRIV_A); > + > + 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) { > + /* Create or replace check constraint. */ > + struct ck_constraint_def *ck_constraint_def = > + ck_constraint_def_decode(new_tuple, &fiber()->gc); > + struct ck_constraint *new_ck_constraint = > + ck_constraint_new(ck_constraint_def, space->def); > + if (new_ck_constraint == NULL) > + diag_raise(); > + const char *space_name = new_ck_constraint->def->name; > + struct ck_constraint *old_ck_constraint = > + space_ck_constraint_by_name(space, space_name, > + strlen(space_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_ck_constraint; > + on_rollback->data = old_tuple == NULL ? new_ck_constraint : > + old_ck_constraint; > + } else if (new_tuple == NULL && old_tuple != NULL) { 8. As I understand, it should be an assertion, not 'if'. When new_tuple == NULL, it is impossible that old_tuple == NULL too. What is more, if you reached 'else' branch, then 'new_tuple == NULL' is already true, and you do not need to check that again. > + /* Drop check constraint. */ > + uint32_t name_len; > + const char *name = > + tuple_field_str_xc(old_tuple, > + BOX_CK_CONSTRAINT_FIELD_NAME, > + &name_len); > + struct ck_constraint *old_ck_constraint = > + space_ck_constraint_by_name(space, name, name_len); > + assert(old_ck_constraint != NULL); > + rlist_del_entry(old_ck_constraint, link); > + on_commit->data = old_ck_constraint; > + on_rollback->data = old_ck_constraint; > + } > + > + txn_on_rollback(txn, on_rollback); > + txn_on_commit(txn, on_commit); > +} > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua > index cc172dc15..a304290a2 100644 > --- a/src/box/lua/upgrade.lua > +++ b/src/box/lua/upgrade.lua > @@ -629,10 +631,46 @@ local function upgrade_priv_to_2_1_1() > local function upgrade_to_2_1_1() > log.info("started upgrade_to_2_1_1") > + local _space = box.space[box.schema.SPACE_ID] > + local _index = box.space[box.schema.INDEX_ID] > + local _ck_constraint = box.space[box.schema.CK_CONSTRAINT_ID] > + local MAP = setmap({}) > + > + log.info("create space _ck_constraint") > + local format = {{name='name', type='string'}, > + {name='space_id', type='unsigned'}, > + {name='expr_str', type='str'}} 9. Why a name is mandatory? As I know, I can omit the name, according to the standard. > + _space:insert{_ck_constraint.id, ADMIN, '_ck_constraint', 'memtx', 0, MAP, format} > + > + log.info("create index primary on _ck_constraint") > + _index:insert{_ck_constraint.id, 0, 'primary', 'tree', > + {unique = true}, {{0, 'string'}, {1, 'unsigned'}}} > + > + log.info("create secondary index child_id on _ck_constraint") > + _index:insert{_ck_constraint.id, 1, 'space_id', 'tree', > + {unique = false}, {{1, 'unsigned'}}} > + > upgrade_priv_to_2_1_1() > end >