From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 6 Apr 2018 17:10:01 +0300 From: Konstantin Osipov Subject: Re: [PATCH v2 2/2] alter: zap space_vtab::commit_alter Message-ID: <20180406141001.GE18946@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [18/04/06 16:20]: > space_vtab::commit_alter is implemented only by memtx, which uses it > to set bsize for a new space. However, it isn't necessary to use > commit_alter for this - instead we can set bsize in prepare_alter and > reset it in drop_primary_key, along with memtx_space::replace. Let's > do it and zap space_vtab::commit_alter altogether, because the callback > is confusing: judging by its name it should be called after WAL write, > but it is called before. > void > DropIndex::alter(struct alter_space *alter) > { > - /* > - * If it's not the primary key, nothing to do -- > - * the dropped index didn't exist in the new space > - * definition, so does not exist in the created space. > - */ > - if (space_index(alter->new_space, 0) != NULL) > - return; > - /* > - * OK to drop the primary key. Inform the engine about it, > - * since it may have to reset handler->replace function, > - * so that: > - * - DML returns proper errors rather than crashes the > - * program > - * - when a new primary key is finally added, the space > - * can be put back online properly. > - */ > - space_drop_primary_key(alter->new_space); > + if (old_index_def->iid == 0) > + space_drop_primary_key(alter->new_space); > } I keep wondering why you ditched a nice comment. If you think it was bad, please write a better one. The patch is OK to push after putting back the previous or adding a new good comment. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov