From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 7629028230 for ; Mon, 13 Aug 2018 09:42:25 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id urShx1Bv9LdI for ; Mon, 13 Aug 2018 09:42:25 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9D19E27436 for ; Mon, 13 Aug 2018 09:42:24 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Allow nullability mismatch in space indices and format. References: <20180807144829.34260-1-sergepetrenko@tarantool.org> <8702b347-f250-8f9f-3571-17013e63d8c4@tarantool.org> From: Vladislav Shpilevoy Message-ID: <44a12fca-129a-b3b3-cc75-cdf388bcc649@tarantool.org> Date: Mon, 13 Aug 2018 16:42:22 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Sergey Petrenko Cc: tarantool-patches@freelists.org 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 написал(а): >> >> 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:
> - 'box.error.FUNCTION_TX_ACTIVE : 30' > + - 'box.error.injection : table:
> - '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() >