[PATCH 00/10] Prepare box/alter.cc for transactional DDL

[PATCH 00/10] Prepare box/alter.cc for transactional DDL

[Vladimir Davydov]

A set of pretty straightforward patches that prepare system space
triggers for transactional DDL, namely:

 - Make sure that in-memory schema updates are in sync with data
   updates, i.e. changes to the schema are done on replace, not on
   commit. Add tests whenever possible.
 - Removes txn_last_stmt from on commit/rollback, because it won't
   be available once there may be more than one DDL statement in a
   transaction.
 - Fixes some inconsistencies in sequence object rollback.

https://github.com/tarantool/tarantool/issues/4083
https://github.com/tarantool/tarantool/commits/dv/prepare-alter-for-transactional-ddl

Vladimir Davydov (10):
  ddl: unreference view on space drop synchronously
  ddl: synchronize user cache with actual data state
  ddl: synchronize func cache with actual data state
  ddl: synchronize sequence cache with actual data state
  ddl: fix _space_sequence rollback
  ddl: restore sequence value if drop is rolled back
  ddl: don't use txn_last_stmt on _collation commit/rollback
  ddl: don't use txn_last_stmt on _trigger commit/rollback
  ddl: don't use txn_last_stmt on _ck_constraint commit/rollback
  ddl: don't use txn_last_stmt on _cluster commit/rollback

 src/box/alter.cc            | 669 ++++++++++++++++++++++++++------------------
 src/box/lua/load_cfg.lua    |   1 -
 src/box/lua/schema.lua      |  48 +---
 src/box/lua/sequence.c      | 110 +++++++-
 src/box/schema.cc           |  39 +--
 src/box/schema.h            |   8 +-
 src/box/sequence.c          |  21 ++
 src/box/sequence.h          |  27 ++
 test/box/function1.result   |  34 +++
 test/box/function1.test.lua |  13 +
 test/box/sequence.result    | 117 ++++++++
 test/box/sequence.test.lua  |  44 +++
 12 files changed, 776 insertions(+), 355 deletions(-)

-- 
2.11.0

[PATCH 01/10] ddl: unreference view on space drop synchronously

[Vladimir Davydov]

Do it on_replace rather than on_commit. This is required to implement
transactional DDL.

Note, this is the only place where on_replace_dd_space() postpones
schema update until after commit. Other than that, space updates are
already transactional DDL friendly.
---
 src/box/alter.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index e76b9e68..7c4d949f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1680,7 +1680,6 @@ on_drop_view_commit(struct trigger *trigger, void *event)
 {
 	(void) event;
 	struct Select *select = (struct Select *)trigger->data;
-	update_view_references(select, -1, true, NULL);
 	sql_select_delete(sql_get(), select);
 }
 
@@ -1694,6 +1693,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event)
 {
 	(void) event;
 	struct Select *select = (struct Select *)trigger->data;
+	update_view_references(select, 1, true, NULL);
 	sql_select_delete(sql_get(), select);
 }
 
@@ -1912,6 +1912,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 				txn_alter_trigger_new(on_drop_view_rollback,
 						      select);
 			txn_on_rollback(txn, on_rollback_view);
+			update_view_references(select, -1, true, NULL);
 			select_guard.is_active = false;
 		}
 	} else { /* UPDATE, REPLACE */
-- 
2.11.0

Re: [PATCH 01/10] ddl: unreference view on space drop synchronously

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/03 22:34]:
> Do it on_replace rather than on_commit. This is required to implement
> transactional DDL.
> 

lgtm

 

-- 
Konstantin Osipov, Moscow, Russia

[PATCH 02/10] ddl: synchronize user cache with actual data state

[Vladimir Davydov]

To implement transactional DDL, we must make sure that in-memory schema
is updated synchronously with system space updates, i.e. on_replace, not
on_commit.

See also commit 22bedebe715c ("ddl: synchronize privileges cache with
actual data state").
---
 src/box/alter.cc | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 7c4d949f..cc057298 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2445,22 +2445,18 @@ user_def_new_from_tuple(struct tuple *tuple)
 }
 
 static void
-user_cache_remove_user(struct trigger * /* trigger */, void *event)
+user_cache_remove_user(struct trigger *trigger, void * /* event */)
 {
-	struct txn *txn = (struct txn *) event;
-	struct txn_stmt *stmt = txn_last_stmt(txn);
-	uint32_t uid = tuple_field_u32_xc(stmt->old_tuple ?
-				       stmt->old_tuple : stmt->new_tuple,
-				       BOX_USER_FIELD_ID);
+	struct tuple *tuple = (struct tuple *)trigger->data;
+	uint32_t uid = tuple_field_u32_xc(tuple, BOX_USER_FIELD_ID);
 	user_cache_delete(uid);
 }
 
 static void
-user_cache_alter_user(struct trigger * /* trigger */, void *event)
+user_cache_alter_user(struct trigger *trigger, void * /* event */)
 {
-	struct txn *txn = (struct txn *) event;
-	struct txn_stmt *stmt = txn_last_stmt(txn);
-	struct user_def *user = user_def_new_from_tuple(stmt->new_tuple);
+	struct tuple *tuple = (struct tuple *)trigger->data;
+	struct user_def *user = user_def_new_from_tuple(tuple);
 	auto def_guard = make_scoped_guard([=] { free(user); });
 	/* Can throw if, e.g. too many users. */
 	user_cache_replace(user);
@@ -2490,7 +2486,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 		(void) user_cache_replace(user);
 		def_guard.is_active = false;
 		struct trigger *on_rollback =
-			txn_alter_trigger_new(user_cache_remove_user, NULL);
+			txn_alter_trigger_new(user_cache_remove_user, new_tuple);
 		txn_on_rollback(txn, on_rollback);
 	} else if (new_tuple == NULL) { /* DELETE */
 		access_check_ddl(old_user->def->name, old_user->def->uid,
@@ -2510,9 +2506,10 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 			tnt_raise(ClientError, ER_DROP_USER,
 				  old_user->def->name, "the user has objects");
 		}
-		struct trigger *on_commit =
-			txn_alter_trigger_new(user_cache_remove_user, NULL);
-		txn_on_commit(txn, on_commit);
+		user_cache_delete(uid);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(user_cache_alter_user, old_tuple);
+		txn_on_rollback(txn, on_rollback);
 	} else { /* UPDATE, REPLACE */
 		assert(old_user != NULL && new_tuple != NULL);
 		/*
@@ -2524,9 +2521,11 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 		access_check_ddl(user->name, user->uid, user->uid,
 			         old_user->def->type, PRIV_A);
 		auto def_guard = make_scoped_guard([=] { free(user); });
-		struct trigger *on_commit =
-			txn_alter_trigger_new(user_cache_alter_user, NULL);
-		txn_on_commit(txn, on_commit);
+		user_cache_replace(user);
+		def_guard.is_active = false;
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(user_cache_alter_user, old_tuple);
+		txn_on_rollback(txn, on_rollback);
 	}
 }
 
-- 
2.11.0

Re: [PATCH 02/10] ddl: synchronize user cache with actual data state

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/03 22:34]:
> To implement transactional DDL, we must make sure that in-memory schema
> is updated synchronously with system space updates, i.e. on_replace, not
> on_commit.
> 
> See also commit 22bedebe715c ("ddl: synchronize privileges cache with
> actual data state").

Here's a problem with replacing before commit:

Imagine you have a yielding transaction, which is in progress.

You replace state in the metadata cache, it doesn't matter what
cache - user, view, space, trigger, or whatever.

Then you yield on DDL.

The yielding transaction kicks-in and does some work, seeing the
dirty state. But doesn't commit yet.

Then you roll back. 

Your yielding transaction isolation is violated.

Despite this gap I think we should proceed along this path. But we
also need to make sure all caches are multi-versioned: when you
replace a value in the cache, only a new transaction should see
the new value, the old one shouldn't. 

This looks fairly straightforward to implement, by adding a
schema_version to each cacheable schema object and adding
schema_version to struct txn as well. 

Now that we have killed implicit transaction start and separated
txn from fiber, we can start txn whenever we want and assign it a
schema version.

So the patch is basically LGTM, but please be aware that moving 
the changes in the cache from on_commit to on_replace breaks
yielding transaction isolation.

I just asked myself, why did I do it this way in the first place,
and recalled the above argument.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 02/10] ddl: synchronize user cache with actual data state

[Vladimir Davydov]

On Wed, Jul 03, 2019 at 10:43:22PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/03 22:34]:
> > To implement transactional DDL, we must make sure that in-memory schema
> > is updated synchronously with system space updates, i.e. on_replace, not
> > on_commit.
> > 
> > See also commit 22bedebe715c ("ddl: synchronize privileges cache with
> > actual data state").
> 
> Here's a problem with replacing before commit:
> 
> Imagine you have a yielding transaction, which is in progress.
> 
> You replace state in the metadata cache, it doesn't matter what
> cache - user, view, space, trigger, or whatever.
> 
> Then you yield on DDL.

If we yield from a DDL transaction, we must invoke on_rollback triggers
immediately. I don't see any problem with that. If we don't do that now,
I'll change that soon enough.

> 
> The yielding transaction kicks-in and does some work, seeing the
> dirty state. But doesn't commit yet.

If on_rollback triggers are run right upon a yield, nobody will see
a dirty state.

> 
> Then you roll back. 
> 
> Your yielding transaction isolation is violated.
> 
> Despite this gap I think we should proceed along this path. But we
> also need to make sure all caches are multi-versioned: when you
> replace a value in the cache, only a new transaction should see
> the new value, the old one shouldn't. 
> 
> This looks fairly straightforward to implement, by adding a
> schema_version to each cacheable schema object and adding
> schema_version to struct txn as well. 
> 
> Now that we have killed implicit transaction start and separated
> txn from fiber, we can start txn whenever we want and assign it a
> schema version.
> 
> So the patch is basically LGTM, but please be aware that moving 
> the changes in the cache from on_commit to on_replace breaks
> yielding transaction isolation.
> 
> I just asked myself, why did I do it this way in the first place,
> and recalled the above argument.

[tarantool-patches] Re: [PATCH 02/10] ddl: synchronize user cache with actual data state

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/03 23:05]:
> > > See also commit 22bedebe715c ("ddl: synchronize privileges cache with
> > > actual data state").
> > 
> > Here's a problem with replacing before commit:
> > 
> > Imagine you have a yielding transaction, which is in progress.
> > 
> > You replace state in the metadata cache, it doesn't matter what
> > cache - user, view, space, trigger, or whatever.
> > 
> > Then you yield on DDL.
> 
> If we yield from a DDL transaction, we must invoke on_rollback triggers
> immediately. I don't see any problem with that. If we don't do that now,
> I'll change that soon enough.

any transaction yields on WAL write. With synchronous replication
it will also involve a network round trip to get a quorum
responses - so a failure is more likely to happen. And then we
have a cascading rollback.


-- 
Konstantin Osipov, Moscow, Russia

[PATCH 03/10] ddl: synchronize func cache with actual data state

[Vladimir Davydov]

To implement transactional DDL, we must make sure that in-memory schema
is updated synchronously with system space updates, i.e. on_replace, not
on_commit.
---
 src/box/alter.cc            | 40 +++++++++++++++++++++++++++++++---------
 test/box/function1.result   | 34 ++++++++++++++++++++++++++++++++++
 test/box/function1.test.lua | 13 +++++++++++++
 3 files changed, 78 insertions(+), 9 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index cc057298..062cf9f8 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2587,14 +2587,31 @@ func_def_new_from_tuple(struct tuple *tuple)
 	return def;
 }
 
-/** Remove a function from function cache */
 static void
-func_cache_remove_func(struct trigger *trigger, void * /* event */)
+on_create_func_rollback(struct trigger *trigger, void * /* event */)
 {
-	struct func *old_func = (struct func *) trigger->data;
-	func_cache_delete(old_func->def->fid);
-	trigger_run_xc(&on_alter_func, old_func);
-	func_delete(old_func);
+	/* Remove the new function from the cache and delete it. */
+	struct func *func = (struct func *)trigger->data;
+	func_cache_delete(func->def->fid);
+	trigger_run_xc(&on_alter_func, func);
+	func_delete(func);
+}
+
+static void
+on_drop_func_commit(struct trigger *trigger, void * /* event */)
+{
+	/* Delete the old function. */
+	struct func *func = (struct func *)trigger->data;
+	func_delete(func);
+}
+
+static void
+on_drop_func_rollback(struct trigger *trigger, void * /* event */)
+{
+	/* Insert the old function back into the cache. */
+	struct func *func = (struct func *)trigger->data;
+	func_cache_insert(func);
+	trigger_run_xc(&on_alter_func, func);
 }
 
 /**
@@ -2619,15 +2636,15 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 		access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION,
 				 PRIV_C);
 		struct trigger *on_rollback =
-			txn_alter_trigger_new(func_cache_remove_func, NULL);
+			txn_alter_trigger_new(on_create_func_rollback, NULL);
 		struct func *func = func_new(def);
 		if (func == NULL)
 			diag_raise();
 		def_guard.is_active = false;
 		func_cache_insert(func);
 		on_rollback->data = func;
+		txn_on_rollback(txn, on_rollback);
 		trigger_run_xc(&on_alter_func, func);
-		txn_on_rollback(txn, on_rollback);
 	} else if (new_tuple == NULL) {         /* DELETE */
 		uint32_t uid;
 		func_def_get_ids_from_tuple(old_tuple, &fid, &uid);
@@ -2644,8 +2661,13 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 				  "function has grants");
 		}
 		struct trigger *on_commit =
