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

[Vladimir Davydov]

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?

> Do you want to copy it in each mem and run and range? It is discussed
> with Kostja, and he has approved key_def reference counting. Kostja,
> correct me, please, if I misunderstood you?
> 
> > 
> > Anyway, this isn't enough. Apart from worker threads, there are also
> > reader fibers, which access key_def too. Referencing key_def for them is
> > likely to turn the code into even a bigger mess than it is with this
> > patch...
> 
> I can not find places in a code, where key_def has no 'index->' before it, except workers (write_iterator), mems, ranges and cache. For workers I moved key_def into struct task. For other places I update key_def in commit_alter(). Where key_def is used in Tx fibers instead of different threads via index->key_def, there is no problems.
> 
> > 
> > IMO we should simply disable this feature for vinyl for now - I don't
> > think anybody will give a damn.
> 
> Disabling this feature for vinyl is breaking change. Actually, we have forbid to reset space format:
> 
> box.cfg{}
> s = box.schema.create_space('test', {engine = 'vinyl'})
> format = {}
> format[1] = {'field1', 'unsigned'}
> format[2] = {'field2', 'unsigned', is_nullable = true}
> format[3] = {'field3', 'unsigned'}
> s:format(format)
> pk = s:create_index('pk')
> 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.

Anyway, the fact that you can't change a key definition or space format
without potentially affecting other key definitions looks cheesy...