From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 9 Apr 2018 23:36:02 +0300 From: Konstantin Osipov Subject: Re: [PATCH 05/12] alter: allocate triggers before the point of no return Message-ID: <20180409203602.GF4527@atlas> References: <55a4b2ee61c194074a98f03c5f42306662a3d9e9.1523105106.git.vdavydov.dev@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55a4b2ee61c194074a98f03c5f42306662a3d9e9.1523105106.git.vdavydov.dev@gmail.com> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [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. > --- > src/box/alter.cc | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 36310f1c..9d0c4c23 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -803,6 +803,11 @@ 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. */ > + 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 +858,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 +883,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); > } > > -- > 2.11.0 > -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov