[tarantool-patches] [PATCH v1 1/1] box: wrong is_nullable for multiple indexes

[tarantool-patches] [PATCH v1 1/1] box: wrong is_nullable for multiple indexes

[imeevma]

If field isn't defined by space format, than in case of multiple
indexes field option is_nullable was the same as it was for last
index that defines it. This is wrong as it should be 'true' only
if it is 'true' for all indexes that defines it.

Closes #3744.
---
Issue: https://github.com/tarantool/tarantool/issues/3744
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3744-wrong-is_nullable-for-multiple-indexes

 src/box/tuple_format.c    | 12 ++----------
 test/engine/null.result   | 24 ++++++++++++++++++++++++
 test/engine/null.test.lua |  9 +++++++++
 3 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 5f4899d..d4af54c 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -38,7 +38,7 @@ static intptr_t recycled_format_ids = FORMAT_ID_NIL;
 static uint32_t formats_size = 0, formats_capacity = 0;
 
 static const struct tuple_field tuple_field_default = {
-	FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, false,
+	FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, true,
 };
 
 /**
@@ -81,15 +81,7 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
 			assert(part->fieldno < format->field_count);
 			struct tuple_field *field =
 				&format->fields[part->fieldno];
-			if (part->fieldno >= field_count) {
-				field->is_nullable = part->is_nullable;
-			} else if (field->is_nullable != part->is_nullable) {
-				/*
-				 * In case of mismatch set the most
-				 * strict option for is_nullable.
-				 */
-				field->is_nullable = false;
-			}
+			field->is_nullable &= part->is_nullable;
 
 			/*
 			 * Check that there are no conflicts
diff --git a/test/engine/null.result b/test/engine/null.result
index 48d6784..4ec0142 100644
--- a/test/engine/null.result
+++ b/test/engine/null.result
@@ -1693,3 +1693,27 @@ s:insert{9, 10} -- Success.
 s:drop()
 ---
 ...
+-- gh-3744: Assertion after improper index creation
+s = box.schema.space.create('test', {engine=engine})
+---
+...
+pk = s:create_index('primary', {parts={1, 'unsigned'}})
+---
+...
+sk1 = s:create_index('sk1', {parts={{2, 'number', is_nullable=false}}})
+---
+...
+s:insert{1, -1, 1}
+---
+- [1, -1, 1]
+...
+sk2 = s:create_index('sk2', {parts={{2, 'number', is_nullable=true}}})
+---
+...
+s:insert{2, nil, 2} --error
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected number'
+...
+s:drop()
+---
+...
diff --git a/test/engine/null.test.lua b/test/engine/null.test.lua
index 5eb0684..ba13adf 100644
--- a/test/engine/null.test.lua
+++ b/test/engine/null.test.lua
@@ -505,3 +505,12 @@ s:insert{5} -- Fail.
 s:insert{9, 10} -- Success.
 
 s:drop()
+
+-- gh-3744: Assertion after improper index creation
+s = box.schema.space.create('test', {engine=engine})
+pk = s:create_index('primary', {parts={1, 'unsigned'}})
+sk1 = s:create_index('sk1', {parts={{2, 'number', is_nullable=false}}})
+s:insert{1, -1, 1}
+sk2 = s:create_index('sk2', {parts={{2, 'number', is_nullable=true}}})
+s:insert{2, nil, 2} --error
+s:drop()
-- 
2.7.4

Re: [tarantool-patches] [PATCH v1 1/1] box: wrong is_nullable for multiple indexes

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 01/11/2018 16:50, imeevma@tarantool.org wrote:
> If field isn't defined by space format, than in case of multiple
> indexes field option is_nullable was the same as it was for last
> index that defines it. This is wrong as it should be 'true' only
> if it is 'true' for all indexes that defines it.
> 
> Closes #3744.
> ---
> Issue: https://github.com/tarantool/tarantool/issues/3744
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3744-wrong-is_nullable-for-multiple-indexes
> 
>   src/box/tuple_format.c    | 12 ++----------
>   test/engine/null.result   | 24 ++++++++++++++++++++++++
>   test/engine/null.test.lua |  9 +++++++++
>   3 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 5f4899d..d4af54c 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -38,7 +38,7 @@ static intptr_t recycled_format_ids = FORMAT_ID_NIL;
>   static uint32_t formats_size = 0, formats_capacity = 0;
>   
>   static const struct tuple_field tuple_field_default = {
> -	FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, false,
> +	FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, true,
>   };
>   
>   /**
> @@ -81,15 +81,7 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
>   			assert(part->fieldno < format->field_count);
>   			struct tuple_field *field =
>   				&format->fields[part->fieldno];
> -			if (part->fieldno >= field_count) {
> -				field->is_nullable = part->is_nullable;
> -			} else if (field->is_nullable != part->is_nullable) {
> -				/*
> -				 * In case of mismatch set the most
> -				 * strict option for is_nullable.
> -				 */
> -				field->is_nullable = false;
> -			}
> +			field->is_nullable &= part->is_nullable;

I have changed it to

     field->is_nullable = field->is_nullable && part->is_nullable;

because it is rather logical operation (&&) than bitwise (&). I've
force pushed the fix. Other part of the patch LGTM.

Vova, please, review. If you will push it, be careful during merge
into 2.1 - field->nullable_action appears there and it may have
the same bug.

Re: [tarantool-patches] [PATCH v1 1/1] box: wrong is_nullable for multiple indexes

[Vladimir Davydov]

On Fri, Nov 02, 2018 at 01:29:19PM +0300, Vladislav Shpilevoy wrote:
> Vova, please, review. If you will push it, be careful during merge
> into 2.1 - field->nullable_action appears there and it may have
> the same bug.

Pushed to 1.10. I'll merge it to 2.1 next week, when there are enough
patches stacked.