-			txn_alter_trigger_new(func_cache_remove_func, old_func);
+			txn_alter_trigger_new(on_drop_func_commit, old_func);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_drop_func_rollback, old_func);
+		func_cache_delete(old_func->def->fid);
 		txn_on_commit(txn, on_commit);
+		txn_on_rollback(txn, on_rollback);
+		trigger_run_xc(&on_alter_func, old_func);
 	} else {                                /* UPDATE, REPLACE */
 		assert(new_tuple != NULL && old_tuple != NULL);
 		/**
diff --git a/test/box/function1.result b/test/box/function1.result
index 439e19e9..ec1ab5e6 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -530,3 +530,37 @@ test_run:cmd("clear filter")
 ---
 - true
 ...
+--
+-- Check that function cache is updated synchronously with _func changes.
+--
+box.begin() box.schema.func.create('test') f = box.func.test box.rollback()
+---
+...
+f ~= nil
+---
+- true
+...
+box.func.test == nil
+---
+- true
+...
+box.schema.func.create('test')
+---
+...
+f = box.func.test
+---
+...
+box.begin() box.space._func:delete{f.id} f = box.func.test box.rollback()
+---
+...
+f == nil
+---
+- true
+...
+box.func.test ~= nil
+---
+- true
+...
+box.func.test:drop()
+---
+...
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index 4ec9c04a..a891e192 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -185,3 +185,16 @@ box.schema.func.drop('secret_leak')
 box.schema.func.drop('secret')
 
 test_run:cmd("clear filter")
+
+--
+-- Check that function cache is updated synchronously with _func changes.
+--
+box.begin() box.schema.func.create('test') f = box.func.test box.rollback()
+f ~= nil
+box.func.test == nil
+box.schema.func.create('test')
+f = box.func.test
+box.begin() box.space._func:delete{f.id} f = box.func.test box.rollback()
+f == nil
+box.func.test ~= nil
+box.func.test:drop()
-- 
2.11.0

Re: [PATCH 03/10] ddl: synchronize func cache with actual data state

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/03 22:34]:
> To implement transactional DDL, we must make sure that in-memory schema
> is updated synchronously with system space updates, i.e. on_replace, not
> on_commit.

lgtm (the isolation gap we're discussing in a parallel thread is
already there for other objects, so let's make all object
update/replace consistent first and then address the isolation gap).

-- 
Konstantin Osipov, Moscow, Russia

[PATCH 04/10] ddl: synchronize sequence cache with actual data state

[Vladimir Davydov]

To implement transactional DDL, we must make sure that in-memory schema
is updated synchronously with system space updates, i.e. on_replace, not
on_commit.

Note, to do this in case of the sequence cache, we have to rework the
way sequences are exported to Lua - make on_alter_sequence similar to
how on_alter_space and on_alter_func triggers are implemented.
---
 src/box/alter.cc           | 114 ++++++++++++++++++++++++---------------------
 src/box/lua/load_cfg.lua   |   1 -
 src/box/lua/schema.lua     |  48 +++----------------
 src/box/lua/sequence.c     | 110 +++++++++++++++++++++++++++++++++++++------
 src/box/schema.cc          |  39 +++++-----------
 src/box/schema.h           |   8 ++--
 src/box/sequence.c         |  21 +++++++++
 src/box/sequence.h         |  27 +++++++++++
 test/box/sequence.result   |  42 +++++++++++++++++
 test/box/sequence.test.lua |  15 ++++++
 10 files changed, 282 insertions(+), 143 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 062cf9f8..7aeed834 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3337,50 +3337,52 @@ sequence_def_new_from_tuple(struct tuple *tuple, uint32_t errcode)
 	return def;
 }
 
-/** Argument passed to on_commit_dd_sequence() trigger. */
-struct alter_sequence {
-	/** Trigger invoked on commit in the _sequence space. */
-	struct trigger on_commit;
-	/** Trigger invoked on rollback in the _sequence space. */
-	struct trigger on_rollback;
-	/** Old sequence definition or NULL if create. */
-	struct sequence_def *old_def;
-	/** New sequence defitition or NULL if drop. */
-	struct sequence_def *new_def;
-};
-
-/**
- * Trigger invoked on commit in the _sequence space.
- */
 static void
-on_commit_dd_sequence(struct trigger *trigger, void *event)
+on_create_sequence_rollback(struct trigger *trigger, void * /* event */)
 {
-	struct txn *txn = (struct txn *) event;
-	struct alter_sequence *alter = (struct alter_sequence *) trigger->data;
+	/* Remove the new sequence from the cache and delete it. */
+	struct sequence *seq = (struct sequence *)trigger->data;
+	sequence_cache_delete(seq->def->id);
+	trigger_run_xc(&on_alter_sequence, seq);
+	sequence_delete(seq);
+}
 
-	if (alter->new_def != NULL && alter->old_def != NULL) {
-		/* Alter a sequence. */
-		sequence_cache_replace(alter->new_def);
-	} else if (alter->new_def == NULL) {
-		/* Drop a sequence. */
-		sequence_cache_delete(alter->old_def->id);
-	}
+static void
+on_drop_sequence_commit(struct trigger *trigger, void * /* event */)
+{
+	/* Delete the old sequence. */
+	struct sequence *seq = (struct sequence *)trigger->data;
+	sequence_delete(seq);
+}
 
-	trigger_run_xc(&on_alter_sequence, txn_last_stmt(txn));
+static void
+on_drop_sequence_rollback(struct trigger *trigger, void * /* event */)
+{
+	/* Insert the old sequence back into the cache. */
+	struct sequence *seq = (struct sequence *)trigger->data;
+	sequence_cache_insert(seq);
+	trigger_run_xc(&on_alter_sequence, seq);
 }
 
-/**
- * Trigger invoked on rollback in the _sequence space.
- */
+
 static void
-on_rollback_dd_sequence(struct trigger *trigger, void * /* event */)
+on_alter_sequence_commit(struct trigger *trigger, void * /* event */)
 {
-	struct alter_sequence *alter = (struct alter_sequence *) trigger->data;
+	/* Delete the old old sequence definition. */
+	struct sequence_def *def = (struct sequence_def *)trigger->data;
+	free(def);
+}
 
-	if (alter->new_def != NULL && alter->old_def == NULL) {
-		/* Rollback creation of a sequence. */
-		sequence_cache_delete(alter->new_def->id);
-	}
+static void
+on_alter_sequence_rollback(struct trigger *trigger, void * /* event */)
+{
+	/* Restore the old sequence definition. */
+	struct sequence_def *def = (struct sequence_def *)trigger->data;
+	struct sequence *seq = sequence_by_id(def->id);
+	assert(seq != NULL);
+	free(seq->def);
+	seq->def = def;
+	trigger_run_xc(&on_alter_sequence, seq);
 }
 
 /**
@@ -3396,24 +3398,25 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
 
-	struct alter_sequence *alter =
-		region_calloc_object_xc(&txn->region, struct alter_sequence);
-
 	struct sequence_def *new_def = NULL;
 	auto def_guard = make_scoped_guard([=] { free(new_def); });
 
+	struct sequence *seq;
 	if (old_tuple == NULL && new_tuple != NULL) {		/* INSERT */
 		new_def = sequence_def_new_from_tuple(new_tuple,
 						      ER_CREATE_SEQUENCE);
-		assert(sequence_by_id(new_def->id) == NULL);
 		access_check_ddl(new_def->name, new_def->id, new_def->uid,
 				 SC_SEQUENCE, PRIV_C);
-		sequence_cache_replace(new_def);
-		alter->new_def = new_def;
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_create_sequence_rollback, NULL);
+		seq = sequence_new_xc(new_def);
+		sequence_cache_insert(seq);
+		on_rollback->data = seq;
+		txn_on_rollback(txn, on_rollback);
 	} else if (old_tuple != NULL && new_tuple == NULL) {	/* DELETE */
 		uint32_t id = tuple_field_u32_xc(old_tuple,
 						 BOX_SEQUENCE_DATA_FIELD_ID);
-		struct sequence *seq = sequence_by_id(id);
+		seq = sequence_by_id(id);
 		assert(seq != NULL);
 		access_check_ddl(seq->def->name, seq->def->id, seq->def->uid,
 				 SC_SEQUENCE, PRIV_D);
@@ -3426,26 +3429,31 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 		if (schema_find_grants("sequence", seq->def->id))
 			tnt_raise(ClientError, ER_DROP_SEQUENCE,
 				  seq->def->name, "the sequence has grants");
-		alter->old_def = seq->def;
+		struct trigger *on_commit =
+			txn_alter_trigger_new(on_drop_sequence_commit, seq);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_drop_sequence_rollback, seq);
+		sequence_cache_delete(seq->def->id);
+		txn_on_commit(txn, on_commit);
+		txn_on_rollback(txn, on_rollback);
 	} else {						/* UPDATE */
 		new_def = sequence_def_new_from_tuple(new_tuple,
 						      ER_ALTER_SEQUENCE);
-		struct sequence *seq = sequence_by_id(new_def->id);
+		seq = sequence_by_id(new_def->id);
 		assert(seq != NULL);
 		access_check_ddl(seq->def->name, seq->def->id, seq->def->uid,
 				 SC_SEQUENCE, PRIV_A);
-		alter->old_def = seq->def;
-		alter->new_def = new_def;
+		struct trigger *on_commit =
+			txn_alter_trigger_new(on_alter_sequence_commit, seq->def);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_alter_sequence_rollback, seq->def);
+		seq->def = new_def;
+		txn_on_commit(txn, on_commit);
+		txn_on_rollback(txn, on_rollback);
 	}
 
 	def_guard.is_active = false;
