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

[Sergey Petrenko <sergepetrenko@tarantool.org>]

Hi! Thank you for review. I fixed your comments.
Please find the new diff below.

> 8 авг. 2018 г., в 15:50, Vladislav Shpilevoy <v.shpilevoy@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()
-- 
2.15.2 (Apple Git-101.1)