[patches] [PATCH 4/4] vinyl: on space alter update key definitions in indexes

[Vladimir Davydov]

On Mon, Mar 12, 2018 at 07:16:31PM +0300, v.shpilevoy at tarantool.org wrote:
> 
> 
> > 12 марта 2018 г., в 19:10, Vladimir Davydov <vdavydov.dev at gmail.com> написал(а):
> > 
> > On Mon, Mar 12, 2018 at 05:41:12PM +0300, v.shpilevoy at tarantool.org wrote:
> >> 
> >> 
> >>> 12 марта 2018 г., в 16:03, Vladimir Davydov <vdavydov.dev at gmail.com> написал(а):
> >>> 
> >>> On Mon, Mar 12, 2018 at 12:53:36AM +0300, Vladislav Shpilevoy wrote:
> >>>> Vinyl alter can change comparators, for example, by removal of a
> >>>> space format and making some fields optional. This alter is
> >>>> allowed on a non-empty space even, because optionality of a
> >>>> field is not persisted. But index key definitions can not be
> >>>> updated in place, since they are used in multiple threads (vinyl
> >>>> workers). So it is necessary to get new key definitions from a
> >>>> new index definition, and leave old ones to be deleted by
> >>>> existing worker tasks, when they will be finished.
> >>> 
> >>> I don't think that workers alone are worth introducing key_def reference
> >>> counting - we can just copy key_def for them, it is a slow path after
> >>> all.
> >> 
> >> We can, but further we want to store key_def per mem and per run to
> >> allow various formats and keys in different places.
> > 
> > But why? All tuples stored in an index should have the same format and
> > be compared by the same key definition, no?
> 
> Now yes. Later - no. On vinyl alter you can not update formats in all older tuples in all mems and runs.

I don't think I understand. If you change space format, all tuples must
conform to it, no? Why do we need a separate format per each run and/or
mem?

> And on index update you can not change key_defs too.

Why would you need to do anything like this? Just rebuild the whole
index on key_def alter, that's all. Simple and robust.

To be honest, I find all those little tricks, like changing key field
type from unsigned to signed without rebuild, useless. Anybody needs
that?

> 
> > 
> >> sk = s:create_index('sk', {parts = {{2, 'unsigned', is_nullable = true}}})
> >> s:format({}) -- this will fail, because min_field_count is changed. But user even does not know about 'min_field_count'. We simply can not explain to a user, why it is forbidden.
> > 
> > Who cares? De facto, vinyl doesn't support alter at all. The user can
> > drop all indexes and recreate the space with another format.
> 
> Vinyl supports alter. At least, it allows to update format in compatible ways.

You've recently forbidden altering key_def of an empty vinyl index, and
nobody cared. How's space format any different?

> 
> > 
> > Anyway, the fact that you can't change a key definition or space format
> > without potentially affecting other key definitions looks cheesy...
> 
> A user does not see changes of non-altered indexes. So it is not cheesy.

I didn't mean users, I meant the ALTER code - it's getting more and more
convoluted. And the bug you're fixing is a living proof to that.