-
-	trigger_create(&alter->on_commit,
-		       on_commit_dd_sequence, alter, NULL);
-	txn_on_commit(txn, &alter->on_commit);
-	trigger_create(&alter->on_rollback,
-		       on_rollback_dd_sequence, alter, NULL);
-	txn_on_rollback(txn, &alter->on_rollback);
+	trigger_run_xc(&on_alter_sequence, seq);
 }
 
 /**
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 9f3344da..d1be0f39 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -467,7 +467,6 @@ setmetatable(box, {
 })
 
 local function load_cfg(cfg)
-    box.internal.schema.init()
     cfg = upgrade_cfg(cfg, translate_cfg)
     cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
     apply_default_cfg(cfg, default_cfg);
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 9c3ee063..084addc2 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1806,42 +1806,11 @@ sequence_mt.drop = function(self)
     box.schema.sequence.drop(self.id)
 end
 
-local function sequence_tuple_decode(seq, tuple)
-    seq.id, seq.uid, seq.name, seq.step, seq.min, seq.max,
-        seq.start, seq.cache, seq.cycle = tuple:unpack()
-end
-
-local function sequence_new(tuple)
-    local seq = setmetatable({}, sequence_mt)
-    sequence_tuple_decode(seq, tuple)
-    return seq
-end
-
-local function sequence_on_alter(old_tuple, new_tuple)
-    if old_tuple and not new_tuple then
-        local old_name = old_tuple.name
-        box.sequence[old_name] = nil
-    elseif not old_tuple and new_tuple then
-        local seq = sequence_new(new_tuple)
-        box.sequence[seq.name] = seq
-    else
-        local old_name = old_tuple.name
-        local seq = box.sequence[old_name]
-        if not seq then
-            seq = sequence_new(seq, new_tuple)
-        else
-            sequence_tuple_decode(seq, new_tuple)
-        end
-        box.sequence[old_name] = nil
-        box.sequence[seq.name] = seq
-    end
-end
-
 box.sequence = {}
-local function box_sequence_init()
-    -- Install a trigger that will update Lua objects on
-    -- _sequence space modifications.
-    internal.sequence.on_alter(sequence_on_alter)
+box.schema.sequence = {}
+
+function box.schema.sequence.bless(seq)
+    setmetatable(seq, {__index = sequence_mt})
 end
 
 local sequence_options = {
@@ -1859,7 +1828,6 @@ create_sequence_options.if_not_exists = 'boolean'
 local alter_sequence_options = table.deepcopy(sequence_options)
 alter_sequence_options.name = 'string'
 
-box.schema.sequence = {}
 box.schema.sequence.create = function(name, opts)
     opts = opts or {}
     check_param(name, 'name', 'string')
@@ -1897,7 +1865,8 @@ box.schema.sequence.alter = function(name, opts)
         return
     end
     local seq = {}
-    sequence_tuple_decode(seq, tuple)
+    seq.id, seq.uid, seq.name, seq.step, seq.min, seq.max,
+        seq.start, seq.cache, seq.cycle = tuple:unpack()
     opts = update_param_table(opts, seq)
     local _sequence = box.space[box.schema.SEQUENCE_ID]
     _sequence:replace{seq.id, seq.uid, opts.name, opts.step, opts.min,
@@ -2689,11 +2658,6 @@ end
 
 setmetatable(box.space, { __serialize = box_space_mt })
 
-box.internal.schema = {}
-box.internal.schema.init = function()
-    box_sequence_init()
-end
-
 box.feedback = {}
 box.feedback.save = function(file_name)
     if type(file_name) ~= "string" then
diff --git a/src/box/lua/sequence.c b/src/box/lua/sequence.c
index 2fead2eb..bd9ec758 100644
--- a/src/box/lua/sequence.c
+++ b/src/box/lua/sequence.c
@@ -31,11 +31,11 @@
 #include "box/lua/sequence.h"
 #include "box/lua/tuple.h"
 #include "lua/utils.h"
-#include "lua/trigger.h"
 
 #include "diag.h"
 #include "box/box.h"
 #include "box/schema.h"
+#include "box/sequence.h"
 #include "box/txn.h"
 
 static int
@@ -68,28 +68,104 @@ lbox_sequence_reset(struct lua_State *L)
 	return 0;
 }
 
-static int
-lbox_sequence_push_on_alter_event(struct lua_State *L, void *event)
+static void
+lbox_sequence_new(struct lua_State *L, struct sequence *seq)
 {
-	struct txn_stmt *stmt = (struct txn_stmt *) event;
-	if (stmt->old_tuple) {
-		luaT_pushtuple(L, stmt->old_tuple);
+	lua_getfield(L, LUA_GLOBALSINDEX, "box");
+	lua_getfield(L, -1, "sequence");
+	lua_rawgeti(L, -1, seq->def->id);
+	if (lua_isnil(L, -1)) {
+		/*
+		 * If the sequence already exists, modify it,
+		 * rather than create a new one -- to not
+		 * invalidate Lua variable references to old
+		 * sequence outside the box.schema.sequence[].
+		 */
+		lua_pop(L, 1);
+		lua_newtable(L);
+		lua_rawseti(L, -2, seq->def->id);
+		lua_rawgeti(L, -1, seq->def->id);
 	} else {
+		/* Clear the reference to old sequence by old name. */
+		lua_getfield(L, -1, "name");
 		lua_pushnil(L);
+		lua_settable(L, -4);
 	}
-	if (stmt->new_tuple) {
-		luaT_pushtuple(L, stmt->new_tuple);
-	} else {
+	int top = lua_gettop(L);
+	lua_pushstring(L, "id");
+	lua_pushnumber(L, seq->def->id);
+	lua_settable(L, top);
+	lua_pushstring(L, "uid");
+	lua_pushnumber(L, seq->def->uid);
+	lua_settable(L, top);
+	lua_pushstring(L, "name");
+	lua_pushstring(L, seq->def->name);
+	lua_settable(L, top);
+	lua_pushstring(L, "step");
+	luaL_pushint64(L, seq->def->step);
+	lua_settable(L, top);
+	lua_pushstring(L, "min");
+	luaL_pushint64(L, seq->def->min);
+	lua_settable(L, top);
+	lua_pushstring(L, "max");
+	luaL_pushint64(L, seq->def->max);
+	lua_settable(L, top);
+	lua_pushstring(L, "start");
+	luaL_pushint64(L, seq->def->start);
+	lua_settable(L, top);
+	lua_pushstring(L, "cache");
+	luaL_pushint64(L, seq->def->cache);
+	lua_settable(L, top);
+	lua_pushstring(L, "cycle");
+	lua_pushboolean(L, seq->def->cycle);
+	lua_settable(L, top);
+
+	/* Bless sequence object. */
+	lua_getfield(L, LUA_GLOBALSINDEX, "box");
+	lua_pushstring(L, "schema");
+	lua_gettable(L, -2);
+	lua_pushstring(L, "sequence");
+	lua_gettable(L, -2);
+	lua_pushstring(L, "bless");
+	lua_gettable(L, -2);
+
+	lua_pushvalue(L, top);
+	lua_call(L, 1, 0);
+	lua_pop(L, 3);
+
+	lua_setfield(L, -2, seq->def->name);
+
+	lua_pop(L, 2);
+}
+
+static void
+lbox_sequence_delete(struct lua_State *L, struct sequence *seq)
+{
+	lua_getfield(L, LUA_GLOBALSINDEX, "box");
+	lua_getfield(L, -1, "sequence");
+	lua_rawgeti(L, -1, seq->def->id);
+	if (!lua_isnil(L, -1)) {
+		lua_getfield(L, -1, "name");
 		lua_pushnil(L);
+		lua_rawset(L, -4);
+		lua_pop(L, 1); /* pop sequence */
+		lua_pushnil(L);
+		lua_rawseti(L, -2, seq->def->id);
+	} else {
+		lua_pop(L, 1);
 	}
-	return 2;
+	lua_pop(L, 2); /* box, sequence */
 }
 
-static int
-lbox_sequence_on_alter(struct lua_State *L)
+static void
+lbox_sequence_new_or_delete(struct trigger *trigger, void *event)
 {
-	return lbox_trigger_reset(L, 2, &on_alter_sequence,
-				  lbox_sequence_push_on_alter_event, NULL);
+	struct lua_State *L = trigger->data;
+	struct sequence *seq = event;
+	if (sequence_by_id(seq->def->id) != NULL)
+		lbox_sequence_new(L, seq);
+	else
+		lbox_sequence_delete(L, seq);
 }
 
 void
@@ -99,9 +175,13 @@ box_lua_sequence_init(struct lua_State *L)
 		{"next", lbox_sequence_next},
 		{"set", lbox_sequence_set},
 		{"reset", lbox_sequence_reset},
-		{"on_alter", lbox_sequence_on_alter},
 		{NULL, NULL}
 	};
 	luaL_register(L, "box.internal.sequence", sequence_internal_lib);
 	lua_pop(L, 1);
+
+	static struct trigger on_alter_sequence_in_lua;
+	trigger_create(&on_alter_sequence_in_lua,
+		       lbox_sequence_new_or_delete, L, NULL);
+	trigger_add(&on_alter_sequence, &on_alter_sequence_in_lua);
 }
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 64412fac..3f90b8d4 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -635,41 +635,24 @@ sequence_cache_find(uint32_t id)
 }
 
 void
-sequence_cache_replace(struct sequence_def *def)
+sequence_cache_insert(struct sequence *seq)
 {
-	struct sequence *seq = sequence_by_id(def->id);
-	if (seq == NULL) {
-		/* Create a new sequence. */
-		seq = (struct sequence *) calloc(1, sizeof(*seq));
-		if (seq == NULL)
-			goto error;
-		struct mh_i32ptr_node_t node = { def->id, seq };
-		if (mh_i32ptr_put(sequences, &node, NULL, NULL) ==
-		    mh_end(sequences))
-			goto error;
-	} else {
-		/* Update an existing sequence. */
-		free(seq->def);
+	assert(sequence_by_id(seq->def->id) == NULL);
+
+	struct mh_i32ptr_node_t node = { seq->def->id, seq };
+	mh_int_t k = mh_i32ptr_put(sequences, &node, NULL, NULL);
+	if (k == mh_end(sequences)) {
+		panic_syserror("Out of memory for the data "
+			       "dictionary cache (sequence).");
 	}
-	seq->def = def;
-	return;
-error:
-	panic_syserror("Out of memory for the data "
-		       "dictionary cache (sequence).");
 }
 
 void
 sequence_cache_delete(uint32_t id)
 {
-	struct sequence *seq = sequence_by_id(id);
-	if (seq != NULL) {
-		/* Delete sequence data. */
-		sequence_reset(seq);
-		mh_i32ptr_del(sequences, seq->def->id, NULL);
-		free(seq->def);
-		TRASH(seq);
-		free(seq);
-	}
+	mh_int_t k = mh_i32ptr_find(sequences, id, NULL);
+	if (k != mh_end(sequences))
+		mh_i32ptr_del(sequences, k, NULL);
 }
 
 const char *
diff --git a/src/box/schema.h b/src/box/schema.h
index 30366382..84f0d33f 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -205,12 +205,12 @@ struct sequence *
 sequence_cache_find(uint32_t id);
 
 /**
- * Insert a new sequence object into the cache or update
- * an existing one if there's already a sequence with
- * the given id in the cache.
+ * Insert a new sequence object into the cache.
+ * There must not be a sequence with the same id
+ * in the cache.
  */
 void
-sequence_cache_replace(struct sequence_def *def);
+sequence_cache_insert(struct sequence *seq);
 
 /** Delete a sequence from the sequence cache. */
 void
diff --git a/src/box/sequence.c b/src/box/sequence.c
index c9828c0d..2bf38f99 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -127,6 +127,27 @@ sequence_free(void)
 	mempool_destroy(&sequence_data_extent_pool);
 }
 
+struct sequence *
+sequence_new(struct sequence_def *def)
+{
+	struct sequence *seq = calloc(1, sizeof(*seq));
+	if (seq == NULL) {
+		diag_set(OutOfMemory, sizeof(*seq), "malloc", "sequence");
+		return NULL;
+	}
+	seq->def = def;
+	return seq;
+}
+
+void
+sequence_delete(struct sequence *seq)
+{
+	/* Delete sequence data. */
+	sequence_reset(seq);
+	free(seq->def);
+	free(seq);
+}
+
 void
 sequence_reset(struct sequence *seq)
 {
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 44194279..9a745ad5 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -36,6 +36,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include "diag.h"
 #include "user_def.h"
 
 #if defined(__cplusplus)
@@ -99,6 +100,22 @@ sequence_init(void);
 void
 sequence_free(void);
 
+/**
+ * Create a new sequence object with the given definition.
+ * Note, on success the sequence definition is assigned to
+ * the new sequence and will be freed automatically when
+ * the sequence is destroyed so it must be allocated with
+ * malloc().
+ */
+struct sequence *
+sequence_new(struct sequence_def *def);
+
+/**
+ * Destroy a sequence and its definition.
+ */
+void
+sequence_delete(struct sequence *seq);
+
 /** Reset a sequence. */
 void
 sequence_reset(struct sequence *seq);
@@ -155,6 +172,16 @@ sequence_data_iterator_create(void);
 
 #if defined(__cplusplus)
 } /* extern "C" */
+
+static inline struct sequence *
+sequence_new_xc(struct sequence_def *def)
+{
+	struct sequence *seq = sequence_new(def);
+	if (seq == NULL)
+		diag_raise();
+	return seq;
+}
+
 #endif /* defined(__cplusplus) */
 
 #endif /* INCLUDES_TARANTOOL_BOX_SEQUENCE_H */
diff --git a/test/box/sequence.result b/test/box/sequence.result
index 990d15db..2c0c0a96 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -2153,3 +2153,45 @@ s:insert{{a = 3, b = box.NULL}}
 s:drop()
 ---
 ...
+--
+-- Check that sequence cache is updated synchronously with _sequence changes.
+--
+box.begin() box.schema.sequence.create('test') sq = box.sequence.test box.rollback()
+---
+...
+sq ~= nil
+---
+- true
+...
+box.sequence.test == nil
+---
+- true
+...
+sq = box.schema.sequence.create('test')
+---
+...
+box.begin() sq:alter{step = 10} step = sq.step box.rollback()
+---
+...
+step -- 10
+---
+- 10
+...
+sq.step -- 1
+---
+- 1
+...
+box.begin() box.space._sequence:delete{sq.id} sq = box.sequence.test box.rollback()
+---
+...
+sq == nil
+---
+- true
+...
+box.sequence.test ~= nil
+---
+- true
+...
+box.sequence.test:drop()
+---
+...
diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua
index 3375572e..3c8140e8 100644
--- a/test/box/sequence.test.lua
+++ b/test/box/sequence.test.lua
@@ -727,3 +727,18 @@ pk:alter{parts = {{'y.b', 'unsigned'}}, sequence = {field = 'y.a'}} -- error
 pk:alter{parts = {{'y.b', 'unsigned'}}, sequence = {field = 'y.b'}} -- ok
 s:insert{{a = 3, b = box.NULL}}
 s:drop()
+
+--
+-- Check that sequence cache is updated synchronously with _sequence changes.
+--
+box.begin() box.schema.sequence.create('test') sq = box.sequence.test box.rollback()
+sq ~= nil
+box.sequence.test == nil
+sq = box.schema.sequence.create('test')
+box.begin() sq:alter{step = 10} step = sq.step box.rollback()
+step -- 10
+sq.step -- 1
+box.begin() box.space._sequence:delete{sq.id} sq = box.sequence.test box.rollback()
+sq == nil
+box.sequence.test ~= nil
+box.sequence.test:drop()
-- 
2.11.0

Re: [PATCH 04/10] ddl: synchronize sequence cache with actual data state

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/03 22:34]:
> To implement transactional DDL, we must make sure that in-memory schema
> is updated synchronously with system space updates, i.e. on_replace, not
> on_commit.
> 
> Note, to do this in case of the sequence cache, we have to rework the
> way sequences are exported to Lua - make on_alter_sequence similar to
> how on_alter_space and on_alter_func triggers are implemented.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[PATCH 05/10] ddl: fix _space_sequence rollback

[Vladimir Davydov]

_space_sequence changes are not rolled back properly. Fix it keeping in
mind that our ultimate goal is to implement transactional DDL, which
implies that all changes to the schema should be done synchronously,
i.e. on_replace, not on_commit.
---
 src/box/alter.cc           | 138 +++++++++++++++++++++++++++++++--------------
 test/box/sequence.result   |  54 ++++++++++++++++++
 test/box/sequence.test.lua |  19 +++++++
 3 files changed, 170 insertions(+), 41 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 7aeed834..89ab9c65 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -126,7 +126,8 @@ access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid,
  */
 static void
 index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
-			 const char *sequence_path, const char *space_name)
+			 const char *sequence_path, uint32_t sequence_path_len,
+			 const char *space_name)
 {
 	struct key_def *key_def = index_def->key_def;
 	struct key_part *sequence_part = NULL;
@@ -137,7 +138,7 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
 		if ((part->path == NULL && sequence_path == NULL) ||
 		    (part->path != NULL && sequence_path != NULL &&
 		     json_path_cmp(part->path, part->path_len,
-				   sequence_path, strlen(sequence_path),
+				   sequence_path, sequence_path_len,
 				   TUPLE_INDEX_BASE) == 0)) {
 			sequence_part = part;
 			break;
@@ -302,7 +303,10 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
 	space_check_index_def_xc(space, index_def);
 	if (index_def->iid == 0 && space->sequence != NULL)
 		index_def_check_sequence(index_def, space->sequence_fieldno,
-					 space->sequence_path, space_name(space));
+					 space->sequence_path,
+					 space->sequence_path != NULL ?
+					 strlen(space->sequence_path) : 0,
+					 space_name(space));
 	index_def_guard.is_active = false;
 	return index_def;
 }
@@ -3484,12 +3488,83 @@ on_replace_dd_sequence_data(struct trigger * /* trigger */, void *event)
 }
 
 /**
- * Run the triggers registered on commit of a change in _space.
+ * Extract field number and path from _space_sequence tuple.
+ * The path is allocated using malloc().
  */
+static uint32_t
+sequence_field_from_tuple(struct space *space, struct tuple *tuple,
+			  char **path_ptr)
+{
+	struct index *pk = index_find_xc(space, 0);
+	struct key_part *part = &pk->def->key_def->parts[0];
+	uint32_t fieldno = part->fieldno;
+	const char *path_raw = part->path;
+	uint32_t path_len = part->path_len;
+
+	/* Sequence field was added in 2.2.1. */
+	if (tuple_field_count(tuple) > BOX_SPACE_SEQUENCE_FIELD_FIELDNO) {
+		fieldno = tuple_field_u32_xc(tuple,
+				BOX_SPACE_SEQUENCE_FIELD_FIELDNO);
+		path_raw = tuple_field_str_xc(tuple,
+				BOX_SPACE_SEQUENCE_FIELD_PATH, &path_len);
+		if (path_len == 0)
+			path_raw = NULL;
+	}
+	index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
+				 space_name(space));
+	char *path = NULL;
+	if (path_raw != NULL) {
+		path = (char *)malloc(path_len + 1);
+		if (path == NULL)
+			tnt_raise(OutOfMemory, path_len + 1,
+				  "malloc", "sequence path");
+		memcpy(path, path_raw, path_len);
+		path[path_len] = 0;
+	}
+	*path_ptr = path;
+	return fieldno;
+}
+
+/** Attach a sequence to a space on rollback in _space_sequence. */
+static void
+set_space_sequence(struct trigger *trigger, void * /* event */)
+{
+	struct tuple *tuple = (struct tuple *)trigger->data;
+	uint32_t space_id = tuple_field_u32_xc(tuple,
+			BOX_SPACE_SEQUENCE_FIELD_ID);
+	uint32_t sequence_id = tuple_field_u32_xc(tuple,
+			BOX_SPACE_SEQUENCE_FIELD_SEQUENCE_ID);
+	bool is_generated = tuple_field_bool_xc(tuple,
+			BOX_SPACE_SEQUENCE_FIELD_IS_GENERATED);
+	struct space *space = space_by_id(space_id);
+	assert(space != NULL);
+	struct sequence *seq = sequence_by_id(sequence_id);
+	assert(seq != NULL);
+	char *path;
+	uint32_t fieldno = sequence_field_from_tuple(space, tuple, &path);
+	seq->is_generated = is_generated;
+	space->sequence = seq;
+	space->sequence_fieldno = fieldno;
+	free(space->sequence_path);
+	space->sequence_path = path;
+	trigger_run_xc(&on_alter_space, space);
+}
+
+/** Detach a sequence from a space on rollback in _space_sequence. */
 static void
-on_commit_dd_space_sequence(struct trigger *trigger, void * /* event */)
+clear_space_sequence(struct trigger *trigger, void * /* event */)
 {
-	struct space *space = (struct space *) trigger->data;
+	struct tuple *tuple = (struct tuple *)trigger->data;
+	uint32_t space_id = tuple_field_u32_xc(tuple,
+			BOX_SPACE_SEQUENCE_FIELD_ID);
+	struct space *space = space_by_id(space_id);
+	assert(space != NULL);
+	assert(space->sequence != NULL);
+	space->sequence->is_generated = false;
+	space->sequence = NULL;
+	space->sequence_fieldno = 0;
+	free(space->sequence_path);
+	space->sequence_path = NULL;
 	trigger_run_xc(&on_alter_space, space);
 }
 
@@ -3537,65 +3612,46 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
 	access_check_ddl(space->def->name, space->def->id, space->def->uid,
 			 SC_SPACE, PRIV_A);
 
