Re: [PATCH 05/12] alter: allocate triggers before the point of no return

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Tue, Apr 10, 2018 at 10:57:42AM +0300, Vladimir Davydov wrote:
> On Mon, Apr 09, 2018 at 11:36:02PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [18/04/09 10:33]:
> > > Trigger allocation, as any other memory allocation, is allowed to fail.
> > > If this happens in alter_space_do, the space will be left in an
> > > inconsistent state. Let's move trigger allocation to the beginning of
> > > alter_space_do and add a comment denoting the point of no return.
> > 
> > Previously we would reference the allocated trigger immediately 
> > in txn_on_commit() /txn_on_rollback(), the changed code leaks
> > memory in case of any exception between allocation and "point of no
> > return".
> > 
> > Please add guards.
> 
> We don't need guards for the triggers, because they are allocated on
> region - see txn_alter_trigger_new().

Added a comment, as discussed:

(not on the branch, don't push)

From d0467518750a6d7aaaf0eb80842ecd6ccc6b2219 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Wed, 4 Apr 2018 18:48:13 +0300
Subject: [PATCH] alter: allocate triggers before the point of no return

Trigger allocation, as any other memory allocation, is allowed to fail.
If this happens in alter_space_do, the space will be left in an
inconsistent state. Let's move trigger allocation to the beginning of
alter_space_do and add a comment denoting the point of no return.

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 96143b95..c4141145 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -803,6 +803,15 @@ alter_space_rollback(struct trigger *trigger, void * /* event */)
 static void
 alter_space_do(struct txn *txn, struct alter_space *alter)
 {
+	/*
+	 * Prepare triggers while we may fail. Note, we don't have to
+	 * free them in case of failure, because they are allocated on
+	 * the region.
+	 */
+	struct trigger *on_commit, *on_rollback;
+	on_commit = txn_alter_trigger_new(alter_space_commit, alter);
+	on_rollback = txn_alter_trigger_new(alter_space_rollback, alter);
+
 	/* Create a definition of the new space. */
 	space_dump_def(alter->old_space, &alter->key_list);
 	class AlterSpaceOp *op;
@@ -853,6 +862,11 @@ alter_space_do(struct txn *txn, struct alter_space *alter)
 		throw;
 	}
 
+	/*
+	 * This function must not throw exceptions or yield after
+	 * this point.
+	 */
+
 	/* Rebuild index maps once for all indexes. */
 	space_fill_index_map(alter->old_space);
 	space_fill_index_map(alter->new_space);
@@ -873,11 +887,7 @@ alter_space_do(struct txn *txn, struct alter_space *alter)
 	 * finish or rollback the DDL depending on the results of
 	 * writing to WAL.
 	 */
-	struct trigger *on_commit =
-		txn_alter_trigger_new(alter_space_commit, alter);
 	txn_on_commit(txn, on_commit);
-	struct trigger *on_rollback =
-		txn_alter_trigger_new(alter_space_rollback, alter);
 	txn_on_rollback(txn, on_rollback);
 }