[PATCH v2 2/2] alter: zap space_vtab::commit_alter
Vladimir Davydov
vdavydov.dev at gmail.com
Fri Apr 6 17:23:39 MSK 2018
On Fri, Apr 06, 2018 at 05:10:01PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev at gmail.com> [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.
This comment is only pertinent to memtx so I just moved it to
memtx_space_drop_primary_key.
Regarding DropIndex::alter() - the code is pretty self-explaining there
(call drop_primary_key if the primary key is dropped) and doesn't need
any comments IMO.
>
> The patch is OK to push after putting back the previous or adding
> a new good comment.
More information about the Tarantool-patches
mailing list