-	struct trigger *on_commit =
-		txn_alter_trigger_new(on_commit_dd_space_sequence, space);
-	txn_on_commit(txn, on_commit);
-
 	if (stmt->new_tuple != NULL) {			/* INSERT, UPDATE */
-		struct index *pk = index_find_xc(space, 0);
-
-		/* Sequence field was added in 2.2.1. */
+		char *sequence_path;
 		uint32_t sequence_fieldno;
-		const char *sequence_path_raw;
-		uint32_t sequence_path_len;
-		if (tuple_field_count(tuple) > BOX_SPACE_SEQUENCE_FIELD_FIELDNO) {
-			sequence_fieldno = tuple_field_u32_xc(tuple,
-					BOX_SPACE_SEQUENCE_FIELD_FIELDNO);
-			sequence_path_raw = tuple_field_str_xc(tuple,
-					BOX_SPACE_SEQUENCE_FIELD_PATH,
-					&sequence_path_len);
-		} else {
-			struct key_part *part = &pk->def->key_def->parts[0];
-			sequence_fieldno = part->fieldno;
-			sequence_path_raw = part->path;
-			sequence_path_len = part->path_len;
-		}
-		char *sequence_path = NULL;
-		if (sequence_path_len > 0) {
-			sequence_path = (char *)malloc(sequence_path_len + 1);
-			if (sequence_path == NULL)
-				tnt_raise(OutOfMemory, sequence_path_len + 1,
-					  "malloc", "sequence path");
-			memcpy(sequence_path, sequence_path_raw,
-			       sequence_path_len);
-			sequence_path[sequence_path_len] = 0;
-		}
+		sequence_fieldno = sequence_field_from_tuple(space, tuple,
+							     &sequence_path);
 		auto sequence_path_guard = make_scoped_guard([=] {
 			free(sequence_path);
 		});
-
-		index_def_check_sequence(pk->def, sequence_fieldno,
-					 sequence_path,
-					 space_name(space));
 		if (seq->is_generated) {
 			tnt_raise(ClientError, ER_ALTER_SPACE,
 				  space_name(space),
 				  "can not attach generated sequence");
 		}
+		struct trigger *on_rollback;
+		if (stmt->old_tuple != NULL)
+			on_rollback = txn_alter_trigger_new(set_space_sequence,
+							    stmt->old_tuple);
+		else
+			on_rollback = txn_alter_trigger_new(clear_space_sequence,
+							    stmt->new_tuple);
 		seq->is_generated = is_generated;
 		space->sequence = seq;
 		space->sequence_fieldno = sequence_fieldno;
 		free(space->sequence_path);
 		space->sequence_path = sequence_path;
 		sequence_path_guard.is_active = false;
+		txn_on_rollback(txn, on_rollback);
 	} else {					/* DELETE */
+		struct trigger *on_rollback;
+		on_rollback = txn_alter_trigger_new(set_space_sequence,
+						    stmt->old_tuple);
 		assert(space->sequence == seq);
 		seq->is_generated = false;
 		space->sequence = NULL;
 		space->sequence_fieldno = 0;
 		free(space->sequence_path);
 		space->sequence_path = NULL;
+		txn_on_rollback(txn, on_rollback);
 	}
+	trigger_run_xc(&on_alter_space, space);
 }
 
 /* }}} sequence */
diff --git a/test/box/sequence.result b/test/box/sequence.result
index 2c0c0a96..99eed0ec 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -2195,3 +2195,57 @@ box.sequence.test ~= nil
 box.sequence.test:drop()
 ---
 ...
+--
+-- Check that changes to _space_sequence are rolled back properly.
+--
+s = box.schema.space.create('test')
+---
+...
+_ = s:create_index('pk')
+---
+...
+sq = box.schema.sequence.create('test')
+---
+...
+box.begin() box.space._space_sequence:insert{s.id, sq.id, false, 0, ''} id = s.index.pk.sequence_id box.rollback()
+---
+...
+id == sq.id
+---
+- true
+...
+s.index.pk.sequence_id == nil
+---
+- true
+...
+s:insert{box.NULL} -- error
+---
+- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
+...
+s.index.pk:alter{sequence = sq}
+---
+...
+box.begin() box.space._space_sequence:delete{s.id} id = s.index.pk.sequence_id box.rollback()
+---
+...
+id == nil
+---
+- true
+...
+s.index.pk.sequence_id == sq.id
+---
+- true
+...
+s:insert{box.NULL} -- ok
+---
+- [1]
+...
+s.index.pk:alter{sequence = false}
+---
+...
+sq:drop()
+---
+...
+s:drop()
+---
+...
diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua
index 3c8140e8..9615c447 100644
--- a/test/box/sequence.test.lua
+++ b/test/box/sequence.test.lua
@@ -742,3 +742,22 @@ box.begin() box.space._sequence:delete{sq.id} sq = box.sequence.test box.rollbac
 sq == nil
 box.sequence.test ~= nil
 box.sequence.test:drop()
+
+--
+-- Check that changes to _space_sequence are rolled back properly.
+--
+s = box.schema.space.create('test')
+_ = s:create_index('pk')
+sq = box.schema.sequence.create('test')
+box.begin() box.space._space_sequence:insert{s.id, sq.id, false, 0, ''} id = s.index.pk.sequence_id box.rollback()
+id == sq.id
+s.index.pk.sequence_id == nil
+s:insert{box.NULL} -- error
+s.index.pk:alter{sequence = sq}
+box.begin() box.space._space_sequence:delete{s.id} id = s.index.pk.sequence_id box.rollback()
+id == nil
+s.index.pk.sequence_id == sq.id
+s:insert{box.NULL} -- ok
+s.index.pk:alter{sequence = false}
+sq:drop()
+s:drop()
-- 
2.11.0

[PATCH 06/10] ddl: restore sequence value if drop is rolled back

[Vladimir Davydov]

A sequence isn't supposed to roll back to the old value if the
transaction it was used in is aborted for some reason. However,
if a sequence is dropped, we do want to restore the original
value on rollback so that we don't lose it on an unsuccessful
attempt to drop the sequence.
---
 src/box/alter.cc           | 24 ++++++++++++++++++++++++
 test/box/sequence.result   | 21 +++++++++++++++++++++
 test/box/sequence.test.lua | 10 ++++++++++
 3 files changed, 55 insertions(+)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 89ab9c65..4f046ef6 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3460,6 +3460,20 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 	trigger_run_xc(&on_alter_sequence, seq);
 }
 
+/** Restore the old sequence value on rollback. */
+static void
+on_drop_sequence_data_rollback(struct trigger *trigger, void * /* event */)
+{
+	struct tuple *tuple = (struct tuple *)trigger->data;
+	uint32_t id = tuple_field_u32_xc(tuple, BOX_SEQUENCE_DATA_FIELD_ID);
+	int64_t val = tuple_field_i64_xc(tuple, BOX_SEQUENCE_DATA_FIELD_VALUE);
+
+	struct sequence *seq = sequence_by_id(id);
+	assert(seq != NULL);
+	if (sequence_set(seq, val) != 0)
+		panic("Can't restore sequence value");
+}
+
 /**
  * A trigger invoked on replace in space _sequence_data.
  * Used to update a sequence value.
@@ -3483,6 +3497,16 @@ on_replace_dd_sequence_data(struct trigger * /* trigger */, void *event)
 		if (sequence_set(seq, value) != 0)
 			diag_raise();
 	} else {					/* DELETE */
+		/*
+		 * A sequence isn't supposed to roll back to the old
+		 * value if the transaction it was used in is aborted
+		 * for some reason. However, if a sequence is dropped,
+		 * we do want to restore the original sequence value
+		 * on rollback.
+		 */
+		struct trigger *on_rollback = txn_alter_trigger_new(
+				on_drop_sequence_data_rollback, old_tuple);
+		txn_on_rollback(txn, on_rollback);
 		sequence_reset(seq);
 	}
 }
diff --git a/test/box/sequence.result b/test/box/sequence.result
index 99eed0ec..91795290 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -2249,3 +2249,24 @@ sq:drop()
 s:drop()
 ---
 ...
+--
+-- Check that if a deletion from _sequence_data is rolled back,
+-- the sequence state is restored.
+--
+sq = box.schema.sequence.create('test')
+---
+...
+sq:next() -- 1
+---
+- 1
+...
+box.begin() box.space._sequence_data:delete{sq.id} box.rollback()
+---
+...
+sq:next() -- 2
+---
+- 2
+...
+sq:drop()
+---
+...
diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua
index 9615c447..b0ec020e 100644
--- a/test/box/sequence.test.lua
+++ b/test/box/sequence.test.lua
@@ -761,3 +761,13 @@ s:insert{box.NULL} -- ok
 s.index.pk:alter{sequence = false}
 sq:drop()
 s:drop()
+
+--
+-- Check that if a deletion from _sequence_data is rolled back,
+-- the sequence state is restored.
+--
+sq = box.schema.sequence.create('test')
+sq:next() -- 1
+box.begin() box.space._sequence_data:delete{sq.id} box.rollback()
+sq:next() -- 2
+sq:drop()
-- 
2.11.0

[PATCH 07/10] ddl: don't use txn_last_stmt on _collation commit/rollback

[Vladimir Davydov]

When we implement transactional DDL, txn_last_stmt won't necessarily
point to the right statement on commit or rollback so we must avoid
using it.
---
 src/box/alter.cc | 51 ++++++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 4f046ef6..071258b7 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2777,43 +2777,38 @@ coll_id_def_new_from_tuple(struct tuple *tuple, struct coll_id_def *def)
 	}
 }
 
-/**
- * Rollback a change in collation space.
- * A change is only INSERT or DELETE, UPDATE is not supported.
- */
+/** Delete the new collation identifier. */
 static void
-coll_id_cache_rollback(struct trigger *trigger, void *event)
+on_create_collation_rollback(struct trigger *trigger, void *event)
 {
+	(void) event;
 	struct coll_id *coll_id = (struct coll_id *) trigger->data;
-	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
-
-	if (stmt->new_tuple == NULL) {
-		/* DELETE: put the collation identifier back. */
-		assert(stmt->old_tuple != NULL);
-		struct coll_id *replaced_id;
-		if (coll_id_cache_replace(coll_id, &replaced_id) != 0) {
-			panic("Out of memory on insertion into collation "\
-			      "cache");
-		}
-		assert(replaced_id == NULL);
-	} else {
-		/* INSERT: delete the new collation identifier. */
-		assert(stmt->old_tuple == NULL);
-		coll_id_cache_delete(coll_id);
-		coll_id_delete(coll_id);
-	}
+	coll_id_cache_delete(coll_id);
+	coll_id_delete(coll_id);
 }
 
 
 /** Free a deleted collation identifier on commit. */
 static void
-coll_id_cache_commit(struct trigger *trigger, void *event)
+on_drop_collation_commit(struct trigger *trigger, void *event)
 {
 	(void) event;
 	struct coll_id *coll_id = (struct coll_id *) trigger->data;
 	coll_id_delete(coll_id);
 }
 
+/** Put the collation identifier back on rollback. */
+static void
+on_drop_collation_rollback(struct trigger *trigger, void *event)
+{
+	(void) event;
+	struct coll_id *coll_id = (struct coll_id *) trigger->data;
+	struct coll_id *replaced_id;
+	if (coll_id_cache_replace(coll_id, &replaced_id) != 0)
+		panic("Out of memory on insertion into collation cache");
+	assert(replaced_id == NULL);
+}
+
 /**
  * A trigger invoked on replace in a space containing
  * collations that a user defined.
@@ -2826,12 +2821,12 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
 	txn_check_singlestatement_xc(txn, "Space _collation");
-	struct trigger *on_rollback =
-		txn_alter_trigger_new(coll_id_cache_rollback, NULL);
-	struct trigger *on_commit =
-		txn_alter_trigger_new(coll_id_cache_commit, NULL);
 	if (new_tuple == NULL && old_tuple != NULL) {
 		/* DELETE */
+		struct trigger *on_commit =
+			txn_alter_trigger_new(on_drop_collation_commit, NULL);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_drop_collation_rollback, NULL);
 		/*
 		 * TODO: Check that no index uses the collation
 		 * identifier.
@@ -2865,6 +2860,8 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 		txn_on_commit(txn, on_commit);
 	} else if (new_tuple != NULL && old_tuple == NULL) {
 		/* INSERT */
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_create_collation_rollback, NULL);
 		struct coll_id_def new_def;
 		coll_id_def_new_from_tuple(new_tuple, &new_def);
 		access_check_ddl(new_def.name, new_def.id, new_def.owner_id,
-- 
2.11.0

[PATCH 08/10] ddl: don't use txn_last_stmt on _trigger commit/rollback

[Vladimir Davydov]

When we implement transactional DDL, txn_last_stmt won't necessarily
point to the right statement on commit or rollback so we must avoid
using it.
---
 src/box/alter.cc | 84 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 071258b7..3ddddabb 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3720,45 +3720,50 @@ lock_before_dd(struct trigger *trigger, void *event)
 	latch_lock(&schema_lock);
 }
 
+/** Delete the new trigger on rollback of an INSERT statement. */
+static void
+on_create_trigger_rollback(struct trigger *trigger, void * /* event */)
+{
+	struct sql_trigger *old_trigger = (struct sql_trigger *)trigger->data;
+	struct sql_trigger *new_trigger;
+	int rc = sql_trigger_replace(sql_trigger_name(old_trigger),
+				     sql_trigger_space_id(old_trigger),
+				     NULL, &new_trigger);
+	(void)rc;
+	assert(rc == 0);
+	assert(new_trigger == old_trigger);
+	sql_trigger_delete(sql_get(), new_trigger);
+}
+
+/** Restore the old trigger on rollback of a DELETE statement. */
+static void
+on_drop_trigger_rollback(struct trigger *trigger, void * /* event */)
+{
+	struct sql_trigger *old_trigger = (struct sql_trigger *)trigger->data;
+	struct sql_trigger *new_trigger;
+	if (old_trigger == NULL)
+		return;
+	if (sql_trigger_replace(sql_trigger_name(old_trigger),
+				sql_trigger_space_id(old_trigger),
+				old_trigger, &new_trigger) != 0)
+		panic("Out of memory on insertion into trigger hash");
+	assert(new_trigger == NULL);
+}
+
 /**
- * Trigger invoked on rollback in the _trigger space.
- * Since rollback trigger is invoked after insertion to hash and space,
- * we have to delete it from those structures and release memory.
- * Vice versa, after deletion of trigger we must return it back to hash and space*.
+ * Restore the old trigger and delete the new trigger on rollback
+ * of a REPLACE statement.
  */
 static void
-on_replace_trigger_rollback(struct trigger *trigger, void *event)
+on_replace_trigger_rollback(struct trigger *trigger, void * /* event */)
 {
-	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
 	struct sql_trigger *old_trigger = (struct sql_trigger *)trigger->data;
 	struct sql_trigger *new_trigger;
-
-	if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
-		/* Rollback DELETE trigger. */
-		if (old_trigger == NULL)
-			return;
-		if (sql_trigger_replace(sql_trigger_name(old_trigger),
-					sql_trigger_space_id(old_trigger),
-					old_trigger, &new_trigger) != 0)
-			panic("Out of memory on insertion into trigger hash");
-		assert(new_trigger == NULL);
-	}  else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) {
-		/* Rollback INSERT trigger. */
-		int rc = sql_trigger_replace(sql_trigger_name(old_trigger),
-					     sql_trigger_space_id(old_trigger),
-					     NULL, &new_trigger);
-		(void)rc;
-		assert(rc == 0);
-		assert(new_trigger == old_trigger);
-		sql_trigger_delete(sql_get(), new_trigger);
-	} else {
-		/* Rollback REPLACE trigger. */
-		if (sql_trigger_replace(sql_trigger_name(old_trigger),
-					sql_trigger_space_id(old_trigger),
-					old_trigger, &new_trigger) != 0)
-			panic("Out of memory on insertion into trigger hash");
-		sql_trigger_delete(sql_get(), new_trigger);
-	}
+	if (sql_trigger_replace(sql_trigger_name(old_trigger),
+				sql_trigger_space_id(old_trigger),
+				old_trigger, &new_trigger) != 0)
+		panic("Out of memory on insertion into trigger hash");
+	sql_trigger_delete(sql_get(), new_trigger);
 }
 
 /**
@@ -3785,8 +3790,7 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
 
-	struct trigger *on_rollback =
-		txn_alter_trigger_new(on_replace_trigger_rollback, NULL);
+	struct trigger *on_rollback = txn_alter_trigger_new(NULL, NULL);
 	struct trigger *on_commit =
 		txn_alter_trigger_new(on_replace_trigger_commit, NULL);
 
@@ -3813,6 +3817,7 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 
 		on_commit->data = old_trigger;
 		on_rollback->data = old_trigger;
+		on_rollback->run = on_drop_trigger_rollback;
 	} else {
 		/* INSERT, REPLACE trigger. */
 		uint32_t trigger_name_len;
@@ -3860,8 +3865,13 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 			diag_raise();
 
 		on_commit->data = old_trigger;
-		on_rollback->data =
-			old_tuple == NULL ? new_trigger : old_trigger;
+		if (old_tuple != NULL) {
+			on_rollback->data = old_trigger;
+			on_rollback->run = on_replace_trigger_rollback;
+		} else {
+			on_rollback->data = new_trigger;
+			on_rollback->run = on_create_trigger_rollback;
+		}
 		new_trigger_guard.is_active = false;
 	}
 
-- 
2.11.0

[PATCH 09/10] ddl: don't use txn_last_stmt on _ck_constraint commit/rollback

[Vladimir Davydov]

When we implement transactional DDL, txn_last_stmt won't necessarily
point to the right statement on commit or rollback so we must avoid
using it.

While we are at it, let's also make sure that changes are propagated
to Lua on replace, not on commit, by moving on_alter_space trigger
invocation appropriately.
---
 src/box/alter.cc | 138 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 78 insertions(+), 60 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3ddddabb..dd66e46f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4371,76 +4371,86 @@ ck_constraint_def_new_from_tuple(struct tuple *tuple)
 	return ck_def;
 }
 
-/** Trigger invoked on rollback in the _ck_constraint space. */
+/** Rollback INSERT check constraint. */
 static void
-on_replace_ck_constraint_rollback(struct trigger *trigger, void *event)
+on_create_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
 {
-	struct txn_stmt *stmt = txn_last_stmt((struct txn *) event);
 	struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
 	assert(ck != NULL);
 	struct space *space = space_by_id(ck->def->space_id);
+	assert(space != NULL);
 	struct trigger *ck_trigger = space->ck_constraint_trigger;
-	if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
-		/* Rollback DELETE check constraint. */
-		assert(ck != NULL);
-		assert(space != NULL);
-		assert(space_ck_constraint_by_name(space,
-				ck->def->name, strlen(ck->def->name)) == NULL);
-		rlist_add_entry(&space->ck_constraint, ck, link);
-		if (rlist_empty(&ck_trigger->link))
-			trigger_add(&space->on_replace, ck_trigger);
-	}  else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) {
-		/* Rollback INSERT check constraint. */
-		assert(space != NULL);
-		assert(space_ck_constraint_by_name(space,
-				ck->def->name, strlen(ck->def->name)) != NULL);
-		rlist_del_entry(ck, link);
-		ck_constraint_delete(ck);
-		if (rlist_empty(&space->ck_constraint)) {
-			trigger_clear(ck_trigger);
-			ck_trigger->destroy(ck_trigger);
-			space->ck_constraint_trigger = NULL;
-		}
-	} else {
-		/* Rollback REPLACE check constraint. */
-		assert(space != NULL);
-		const char *name = ck->def->name;
-		struct ck_constraint *new_ck =
-			space_ck_constraint_by_name(space, name, strlen(name));
-		assert(new_ck != NULL);
-		rlist_del_entry(new_ck, link);
-		rlist_add_entry(&space->ck_constraint, ck, link);
-		ck_constraint_delete(new_ck);
+	assert(ck_trigger != NULL);
+	assert(space_ck_constraint_by_name(space, ck->def->name,
+					   strlen(ck->def->name)) != NULL);
+	rlist_del_entry(ck, link);
+	ck_constraint_delete(ck);
+	if (rlist_empty(&space->ck_constraint)) {
+		trigger_clear(ck_trigger);
+		ck_trigger->destroy(ck_trigger);
+		space->ck_constraint_trigger = NULL;
 	}
+	trigger_run_xc(&on_alter_space, space);
 }
 
