Re: [PATCH v2 2/2] alter: zap space_vtab::commit_alter

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@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.

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