From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Roman Khabibov <roman.habibov@tarantool.org>,
tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 2/2] sql: make constraint operations transactional
Date: Wed, 13 Nov 2019 00:04:30 +0100 [thread overview]
Message-ID: <ccca11aa-efa5-6a0c-9473-91fec1f28151@tarantool.org> (raw)
In-Reply-To: <792e035b34839371b2fae03d91c782ebbe04c27c.1573566885.git.roman.habibov@tarantool.org>
Thanks for the patch!
See 22 comments below.
On 12/11/2019 15:02, Roman Khabibov wrote:
> Put constraint names into the space's hash table and drop them on
> commit, replace and drop (in the alter.cc triggers).
>
> Closes #3503
1. Please, write a documentation request describing what is
changed in the public behaviour.
> ---
> src/box/alter.cc | 254 +++++++++++++++++++++++++++++--
> test/sql-tap/constraint.test.lua | 244 +++++++++++++++++++++++++++++
> 2 files changed, 488 insertions(+), 10 deletions(-)
> create mode 100755 test/sql-tap/constraint.test.lua
>
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 272d7cd32..8255f2f83 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1690,6 +1690,38 @@ MoveCkConstraints::rollback(struct alter_space *alter)
> space_swap_ck_constraint(alter->new_space, alter->old_space);
> }
>
> +/**
> + * Move constraint names hash table from old space to the new one.
> + */
> +class MoveConstraints: public AlterSpaceOp
> +{
> + void space_swap_constraint(struct space *old_space,
> + struct space *new_space);
> +public:
> + MoveConstraints(struct alter_space *alter) : AlterSpaceOp(alter) {}
> + virtual void alter(struct alter_space *alter);
> + virtual void rollback(struct alter_space *alter);
> +};
> +
> +void
> +MoveConstraints::space_swap_constraint(struct space *old_space,
> + struct space *new_space)
> +{
> + SWAP(new_space->constraint_names, old_space->constraint_names);
> +}
2. I think this one-liner method can be inlined.
> +
> +void
> +MoveConstraints::alter(struct alter_space *alter)
> +{
> + space_swap_constraint(alter->old_space, alter->new_space);
> +}
> +
> +void
> +MoveConstraints::rollback(struct alter_space *alter)
> +{
> + space_swap_constraint(alter->new_space, alter->old_space);
> +}
> +
> /* }}} */
> @@ -2258,6 +2291,73 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
> return false;
> }
>
> +/**
> + * Bring back the name of a unique index to the hash table of a
> + * space.
> + *
> + * Rollback DROP index.
> + *
> + * @param trigger Trigger.
> + * @param event Event.
3. These @param are totally useless here. I propose to drop them.
In other similar places too.
> + */
> +static int
> +on_drop_index_rollback(struct trigger *trigger, void * /* event */)
> +{
> + struct index_def *def = (struct index_def *)trigger->data;
> + struct space *space = space_by_id(def->space_id);
> + if (space_put_constraint_name(space, def->name) != 0) {
> + panic("Can't recover after index drop rollback (out of "
> + "memory)");
> + }
> + index_def_delete(def);
> + return 0;
> +}
> @@ -2393,17 +2493,46 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
> }
> alter_space_move_indexes(alter, 0, iid);
> (void) new DropIndex(alter, old_index);
> + if (old_index->def->opts.is_unique &&
> + !space_is_system(old_space)) {
> + space_drop_constraint_name(old_space,
> + old_index->def->name);
> + struct trigger *on_rollback =
> + txn_alter_trigger_new(NULL, NULL);
> + if (on_rollback == NULL)
> + return -1;
> + on_rollback->data = index_def_dup(old_index->def);
4. This index def leaks in case of commit.
5. You don't check for NULL.
6. Why do you need the whole def, why a name is not enough?
7. Why can't you use region memory?
8. Why can't you take a pointer at old_index->def->name instead
of copying it? The same about new index.
All the same questions about each other similar place.
> + on_rollback->run = on_drop_index_rollback;
> + txn_stmt_on_rollback(stmt, on_rollback);
> + }
> }
> /* Case 2: create an index, if it is simply created. */
> if (old_index == NULL && new_tuple != NULL) {
> + struct index_def *def = NULL;
9. Why do you nullify it, if you assign this variable on a next
line?
> + def = index_def_new_from_tuple(new_tuple, old_space);
> + if (def == NULL)
> + return -1;
> + if (def->opts.is_unique && !space_is_system(old_space)) {
> + if (space_has_constraint(old_space, def->name)) {
> + diag_set(ClientError, ER_CONSTRAINT_EXISTS,
> + def->name);
> + return -1;
> + }
> + if (space_put_constraint_name(old_space,
> + def->name) != 0)
> + return -1;
> + struct trigger *on_rollback =
> + txn_alter_trigger_new(NULL, NULL);
> + if (on_rollback == NULL)
> + return -1;
> + on_rollback->data = index_def_dup(def);
> + on_rollback->run = on_create_index_rollback;
> + txn_stmt_on_rollback(stmt, on_rollback);
> + }
> alter_space_move_indexes(alter, 0, iid);
> CreateIndex *create_index = new CreateIndex(alter);
> - 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) {
> @@ -2411,6 +2540,82 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
> index_def = index_def_new_from_tuple(new_tuple, old_space);
> if (index_def == NULL)
> return -1;
> + struct index_def *old_def = old_index->def;
> + if (!space_is_system(old_space)) {
> + /**
> + * We put a new name either the an index
10. 'the an index'? - don't understand. Please, rephrase.
> + * 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) {
> + if (space_has_constraint(old_space,
> + index_def->name) != 0) {
> + diag_set(ClientError,
> + ER_CONSTRAINT_EXISTS,
> + index_def->name);
> + return -1;
> + }> @@ -5121,6 +5343,9 @@ on_drop_ck_constraint_commit(struct trigger *trigger, void * /* event */)
> {
> struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
> assert(ck != NULL);
> + struct space *space = space_by_id(ck->def->space_id);
> + if (space != NULL)
> + space_drop_constraint_name(space, ck->def->name);
11. How is it possible, that space == NULL?
> ck_constraint_delete(ck);
> return 0;
> }
> diff --git a/test/sql-tap/constraint.test.lua b/test/sql-tap/constraint.test.lua
> new file mode 100755
> index 000000000..6444c06e9
> --- /dev/null
> +++ b/test/sql-tap/constraint.test.lua
> @@ -0,0 +1,244 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(20)
> +
> +--!./tcltestrunner.lua
> +-- 2001 September 15
12. I don't think you created this file in 2001. Please, don't
blindly copy code, and read it before submission. This whole
comment is garbage from SQLite.
> +--
> +-- The author disclaims copyright to this source code. In place
> +-- of a legal notice, here is a blessing:
> +--
> +-- May you do good and not evil.
> +-- May you find forgiveness for yourself and forgive others.
> +-- May you share freely, never taking more than you give.
> +--
> +------------------------------------------------------------------
> +-- This file implements regression tests for sql library. The
> +-- focus of this file is testing the constraint creation, alter
> +-- and drop.
> +--
> +
> +--
> +-- gh-3503 Check constraint name for duplicate.
> +--
> +test:do_execsql_test(
> + "constraint-1.1",
> + [[
> + CREATE TABLE t1 (i INT PRIMARY KEY);
> + DROP TABLE IF EXISTS t2;
> + ]], {
> + -- <constraint-1.1>
> + -- </constraint-1.1>
13. We don't write these tags. They are artifacts from auto conversion
from TCL for Lua.
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.2",
> + [[
> + CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0),
> + CONSTRAINT c PRIMARY KEY (i));
> + ]], {
> + -- <constraint-1.2>
> + 1,"Constraint C already exists"
14. I think you need to add more information. At least space name,
and the existing constraint type.
> + -- </constraint-1.2>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.3",
> + [[
> + CREATE TABLE t2 (i INT,
> + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
> + CONSTRAINT c PRIMARY KEY (i));
> + ]], {
> + -- <constraint-1.3>
> + 1,"Constraint C already exists"
> + -- </constraint-1.3>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.4",
> + [[
> + CREATE TABLE t2 (i INT PRIMARY KEY,
> + CONSTRAINT c CHECK (i > 0),
> + CONSTRAINT c UNIQUE (i));
> + ]], {
> + -- <constraint-1.4>
> + 1,"Constraint C already exists"
> + -- </constraint-1.4>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.5",
> + [[
> + CREATE TABLE t2 (i INT PRIMARY KEY,
> + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
> + CONSTRAINT c UNIQUE (i));
> + ]], {
> + -- <constraint-1.5>
> + 1,"Constraint C already exists"
> + -- </constraint-1.5>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.6",
> + [[
> + CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY,
> + CONSTRAINT c CHECK (i > 0));
15. You have exactly the same test in 1.2.
> + ]], {
> + -- <constraint-1.6>
> + 1,"Constraint C already exists"
> + -- </constraint-1.6>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.7",
> + [[
> + CREATE TABLE t2 (i INT PRIMARY KEY,
> + CONSTRAINT c UNIQUE (i),
> + CONSTRAINT c CHECK (i > 0));
16. 1.4 is the same.
> + ]], {
> + -- <constraint-1.7>
> + 1,"Constraint C already exists"
> + -- </constraint-1.7>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.8",
> + [[
> + CREATE TABLE t2 (i INT PRIMARY KEY,
> + CONSTRAINT c CHECK (i > 0),
> + CONSTRAINT c CHECK (i < 0));
> + ]], {
> + -- <constraint-1.8>
> + 1,"Constraint C already exists"
> + -- </constraint-1.8>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.9",
> + [[
> + CREATE TABLE t2 (i INT PRIMARY KEY,
> + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
> + CONSTRAINT c CHECK (i > 0));> + ]], {
> + -- <constraint-1.9>
> + 1,"Constraint C already exists"
> + -- </constraint-1.9>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.10",
> + [[
> + CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY,
> + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i));
17. 1.3 is the same.
> + ]], {
> + -- <constraint-1.10>
> + 1,"Constraint C already exists"
> + -- </constraint-1.10>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.11",
> + [[
> + CREATE TABLE t2 (i INT PRIMARY KEY,
> + CONSTRAINT c UNIQUE (i),
> + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i));
18. 1.5 is the same.
> + ]], {
> + -- <constraint-1.11>
> + 1,"Constraint C already exists"
> + -- </constraint-1.11>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.12",
> + [[
> + CREATE TABLE t2 (i INT PRIMARY KEY,
> + CONSTRAINT c CHECK (i > 0),
> + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i));
19. 1.9 is the same.
> + ]], {
> + -- <constraint-1.12>
> + 1,"Constraint C already exists"
> + -- </constraint-1.12>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-1.13",
> + [[
> + CREATE TABLE t2 (i INT PRIMARY KEY CONSTRAINT c REFERENCES t1(i),
> + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i));
> + ]], {
> + -- <constraint-1.13>
> + 1,"Constraint C already exists"
> + -- </constraint-1.13>
> + })
> +
> +--
> +-- gh-3503 Make sure that _constraint works.
20. What is _constraint?
> +--
> +test:do_execsql_test(
> + "constraint-2.1",
> + [[
> + DROP TABLE IF EXISTS t2;
> + CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);
> + ]], {
> + -- <constraint-2.1>
> + -- </constraint-2.1>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-2.2",
> + [[
> + ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);
> + ]], {
> + -- <constraint-2.2>
> + 1, "Constraint C already exists"
> + -- </constraint-2.2>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-2.3",
> + [[
> + ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);
> + ]], {
> + -- <constraint-2.3>
> + 1, "Index 'C' already exists in space 'T2'"
> + -- </constraint-2.3>
> + })
> +
> +test:do_catchsql_test(
> + "constraint-2.4",
> + [[
> + ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i);
> + ]], {
> + -- <constraint-2.4>
> + 1, "Constraint C already exists"
> + -- </constraint-2.4>
> + })
> +
> +test:do_execsql_test(
> + "constraint-2.5",
> + [[
> + ALTER TABLE t2 ADD CONSTRAINT f FOREIGN KEY(i) REFERENCES t1(i);
> + ]], {
> + -- <constraint-2.5>
> + -- </constraint-2.5>
> + })
> +
> +test:do_execsql_test(
> + "constraint-2.6",
> + [[
> + ALTER TABLE t2 DROP CONSTRAINT f;
> + ]], {
> + -- <constraint-2.6>
> + -- </constraint-2.6>
> + })
> +
> +test:do_execsql_test(
> + "constraint-2.7",
> + [[
> + ALTER TABLE t2 ADD CONSTRAINT f FOREIGN KEY(i) REFERENCES t1(i);
> + ]], {
> + -- <constraint-2.7>
> + -- </constraint-2.7>
> + })
> +
> +test:finish_test()
> \ No newline at end of file
>
21. Please, append an empty line to the file.
22. You've used tap test file, but as far as I know they are
obsolete for SQL. It is better to use normal tarantool tests
(sql/*.test.lua) or SQL tests (sql/*.test.sql). But I may be
wrong here.
next prev parent reply other threads:[~2019-11-12 22:58 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-12 14:02 [Tarantool-patches] [PATCH 0/2] Add constraint names hash table to space Roman Khabibov
2019-11-12 14:02 ` [Tarantool-patches] [PATCH 1/2] box: introduce constraint names hash table Roman Khabibov
2019-11-12 14:17 ` Cyrill Gorcunov
2019-11-12 22:30 ` Roman Khabibov
2019-11-12 23:04 ` Vladislav Shpilevoy
2019-11-12 14:02 ` [Tarantool-patches] [PATCH 2/2] sql: make constraint operations transactional Roman Khabibov
2019-11-12 23:04 ` Vladislav Shpilevoy [this message]
2019-11-12 23:04 ` [Tarantool-patches] [PATCH 0/2] Add constraint names hash table to space 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=ccca11aa-efa5-6a0c-9473-91fec1f28151@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=roman.habibov@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH 2/2] 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