From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Roman Khabibov <roman.habibov@tarantool.org>, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 2/3] sql: make constraint operations transactional Date: Sat, 7 Dec 2019 17:35:27 +0100 [thread overview] Message-ID: <f982e884-1835-22ff-4237-3ac001965bda@tarantool.org> (raw) In-Reply-To: <FE9F3144-D4E5-4742-8AC8-9363A49C3797@tarantool.org> Thanks for the fixes! See 9 comments below. >> 14. Talking of the super mess with on_replace_dd_index, and >> how tight it is now with all these struggling about replacing >> constraints, and on_commit/on_rollback. I think, that this is >> a dead end. Too complex. >> >> How about introducing a couple of new AlterSpaceOp classes? >> AddConstraint, and DropConstraint. AlterSpaceOp looks good >> because >> - it will allow you to not set on_commit/on_rollback triggers >> manually - it already provides these methods; >> - you will be able to move this huge pieces of code out of >> on_replace_dd_index and other on_replace_*; >> - it is consistent with MoveIndex/DropIndex/etc classes. >> >> All you will need to do is to add to on_replace_* >> (void) new AddConstraint(), and (void) new DropConstraint. >> And you will keep (void) new MoveConstraints(). >> >> Also that should eliminate code duplication between different >> on_replace_* functions about adding/dropping constraints. > +/** > + * CreateConstraintDef - add a new constraint def to the space. > + */ > +class CreateConstraintDef: public AlterSpaceOp > +{ > +private: > + struct constraint_def *new_constraint_def; > +public: > + CreateConstraintDef(struct alter_space *alter, > + struct constraint_def *def) > + :AlterSpaceOp(alter), new_constraint_def(def) > + {} > + virtual void alter(struct alter_space *alter); > + virtual void rollback(struct alter_space *alter); > +}; > + > +void > +CreateConstraintDef::alter(struct alter_space *alter) > +{ > + assert(new_constraint_def != NULL); > + if (space_put_constraint(alter->old_space, new_constraint_def) != 0) > + panic("Can't recover after constraint alter (out of memory)"); 1. Wow, why panic? Alter can safely fail. It it is not a commit or rollback trigger. Maybe it can't return an error, but it can throw it. (Although I would like to be able to panic on malloc fails, but it does not seem to happen in the nearest future.) > +} > + > +void > +CreateConstraintDef::rollback(struct alter_space *alter) > +{ > + assert(space_pop_constraint(alter->new_space, > + new_constraint_def->name) == > + new_constraint_def); 2. This will fail in Release build mode, because assert arguments are not calculated in Release. So you delete the object, but it is still in the hash table. > + (void) alter; > + constraint_def_delete(new_constraint_def); > +} > + > > I know, that I can combine these ifs to the one, but I did so, because I believe that > it is more readable. 3. Yes, here I agree. > > + struct index_def *old_def = old_index->def; > + /* > + * We put a new name either an index is becoming > + * unique (i.e. constraint), or when an unique > + * index's name is under change. > + */ > + if (!old_def->opts.is_unique && index_def->opts.is_unique && > + create_index_as_constraint(alter, index_def) != 0) > + return -1; > + if (old_def->opts.is_unique && index_def->opts.is_unique && > + strcmp(index_def->name, old_def->name) != 0 && > + (create_index_as_constraint(alter, index_def) != 0 || > + drop_index_as_constraint(alter, old_def) != 0)) > + return -1; > + if (old_def->opts.is_unique && !index_def->opts.is_unique && > + drop_index_as_constraint(alter, old_def) != 0) > + return -1; > > > commit c54d2e89d660d2f0b07f1e6917327131362a6568 > Author: Roman Khabibov <roman.habibov@tarantool.org> > Date: Wed Oct 23 15:54:16 2019 +0300 > > sql: make constraint operations transactional > > Put constraint names into the space's hash table and drop them on > replace in corresponding system spaces (_index, _fk_constraint, > _ck_constraint). > > Closes #3503 > > @TarantoolBot document > Title: Table constraints in SQL > > SQL: > According to ANSI SQL, table constraint is one of the following > entities: PRIMARY KEY, UNIQUE, FOREIGN KEY, CHECK. Every > constraint have its own name passed by user or automatically > generated. And these names must be unique within one table/space. > Naming in SQL is case-insensitive, so "CONSTRAINT c" and > "CONSTRAINT C" are the same. For example, you will get error, if > you try to: > > CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0), > CONSTRAINT c PRIMARY KEY (i)); > > or > > CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY); > ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);'); > > The same is for any other constraint types. > > Lua/box: > > You also can create/drop table constraints from box. See > space_object:create_check_constraint() and space_object:create_index() > (if an index is unique). Naming in box is case-sensitive, so 'c' and > 'C' are not the same (see naming policy). For example, an unique > index is a constraint, but a non-unique index is not. So, a non-unique > index can have the same name with a check/foreign key constraint > within one space: > > box.execute('CREATE TABLE t2 (i INT PRIMARY KEY);'); > box.execute('CREATE INDEX e ON t2(i);'); > box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);'); > > But, if you try to: > > box.space.T2.index.E:alter({unique = true}); > > You will get error, beacuse index 'e' is becoming unique. 4. That is a good comment. (But typo: beacuse -> because.) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index bef25b605..c03bad589 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc> @@ -2510,18 +2649,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) > if (old_index == NULL && new_tuple != NULL) { > if (alter_space_move_indexes(alter, 0, iid)) > return -1; > + struct index_def *def = > + index_def_new_from_tuple(new_tuple, old_space); > + if (def == NULL) > + return -1; > + if (def->opts.is_unique && > + create_index_as_constraint(alter, def) != 0) { > + index_def_delete(def); > + return -1; > + } > CreateIndex *create_index; > try { > create_index = new CreateIndex(alter); > } catch (Exception *e) { > return -1; 5. 'def' leaks here. > } > - create_index->new_index_def = > - index_def_new_from_tuple(new_tuple, old_space); > - if (create_index->new_index_def == NULL) > - return -1; > - index_def_update_optionality(create_index->new_index_def, > - alter->new_min_field_count); > + create_index->new_index_def = def; > + index_def_update_optionality(def, alter->new_min_field_count); > } > /* Case 3 and 4: check if we need to rebuild index data. */ > if (old_index != NULL && new_tuple != NULL) { > @@ -2531,6 +2675,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) > return -1; > auto index_def_guard = > make_scoped_guard([=] { index_def_delete(index_def); }); > + struct index_def *old_def = old_index->def; > + /* > + * We put a new name either an index is becoming 6. 'either' -> 'when either'. > + * unique (i.e. constraint), or when an unique > + * index's name is under change. > + */ > + if (!old_def->opts.is_unique && index_def->opts.is_unique && > + create_index_as_constraint(alter, index_def) != 0) > + return -1; > + if (old_def->opts.is_unique && index_def->opts.is_unique && > + strcmp(index_def->name, old_def->name) != 0 && > + (create_index_as_constraint(alter, index_def) != 0 || > + drop_index_as_constraint(alter, old_def) != 0)) > + return -1; > + if (old_def->opts.is_unique && !index_def->opts.is_unique && > + drop_index_as_constraint(alter, old_def) != 0) > + return -1; > /* > * To detect which key parts are optional, > * min_field_count is required. But> @@ -5219,6 +5407,16 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event) > fk); > if (on_rollback == NULL) > return -1; > + struct constraint_def *constr_def = > + constraint_def_new(child_space->def->id, > + CONSTRAINT_TYPE_FK, > + fk_def->name); > + if (constr_def == NULL) > + return -1; > + if (space_put_constraint(child_space, constr_def) != 0) { > + constraint_def_delete(constr_def); > + return -1; > + } 7. Sequence constraint_def_new -> space_put_constraint is repeated 4 times. I think this is worth wrapping into a function. The same about space_constraint_def_by_name -> space_pop_constraint -> constraint_def_delete. > txn_stmt_on_rollback(stmt, on_rollback); > fk_constraint_set_mask(fk, > &parent_space->fk_constraint_mask, > diff --git a/test/sql/constraint.result b/test/sql/constraint.result > new file mode 100644 > index 000000000..ba262182b > --- /dev/null > +++ b/test/sql/constraint.result > @@ -0,0 +1,190 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... > +engine = test_run:get_cfg('engine') > + | --- > + | ... > +box.execute('pragma sql_default_engine=\''..engine..'\'') > + | --- > + | - row_count: 0 > + | ... > +test_run:cmd("setopt delimiter ';'") 8. Please, don't. There is only one small place where you need multiple lines. You can either use ';' just for it, or use /. > diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua > new file mode 100755 > index 000000000..50162e47d > --- /dev/null > +++ b/test/sql/constraint.test.lua 9. Your test leaves lots of not deleted data. Please, do cleanup.
next prev parent reply other threads:[~2019-12-07 16:35 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-28 18:34 [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space Roman Khabibov 2019-11-28 18:34 ` [Tarantool-patches] [PATCH v2 1/2] box: introduce constraint names hash table Roman Khabibov 2019-11-30 1:03 ` Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 1/3] " Roman Khabibov 2019-12-07 16:34 ` Vladislav Shpilevoy 2019-12-10 12:48 ` Roman Khabibov 2019-11-28 18:34 ` [Tarantool-patches] [PATCH v2 2/2] sql: make constraint operations transactional Roman Khabibov 2019-11-29 7:38 ` Roman Khabibov 2019-11-30 1:03 ` Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 2/3] " Roman Khabibov 2019-12-05 18:43 ` Roman Khabibov 2019-12-07 16:35 ` Vladislav Shpilevoy [this message] 2019-12-10 12:49 ` [Tarantool-patches] [PATCH v2 2/3] box: " Roman Khabibov 2019-12-15 22:26 ` Vladislav Shpilevoy 2019-12-17 15:03 ` Roman Khabibov 2019-12-28 0:18 ` Nikita Pettik 2019-12-28 11:07 ` Vladislav Shpilevoy 2019-12-29 0:07 ` Nikita Pettik 2019-12-29 15:51 ` Vladislav Shpilevoy 2019-12-29 22:28 ` Nikita Pettik 2019-12-29 22:35 ` Vladislav Shpilevoy 2019-12-30 11:12 ` Sergey Ostanevich 2019-12-30 12:05 ` Nikita Pettik 2019-12-21 20:54 ` Sergey Ostanevich 2019-12-22 14:59 ` Vladislav Shpilevoy 2019-12-24 12:06 ` Roman Khabibov 2019-11-30 1:03 ` [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 0/3] " Roman Khabibov 2019-12-07 16:34 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=f982e884-1835-22ff-4237-3ac001965bda@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=roman.habibov@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 2/3] sql: make constraint operations transactional' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox