Tarantool development patches archive
 help / color / mirror / Atom feed
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.

  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