[patches] [PATCH 4/4] Allow to do not specify tail nullable index columns

v.shpilevoy at tarantool.org v.shpilevoy at tarantool.org
Sat Feb 17 23:38:57 MSK 2018



С уважением, Владислав Шпилевой.

> 17 февр. 2018 г., в 23:19, Vladimir Davydov <vdavydov.dev at gmail.com> написал(а):
> 
>> On Tue, Feb 13, 2018 at 06:28:38PM +0300, Vladislav Shpilevoy wrote:
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index c48e89f9e..cb3b8353d 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -323,7 +323,39 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
>>                         space->def->field_count) != 0)
>>            diag_raise();
>>    }
>> -    key_def = key_def_new_with_parts(part_def, part_count);
>> +    struct space_def *sd = space->def;
>> +    int unchanged_index_count = 0;
>> +    struct key_def **keys = NULL;
>> +    /*
>> +     * To detect which key parts are optional, min_field_count
>> +     * is required. But min_field_count from the old space
>> +     * format can not be used. For example, consider the case,
>> +     * when a space has no format, has a primary index on the
>> +     * first field and has a single secondary index on a
>> +     * non-nullable second field. Min field count here is 2.
>> +     * Now alter the secondary index to make its part be
>> +     * nullable. The 'space' variable here is the old space,
>> +     * where min_field_count is still 2, but actually it is
>> +     * already 1. Actual min_field_count must be calculated
>> +     * using old unchanged indexes, NEW definition of an
>> +     * updated index and a space format, defined by a user.
>> +     */
>> +    for (uint32_t i = 0; i < space->index_count; ++i) {
>> +        if (space->index[i]->def->iid != index_id)
>> +            ++unchanged_index_count;
>> +    }
>> +    if (unchanged_index_count > 0) {
>> +        size_t bsize = unchanged_index_count * sizeof(keys[0]);
>> +        keys = (struct key_def **) region_alloc_xc(&fiber()->gc, bsize);
>> +        for (uint32_t i = 0, j = 0; i < space->index_count; ++i) {
>> +            if (space->index[i]->def->iid != index_id)
>> +                keys[j++] = space->index[i]->def->key_def;
>> +        }
>> +    }
>> +    uint32_t min_field_count =
>> +        tuple_format_min_field_count(keys, unchanged_index_count,
>> +                         sd->fields, sd->field_count);
>> +    key_def = key_def_new_with_parts(part_def, part_count, min_field_count);
> 
> What will happen if there are two indexes over the same field, nullable
> and not nullable, and the one which is not nullable is set to be
> nullable? I don't see where you update is_optional then.

Is_optional in such a case is set due to decreased min_field_count.

> 
> AFAIU is_optional should be updated for all indexes in alter_space_do()
> or in AlterSpaceOps, not here.

I disagree.

> 
>> diff --git a/src/box/key_def.h b/src/box/key_def.h
>> index 96f4cdbf9..a7d6eedfb 100644
>> --- a/src/box/key_def.h
>> +++ b/src/box/key_def.h
>> @@ -72,6 +72,8 @@ struct key_part {
>>    struct coll *coll;
>>    /** True if a part can store NULLs. */
>>    bool is_nullable;
>> +    /** True, if a part can absense in a tuple. */
> 
> 'absence' is always a noun, never a verb.
> Fix your comments please.

Ok.

> 
>> +    bool is_optional;
>> };
>> 
>> struct key_def;
>> @@ -126,6 +128,11 @@ struct key_def {
>>    uint32_t unique_part_count;
>>    /** True, if at least one part can store NULL. */
>>    bool is_nullable;
>> +    /**
>> +     * True, if some key parts can absense in a tuple. These
>> +     * fields assumed to be MP_NIL.
>> +     */
>> +    bool is_optional;
> 
> You add is_optional to both key_def and key_part, but I fail to
> understand why you need them. You can detect if a field is missing
> in a tuple without any flags, right? So why do you need them?

It is Kostja’s decision.

> 
>> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
>> index dcfb4f636..10c00a6f4 100644
>> --- a/src/box/tuple_compare.cc
>> +++ b/src/box/tuple_compare.cc
>> @@ -589,18 +616,12 @@ tuple_compare_with_key_slowpath(const struct tuple *tuple, const char *key,
>>    return 0;
>> }
>> 
>> -/**
>> - * is_sequential_nullable_tuples used to determine if this
>> - * function is used to compare two tuples, which keys are
>> - * sequential. In such a case it is necessary to compare them
>> - * using primary parts if a NULL was met.
>> - */
>> -template<bool is_nullable, bool is_sequential_nullable_tuples>
>> +template<bool is_nullable>
>> static inline int
>> key_compare_parts(const char *key_a, const char *key_b, uint32_t part_count,
>>          const struct key_def *key_def)
> 
> To ease review, removal of is_sequential_nullable_tuples template
> parameter should be done in a separate patch.
> 
>> -static int
>> -tuple_compare_sequential_nullable(const struct tuple *tuple_a,
>> -                  const struct tuple *tuple_b,
>> -                  const struct key_def *key_def)
> 
> Turning tuple_compare_sequential_nullable into a template parameter
> should be done in a separate patch. Folding it in the patch removing
> is_sequential_nullable_tuples (see above) would be OK.




More information about the Tarantool-patches mailing list