[tarantool-patches] Re: [PATCH] Allow nullability mismatch in space indices and format.

Mon Aug 13 16:42:22 MSK 2018

Hi! Thanks for the fixes! LGTM.

On 09/08/2018 10:09, Sergey Petrenko wrote:
> Hi! Thank you for review. I fixed your comments.
> Please find the new diff below.
> 
>> 8 авг. 2018 г., в 15:50, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> написал(а):
>>
>> Hi! Thanks for the patch! See 8 comments below.
>>
>> 1. Please, prefix the commit title with a subsystem name to
>> which the patch is related. (Here it is 'box: ' I think).
> 
> Done.
> 
>>
>>> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
>>> index 2e19d2e3c..a640c23d3 100644
>>> --- a/src/box/tuple_format.c
>>> +++ b/src/box/tuple_format.c
>>> @@ -84,12 +84,11 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
>>>   			if (part->fieldno >= field_count) {
>>>   				field->is_nullable = part->is_nullable;
>>>   			} else if (field->is_nullable != part->is_nullable) {
>>> -				diag_set(ClientError, ER_NULLABLE_MISMATCH,
>>> -					 part->fieldno + TUPLE_INDEX_BASE,
>>> -					 field->is_nullable ? "nullable" :
>>> -					 "not nullable", part->is_nullable ?
>>> -					 "nullable" : "not nullable");
>>> -				return -1;
>>> +				/*
>>
>> 2. On the branch here a leading white space is. Please, remove.
>>
>> 3. Please, wrap the comment by 66 symbols.
>>
>> 4. ER_NULLABLE_MISMATCH is now unused. Please, replace it with
>> a placeholder in errcode.h file. (For examples grep ER_UNUSED in
>> the file).
> 
> Done, done, done.
> 
>>> +				 * In case of mismatch set the most strict
>>> +				 * option for is_nullable.
>>> +				 */
>>> +				field->is_nullable = false;
>>>   			}
>>>     			/*
>>> diff --git a/test/engine/ddl.result b/test/engine/ddl.result
>>> index 0cb9ed6c2..c8fd8e6f2 100644
>>> --- a/test/engine/ddl.result
>>> +++ b/test/engine/ddl.result
>>> @@ -1431,9 +1431,12 @@ s:create_index('primary', { parts = { 'field1' } })
>>>   ---
>>>   - error: Primary index of the space 'test' can not contain nullable parts
>>>   ...
>>> -s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
>>> +-- this is allowed, but the actual part is_nullable stays false
>>
>> 5. Please, in comments start sentences from a capital letter and finish
>> with the dot.
> 
> Done.
> 
>>
>>> +pk = s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
>>> +---
>>> +...
>>> +pk:drop()
>>>   ---
>>> -- error: Field 1 is nullable in space format, but not nullable in index parts
>>>   ...
>>>   format[1].is_nullable = false
>>>   ---
>>> diff --git a/test/engine/null.result b/test/engine/null.result
>>> index 4abf85021..e0ad7df8e 100644
>>> --- a/test/engine/null.result
>>> +++ b/test/engine/null.result
>>> @@ -70,48 +70,8 @@ ok
>>>   ---
>>>   - false
>>>   ...
>>> --- Conflict of is_nullable in format and in parts.
>>> -parts[1].is_nullable = false
>>> ----
>>> -...
>>>   sk = s:create_index('sk', { parts = parts }) -- Fail.
>>
>> 6. Now the comment is wrong - this line does not fail.
> 
> Fixed.
> 
>>
>>>   ---
>>> -- error: Field 2 is nullable in space format, but not nullable in index parts
>>> -...
>>> --- Try skip nullable in format and specify in part.
>>> -parts[1].is_nullable = true
>>> ----
>>> -...
>>> -sk = s:create_index('sk', { parts = parts }) -- Ok.
>>> ----
>>> -...
>>> -format[2].is_nullable = nil
>>> ----
>>> -...
>>> -s:format(format) -- Fail.
>>> ----
>>> -- error: Field 2 is not nullable in space format, but nullable in index parts
>>> -...
>>> -sk:drop()
>>> ----
>>> -...
>>> --- Try to set nullable in part with no format.
>>> -s:format({})
>>> ----
>>> -...
>>> -sk = s:create_index('sk', { parts = parts })
>>> ----
>>> -...
>>> --- And then set format with no nullable.
>>> -s:format(format) -- Fail.
>>> ----
>>> -- error: Field 2 is not nullable in space format, but nullable in index parts
>>> -...
>>> -format[2].is_nullable = true
>>> ----
>>> -...
>>> -s:format(format) -- Ok.
>>> ----
>>>   ...
>>>   -- Test insert.
>>>   s:insert{1, 1}
>>> @@ -1560,3 +1520,168 @@ i2:select{}
>>>   s:drop()
>>>   ---
>>>   ...
>>> +-- gh-3430 allow different nullability in space format and indices.
>>> +-- resulting field nullability is the strictest of the two.
>>> +s = box.schema.space.create('test', {engine=engine})
>>> +---
>>> +...
>>> +pk = s:create_index('primary', {parts={1, 'unsigned'}})
>>> +---
>>> +...
>>> +sk = s:create_index('secondary', {parts={2, 'unsigned', is_nullable=false}})
>>> +---
>>> +...
>>> +format = {}
>>> +---
>>> +...
>>> +format[1] = {name = 'first', type = 'unsigned', is_nullable = false}
>>> +---
>>> +...
>>> +format[2] = {name = 'second', type = 'unsigned', is_nullable = false}
>>> +---
>>> +...
>>> +s:format(format)
>>> +---
>>> +...
>>> +-- field 2 is not nullable
>>> +s:insert{5}
>>> +---
>>> +- error: Tuple field count 1 is less than required by space format or defined indexes
>>> +    (expected at least 2)
>>> +...
>>> +s:insert{5, nil}
>>
>> 7. {5, nil} == {5}, so I think you can omit this insertion. It is
>> covered by the previous one.
> 
> Done.
> 
>> 8. What if I insert NULL into the space and then set this field to
>> be not nullable in format or one of indexes? Please, add a test.
> 
> Added an extensive test, setting in format fails, setting in index fails.
> 
> Here’s diff:
> 
>   src/box/errcode.h         |   2 +-
>   src/box/tuple_format.c    |  12 +--
>   test/box/misc.result      |   5 +-
>   test/engine/ddl.result    |  10 ++-
>   test/engine/ddl.test.lua  |   6 +-
>   test/engine/null.result   | 213 +++++++++++++++++++++++++++++++++++++---------
>   test/engine/null.test.lua |  78 +++++++++++++----
>   7 files changed, 255 insertions(+), 71 deletions(-)
> 
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index 3d5f66af8..4115e6b65 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -205,7 +205,7 @@ struct errcode_record {
>   	/*150 */_(ER_CANT_CREATE_COLLATION,	"Failed to initialize collation: %s.") \
>   	/*151 */_(ER_WRONG_COLLATION_OPTIONS,	"Wrong collation options (field %u): %s") \
>   	/*152 */_(ER_NULLABLE_PRIMARY,		"Primary index of the space '%s' can not contain nullable parts") \
> -	/*153 */_(ER_NULLABLE_MISMATCH,		"Field %d is %s in space format, but %s in index parts") \
> +	/*153 */_(ER_UNUSED,			"") \
>   	/*154 */_(ER_TRANSACTION_YIELD,		"Transaction has been aborted by a fiber yield") \
>   	/*155 */_(ER_NO_SUCH_GROUP,		"Replication group '%s' does not exist") \
>   
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 2e19d2e3c..8a008858e 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -84,12 +84,12 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
>   			if (part->fieldno >= field_count) {
>   				field->is_nullable = part->is_nullable;
>   			} else if (field->is_nullable != part->is_nullable) {
> -				diag_set(ClientError, ER_NULLABLE_MISMATCH,
> -					 part->fieldno + TUPLE_INDEX_BASE,
> -					 field->is_nullable ? "nullable" :
> -					 "not nullable", part->is_nullable ?
> -					 "nullable" : "not nullable");
> -				return -1;
> +				/*
> +				 * In case of mismatch set the
> +				 * most strict option for
> +				 * is_nullable.
> +				 */
> +				field->is_nullable = false;
>   			}
>   
>   			/*
> diff --git a/test/box/misc.result b/test/box/misc.result
> index 4895a78a2..8934e0c87 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -398,13 +398,12 @@ t;
>     - 'box.error.UPDATE_ARG_TYPE : 26'
>     - 'box.error.CROSS_ENGINE_TRANSACTION : 81'
>     - 'box.error.FORMAT_MISMATCH_INDEX_PART : 27'
> -  - 'box.error.injection : table: <address>
>     - 'box.error.FUNCTION_TX_ACTIVE : 30'
> +  - 'box.error.injection : table: <address>
>     - 'box.error.ITERATOR_TYPE : 72'
> -  - 'box.error.TRANSACTION_YIELD : 154'
>     - 'box.error.NO_SUCH_ENGINE : 57'
>     - 'box.error.COMMIT_IN_SUB_STMT : 122'
> -  - 'box.error.NULLABLE_MISMATCH : 153'
> +  - 'box.error.TRANSACTION_YIELD : 154'
>     - 'box.error.UNSUPPORTED : 5'
>     - 'box.error.LAST_DROP : 15'
>     - 'box.error.SPACE_FIELD_IS_DUPLICATE : 149'
> diff --git a/test/engine/ddl.result b/test/engine/ddl.result
> index 0cb9ed6c2..c114fb98b 100644
> --- a/test/engine/ddl.result
> +++ b/test/engine/ddl.result
> @@ -1431,9 +1431,12 @@ s:create_index('primary', { parts = { 'field1' } })
>   ---
>   - error: Primary index of the space 'test' can not contain nullable parts
>   ...
> -s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
> +-- This is allowed, but the actual part is_nullable stays false.
> +pk = s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
> +---
> +...
> +pk:drop()
>   ---
> -- error: Field 1 is nullable in space format, but not nullable in index parts
>   ...
>   format[1].is_nullable = false
>   ---
> @@ -1462,9 +1465,10 @@ s:insert({1, NULL})
>   format[1].is_nullable = true
>   ---
>   ...
> +-- The format is allowed since in primary index parts
> +-- is_nullable is still set to false.
>   s:format(format)
>   ---
> -- error: Field 1 is nullable in space format, but not nullable in index parts
>   ...
>   format[1].is_nullable = false
>   ---
> diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
> index a128f6000..57aa756ab 100644
> --- a/test/engine/ddl.test.lua
> +++ b/test/engine/ddl.test.lua
> @@ -534,7 +534,9 @@ format[1] = { name = 'field1', type = 'unsigned', is_nullable = true }
>   format[2] = { name = 'field2', type = 'unsigned', is_nullable = true }
>   s = box.schema.space.create('test', {engine = engine, format = format})
>   s:create_index('primary', { parts = { 'field1' } })
> -s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
> +-- This is allowed, but the actual part is_nullable stays false.
> +pk = s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
> +pk:drop()
>   format[1].is_nullable = false
>   s:format(format)
>   s:create_index('primary', { parts = {{'field1', is_nullable = true}} })
> @@ -545,6 +547,8 @@ i.parts
>   -- Check that is_nullable can't be set to false on non-empty space
>   s:insert({1, NULL})
>   format[1].is_nullable = true
> +-- The format is allowed since in primary index parts
> +-- is_nullable is still set to false.
>   s:format(format)
>   format[1].is_nullable = false
>   format[2].is_nullable = false
> diff --git a/test/engine/null.result b/test/engine/null.result
> index 4abf85021..48d678443 100644
> --- a/test/engine/null.result
> +++ b/test/engine/null.result
> @@ -70,49 +70,9 @@ ok
>   ---
>   - false
>   ...
> --- Conflict of is_nullable in format and in parts.
> -parts[1].is_nullable = false
> ----
> -...
> -sk = s:create_index('sk', { parts = parts }) -- Fail.
> ----
> -- error: Field 2 is nullable in space format, but not nullable in index parts
> -...
> --- Try skip nullable in format and specify in part.
> -parts[1].is_nullable = true
> ----
> -...
> -sk = s:create_index('sk', { parts = parts }) -- Ok.
> ----
> -...
> -format[2].is_nullable = nil
> ----
> -...
> -s:format(format) -- Fail.
> ----
> -- error: Field 2 is not nullable in space format, but nullable in index parts
> -...
> -sk:drop()
> ----
> -...
> --- Try to set nullable in part with no format.
> -s:format({})
> ----
> -...
>   sk = s:create_index('sk', { parts = parts })
>   ---
>   ...
> --- And then set format with no nullable.
> -s:format(format) -- Fail.
> ----
> -- error: Field 2 is not nullable in space format, but nullable in index parts
> -...
> -format[2].is_nullable = true
> ----
> -...
> -s:format(format) -- Ok.
> ----
> -...
>   -- Test insert.
>   s:insert{1, 1}
>   ---
> @@ -1560,3 +1520,176 @@ i2:select{}
>   s:drop()
>   ---
>   ...
> +-- gh-3430 allow different nullability in space format and indices.
> +-- Resulting field nullability is the strictest of the two.
> +s = box.schema.space.create('test', {engine=engine})
> +---
> +...
> +pk = s:create_index('primary', {parts={1, 'unsigned'}})
> +---
> +...
> +sk = s:create_index('secondary', {parts={2, 'unsigned', is_nullable=false}})
> +---
> +...
> +format = {}
> +---
> +...
> +format[1] = {name = 'first', type = 'unsigned', is_nullable = false}
> +---
> +...
> +format[2] = {name = 'second', type = 'unsigned', is_nullable = false}
> +---
> +...
> +s:format(format)
> +---
> +...
> +-- Field 2 is not nullable.
> +s:insert{5}
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +s:insert{5, box.NULL}
> +---
> +- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
> +...
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=true}}} -- This is allowed.
> +---
> +...
> +-- Without space format setting this fails.
> +s:insert{5, box.NULL}
> +---
> +- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
> +...
> +s:insert{5}
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}}
> +---
> +...
> +format[2].is_nullable = true
> +---
> +...
> +s:format(format) -- This is also allowed.
> +---
> +...
> +-- inserts still fail due to not nullable index parts.
> +s:insert{5, box.NULL}
> +---
> +- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
> +...
> +s:insert{5}
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=true}}}
> +---
> +...
> +-- Now the field really is nullable.
> +-- Success.
> +s:insert{5, box.NULL}
> +---
> +- [5, null]
> +...
> +s:insert{6}
> +---
> +- [6]
> +...
> +s:insert{7, 8}
> +---
> +- [7, 8]
> +...
> +s:select{}
> +---
> +- - [5, null]
> +  - [6]
> +  - [7, 8]
> +...
> +-- Check that we cannot set field nullability to false when the
> +-- space has tuples with NULL in this field.
> +format[2].is_nullable = false
> +---
> +...
> +s:format(format) -- Fail.
> +---
> +- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
> +...
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}} -- Fail.
> +---
> +- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
> +...
> +_ = s:delete{5}
> +---
> +...
> +s:format(format) -- Still fail.
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}} -- Still fail.
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +-- Now check we can set nullability to false step by step.
> +_ = s:delete{6}
> +---
> +...
> +format[2].is_nullable = false
> +---
> +...
> +s:format(format)
> +---
> +...
> +s:insert{5, box.NULL} -- Fail.
> +---
> +- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
> +...
> +s:insert{5} -- Fail.
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +format[2].is_nullable = true
> +---
> +...
> +s:format(format)
> +---
> +...
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}}
> +---
> +...
> +s:insert{5, box.NULL} -- Fail.
> +---
> +- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
> +...
> +s:insert{5} -- Fail.
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +format[2].is_nullable = false
> +---
> +...
> +s:format(format)
> +---
> +...
> +s:select{}
> +---
> +- - [7, 8]
> +...
> +s:insert{5} -- Fail.
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +s:insert{9, 10} -- Success.
> +---
> +- [9, 10]
> +...
> +s:drop()
> +---
> +...
> diff --git a/test/engine/null.test.lua b/test/engine/null.test.lua
> index 7f5a7dd33..5eb068499 100644
> --- a/test/engine/null.test.lua
> +++ b/test/engine/null.test.lua
> @@ -35,24 +35,7 @@ pk = s:create_index('pk')
>   ok = pcall(s.create_index, s, 'sk', { parts = parts, type = 'hash' }) -- Fail.
>   ok
>   
> --- Conflict of is_nullable in format and in parts.
> -parts[1].is_nullable = false
> -sk = s:create_index('sk', { parts = parts }) -- Fail.
> -
> --- Try skip nullable in format and specify in part.
> -parts[1].is_nullable = true
> -sk = s:create_index('sk', { parts = parts }) -- Ok.
> -format[2].is_nullable = nil
> -s:format(format) -- Fail.
> -sk:drop()
> -
> --- Try to set nullable in part with no format.
> -s:format({})
>   sk = s:create_index('sk', { parts = parts })
> --- And then set format with no nullable.
> -s:format(format) -- Fail.
> -format[2].is_nullable = true
> -s:format(format) -- Ok.
>   
>   -- Test insert.
>   
> @@ -461,3 +444,64 @@ i2:select{}
>   i2:select{box.NULL, 50}
>   i2:select{}
>   s:drop()
> +
> +-- gh-3430 allow different nullability in space format and indices.
> +-- Resulting field nullability is the strictest of the two.
> +s = box.schema.space.create('test', {engine=engine})
> +pk = s:create_index('primary', {parts={1, 'unsigned'}})
> +sk = s:create_index('secondary', {parts={2, 'unsigned', is_nullable=false}})
> +format = {}
> +format[1] = {name = 'first', type = 'unsigned', is_nullable = false}
> +format[2] = {name = 'second', type = 'unsigned', is_nullable = false}
> +s:format(format)
> +-- Field 2 is not nullable.
> +s:insert{5}
> +s:insert{5, box.NULL}
> +
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=true}}} -- This is allowed.
> +-- Without space format setting this fails.
> +s:insert{5, box.NULL}
> +s:insert{5}
> +
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}}
> +format[2].is_nullable = true
> +s:format(format) -- This is also allowed.
> +-- inserts still fail due to not nullable index parts.
> +s:insert{5, box.NULL}
> +s:insert{5}
> +
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=true}}}
> +-- Now the field really is nullable.
> +-- Success.
> +s:insert{5, box.NULL}
> +s:insert{6}
> +s:insert{7, 8}
> +s:select{}
> +
> +-- Check that we cannot set field nullability to false when the
> +-- space has tuples with NULL in this field.
> +format[2].is_nullable = false
> +s:format(format) -- Fail.
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}} -- Fail.
> +_ = s:delete{5}
> +s:format(format) -- Still fail.
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}} -- Still fail.
> +-- Now check we can set nullability to false step by step.
> +_ = s:delete{6}
> +
> +format[2].is_nullable = false
> +s:format(format)
> +s:insert{5, box.NULL} -- Fail.
> +s:insert{5} -- Fail.
> +format[2].is_nullable = true
> +s:format(format)
> +s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}}
> +s:insert{5, box.NULL} -- Fail.
> +s:insert{5} -- Fail.
> +format[2].is_nullable = false
> +s:format(format)
> +s:select{}
> +s:insert{5} -- Fail.
> +s:insert{9, 10} -- Success.
> +
> +s:drop()
> 