-/**
- * Trigger invoked on commit in the _ck_constraint space.
- * Drop useless old check constraint object if exists.
- */
+/** Commit DELETE check constraint. */
 static void
-on_replace_ck_constraint_commit(struct trigger *trigger, void *event)
+on_drop_ck_constraint_commit(struct trigger *trigger, void * /* event */)
 {
-	struct txn_stmt *stmt = txn_last_stmt((struct txn *) event);
 	struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
 	assert(ck != NULL);
 	struct space *space = space_by_id(ck->def->space_id);
 	assert(space != NULL);
-	if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
-		/* Commit DELETE check constraint. */
-		struct trigger *ck_trigger = space->ck_constraint_trigger;
-		assert(ck_trigger != NULL);
-		if (rlist_empty(&space->ck_constraint)) {
-			ck_trigger->destroy(ck_trigger);
-			space->ck_constraint_trigger = NULL;
-			ck_constraint_delete(ck);
-		}
-	} else if (stmt->old_tuple != NULL) {
-		/* Commit REPLACE check constraint. */
-		assert(stmt->new_tuple != NULL);
+	struct trigger *ck_trigger = space->ck_constraint_trigger;
+	assert(ck_trigger != NULL);
+	if (rlist_empty(&space->ck_constraint)) {
+		ck_trigger->destroy(ck_trigger);
+		space->ck_constraint_trigger = NULL;
 		ck_constraint_delete(ck);
 	}
-	/* Export schema changes to Lua. */
+}
+
+/** Rollback DELETE check constraint. */
+static void
+on_drop_ck_constraint_rollback(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);
+	assert(space != NULL);
+	struct trigger *ck_trigger = space->ck_constraint_trigger;
+	assert(ck_trigger != NULL);
+	assert(space_ck_constraint_by_name(space, ck->def->name,
+					   strlen(ck->def->name)) == NULL);
+	rlist_add_entry(&space->ck_constraint, ck, link);
+	if (rlist_empty(&ck_trigger->link))
+		trigger_add(&space->on_replace, ck_trigger);
+	trigger_run_xc(&on_alter_space, space);
+}
+
+/** Commit REPLACE check constraint. */
+static void
+on_replace_ck_constraint_commit(struct trigger *trigger, void * /* event */)
+{
+	struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
+	if (ck != NULL)
+		ck_constraint_delete(ck);
+}
+
+/** Rollback REPLACE check constraint. */
+static void
+on_replace_ck_constraint_rollback(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);
+	assert(space != NULL);
+	struct ck_constraint *new_ck = space_ck_constraint_by_name(space,
+					ck->def->name, strlen(ck->def->name));
+	assert(new_ck != NULL);
+	rlist_del_entry(new_ck, link);
+	rlist_add_entry(&space->ck_constraint, ck, link);
+	ck_constraint_delete(new_ck);
 	trigger_run_xc(&on_alter_space, space);
 }
 
@@ -4459,10 +4469,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 	struct space *space = space_cache_find_xc(space_id);
 	struct trigger *ck_trigger = space->ck_constraint_trigger;
 	assert(ck_trigger == NULL || !rlist_empty(&ck_trigger->link));
-	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);
+	struct trigger *on_rollback = txn_alter_trigger_new(NULL, NULL);
+	struct trigger *on_commit = txn_alter_trigger_new(NULL, NULL);
 
 	if (new_tuple != NULL) {
 		bool is_deferred =
@@ -4516,9 +4524,15 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 			space->ck_constraint_trigger = ck_trigger;
 		}
 		ck_guard.is_active = false;
-		on_commit->data = old_tuple == NULL ? new_ck_constraint :
-						      old_ck_constraint;
-		on_rollback->data = on_commit->data;
+		if (old_tuple != NULL) {
+			on_rollback->data = old_ck_constraint;
+			on_rollback->run = on_replace_ck_constraint_rollback;
+		} else {
+			on_rollback->data = new_ck_constraint;
+			on_rollback->run = on_create_ck_constraint_rollback;
+		}
+		on_commit->data = old_ck_constraint;
+		on_commit->run = on_replace_ck_constraint_commit;
 	} else {
 		assert(ck_trigger != NULL);
 		assert(new_tuple == NULL && old_tuple != NULL);
@@ -4535,11 +4549,15 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 		if (rlist_empty(&space->ck_constraint))
 			trigger_clear(ck_trigger);
 		on_commit->data = old_ck_constraint;
+		on_commit->run = on_drop_ck_constraint_commit;
 		on_rollback->data = old_ck_constraint;
+		on_rollback->run = on_drop_ck_constraint_rollback;
 	}
 
 	txn_on_rollback(txn, on_rollback);
 	txn_on_commit(txn, on_commit);
+
+	trigger_run_xc(&on_alter_space, space);
 }
 
 struct trigger alter_space_on_replace_space = {
-- 
2.11.0

[PATCH 10/10] ddl: don't use txn_last_stmt on _cluster commit/rollback

[Vladimir Davydov]

When we implement transactional DDL, txn_last_stmt won't necessarily
point to the right statement on commit or rollback so we must avoid
using it.

Note, replicas are still registered/unregisterd in the on_commit
trigger, but that's okay, as we don't really need _cluster space to
be in sync with the replica set.
---
 src/box/alter.cc | 46 +++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index dd66e46f..b470b763 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3189,24 +3189,9 @@ on_replace_dd_schema(struct trigger * /* trigger */, void *event)
  * with it.
  */
 static void
-on_commit_dd_cluster(struct trigger *trigger, void *event)
+register_replica(struct trigger *trigger, void * /* event */)
 {
-	(void) trigger;
-	struct txn_stmt *stmt = txn_last_stmt((struct txn *) event);
-	struct tuple *new_tuple = stmt->new_tuple;
-	struct tuple *old_tuple = stmt->old_tuple;
-
-	if (new_tuple == NULL) {
-		struct tt_uuid old_uuid;
-		tuple_field_uuid_xc(stmt->old_tuple, BOX_CLUSTER_FIELD_UUID,
-				    &old_uuid);
-		struct replica *replica = replica_by_uuid(&old_uuid);
-		assert(replica != NULL);
-		replica_clear_id(replica);
-		return;
-	} else if (old_tuple != NULL) {
-		return; /* nothing to change */
-	}
+	struct tuple *new_tuple = (struct tuple *)trigger->data;
 
 	uint32_t id = tuple_field_u32_xc(new_tuple, BOX_CLUSTER_FIELD_ID);
 	tt_uuid uuid;
@@ -3224,6 +3209,19 @@ on_commit_dd_cluster(struct trigger *trigger, void *event)
 	}
 }
 
+static void
+unregister_replica(struct trigger *trigger, void * /* event */)
+{
+	struct tuple *old_tuple = (struct tuple *)trigger->data;
+
+	struct tt_uuid old_uuid;
+	tuple_field_uuid_xc(old_tuple, BOX_CLUSTER_FIELD_UUID, &old_uuid);
+
+	struct replica *replica = replica_by_uuid(&old_uuid);
+	assert(replica != NULL);
+	replica_clear_id(replica);
+}
+
 /**
  * A trigger invoked on replace in the space _cluster,
  * which contains cluster configuration.
@@ -3276,6 +3274,11 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
 					  "Space _cluster",
 					  "updates of instance uuid");
 			}
+		} else {
+			struct trigger *on_commit;
+			on_commit = txn_alter_trigger_new(register_replica,
+							  new_tuple);
+			txn_on_commit(txn, on_commit);
 		}
 	} else {
 		/*
@@ -3286,11 +3289,12 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
 		uint32_t replica_id =
 			tuple_field_u32_xc(old_tuple, BOX_CLUSTER_FIELD_ID);
 		replica_check_id(replica_id);
-	}
 
-	struct trigger *on_commit =
-			txn_alter_trigger_new(on_commit_dd_cluster, NULL);
-	txn_on_commit(txn, on_commit);
+		struct trigger *on_commit;
+		on_commit = txn_alter_trigger_new(unregister_replica,
+						  old_tuple);
+		txn_on_commit(txn, on_commit);
+	}
 }
 
 /* }}} cluster configuration */
-- 
2.11.0

Re: [PATCH 00/10] Prepare box/alter.cc for transactional DDL

[Vladimir Davydov]

On Wed, Jul 03, 2019 at 10:30:02PM +0300, Vladimir Davydov wrote:
> A set of pretty straightforward patches that prepare system space
> triggers for transactional DDL, namely:
> 
>  - Make sure that in-memory schema updates are in sync with data
>    updates, i.e. changes to the schema are done on replace, not on
>    commit. Add tests whenever possible.
>  - Removes txn_last_stmt from on commit/rollback, because it won't
>    be available once there may be more than one DDL statement in a
>    transaction.
>  - Fixes some inconsistencies in sequence object rollback.

Discussed f2f with Kostja. Agreed to push this patch set as is.

Pushed to master.