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 A16D128CD7 for ; Thu, 9 Aug 2018 03:09:52 -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 nYFMFiBdN5yp for ; Thu, 9 Aug 2018 03:09:52 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 3096928C29 for ; Thu, 9 Aug 2018 03:09:52 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: [tarantool-patches] Re: [PATCH] Allow nullability mismatch in space indices and format. From: Sergey Petrenko In-Reply-To: <8702b347-f250-8f9f-3571-17013e63d8c4@tarantool.org> Date: Thu, 9 Aug 2018 10:09:49 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20180807144829.34260-1-sergepetrenko@tarantool.org> <8702b347-f250-8f9f-3571-17013e63d8c4@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org Hi! Thank you for review. I fixed your comments. Please find the new diff below. > 8 =D0=B0=D0=B2=D0=B3. 2018 =D0=B3., =D0=B2 15:50, Vladislav Shpilevoy = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): >=20 > Hi! Thanks for the patch! See 8 comments below. >=20 > 1. Please, prefix the commit title with a subsystem name to > which the patch is related. (Here it is 'box: ' I think). Done. >=20 >> 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 >=3D field_count) { >> field->is_nullable =3D = part->is_nullable; >> } else if (field->is_nullable !=3D = 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; >> + /* >=20 > 2. On the branch here a leading white space is. Please, remove. >=20 > 3. Please, wrap the comment by 66 symbols. >=20 > 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 =3D 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 =3D { = 'field1' } }) >> --- >> - error: Primary index of the space 'test' can not contain nullable = parts >> ... >> -s:create_index('primary', { parts =3D {{'field1', is_nullable =3D = false}} }) >> +-- this is allowed, but the actual part is_nullable stays false >=20 > 5. Please, in comments start sentences from a capital letter and = finish > with the dot. Done.=20 >=20 >> +pk =3D s:create_index('primary', { parts =3D {{'field1', is_nullable = =3D false}} }) >> +--- >> +... >> +pk:drop() >> --- >> -- error: Field 1 is nullable in space format, but not nullable in = index parts >> ... >> format[1].is_nullable =3D 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 =3D false >> ---- >> -... >> sk =3D s:create_index('sk', { parts =3D parts }) -- Fail. >=20 > 6. Now the comment is wrong - this line does not fail. Fixed. >=20 >> --- >> -- 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 =3D true >> ---- >> -... >> -sk =3D s:create_index('sk', { parts =3D parts }) -- Ok. >> ---- >> -... >> -format[2].is_nullable =3D 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 =3D s:create_index('sk', { parts =3D 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 =3D 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 =3D box.schema.space.create('test', {engine=3Dengine}) >> +--- >> +... >> +pk =3D s:create_index('primary', {parts=3D{1, 'unsigned'}}) >> +--- >> +... >> +sk =3D s:create_index('secondary', {parts=3D{2, 'unsigned', = is_nullable=3Dfalse}}) >> +--- >> +... >> +format =3D {} >> +--- >> +... >> +format[1] =3D {name =3D 'first', type =3D 'unsigned', is_nullable =3D = false} >> +--- >> +... >> +format[2] =3D {name =3D 'second', type =3D 'unsigned', is_nullable =3D= 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} >=20 > 7. {5, nil} =3D=3D {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=E2=80=99s 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") \ =20 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 >=3D field_count) { field->is_nullable =3D = part->is_nullable; } else if (field->is_nullable !=3D = 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 =3D false; } =20 /* 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 =3D { 'field1' = } }) --- - error: Primary index of the space 'test' can not contain nullable = parts ... -s:create_index('primary', { parts =3D {{'field1', is_nullable =3D = false}} }) +-- This is allowed, but the actual part is_nullable stays false. +pk =3D s:create_index('primary', { parts =3D {{'field1', is_nullable =3D = false}} }) +--- +... +pk:drop() --- -- error: Field 1 is nullable in space format, but not nullable in index = parts ... format[1].is_nullable =3D false --- @@ -1462,9 +1465,10 @@ s:insert({1, NULL}) format[1].is_nullable =3D 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 =3D 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] =3D { name =3D 'field1', type =3D = 'unsigned', is_nullable =3D true } format[2] =3D { name =3D 'field2', type =3D 'unsigned', is_nullable =3D = true } s =3D box.schema.space.create('test', {engine =3D engine, format =3D = format}) s:create_index('primary', { parts =3D { 'field1' } }) -s:create_index('primary', { parts =3D {{'field1', is_nullable =3D = false}} }) +-- This is allowed, but the actual part is_nullable stays false. +pk =3D s:create_index('primary', { parts =3D {{'field1', is_nullable =3D = false}} }) +pk:drop() format[1].is_nullable =3D false s:format(format) s:create_index('primary', { parts =3D {{'field1', is_nullable =3D = 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 =3D true +-- The format is allowed since in primary index parts +-- is_nullable is still set to false. s:format(format) format[1].is_nullable =3D false format[2].is_nullable =3D 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 =3D false ---- -... -sk =3D s:create_index('sk', { parts =3D 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 =3D true ---- -... -sk =3D s:create_index('sk', { parts =3D parts }) -- Ok. ---- -... -format[2].is_nullable =3D 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 =3D s:create_index('sk', { parts =3D 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 =3D 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 =3D box.schema.space.create('test', {engine=3Dengine}) +--- +... +pk =3D s:create_index('primary', {parts=3D{1, 'unsigned'}}) +--- +... +sk =3D s:create_index('secondary', {parts=3D{2, 'unsigned', = is_nullable=3Dfalse}}) +--- +... +format =3D {} +--- +... +format[1] =3D {name =3D 'first', type =3D 'unsigned', is_nullable =3D = false} +--- +... +format[2] =3D {name =3D 'second', type =3D 'unsigned', is_nullable =3D = 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=3D{{2, 'unsigned', is_nullable=3Dtrue}}} = -- 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=3D{{2, 'unsigned', is_nullable=3Dfalse}}} +--- +... +format[2].is_nullable =3D 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=3D{{2, 'unsigned', is_nullable=3Dtrue}}} +--- +... +-- 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 =3D false +--- +... +s:format(format) -- Fail. +--- +- error: 'Tuple field 2 type does not match one required by operation: = expected unsigned' +... +s.index.secondary:alter{parts=3D{{2, 'unsigned', is_nullable=3Dfalse}}} = -- Fail. +--- +- error: 'Tuple field 2 type does not match one required by operation: = expected unsigned' +... +_ =3D 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=3D{{2, 'unsigned', is_nullable=3Dfalse}}} = -- 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. +_ =3D s:delete{6} +--- +... +format[2].is_nullable =3D 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 =3D true +--- +... +s:format(format) +--- +... +s.index.secondary:alter{parts=3D{{2, 'unsigned', is_nullable=3Dfalse}}} +--- +... +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 =3D 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 =3D s:create_index('pk') ok =3D pcall(s.create_index, s, 'sk', { parts =3D parts, type =3D = 'hash' }) -- Fail. ok =20 --- Conflict of is_nullable in format and in parts. -parts[1].is_nullable =3D false -sk =3D s:create_index('sk', { parts =3D parts }) -- Fail. - --- Try skip nullable in format and specify in part. -parts[1].is_nullable =3D true -sk =3D s:create_index('sk', { parts =3D parts }) -- Ok. -format[2].is_nullable =3D nil -s:format(format) -- Fail. -sk:drop() - --- Try to set nullable in part with no format. -s:format({}) sk =3D s:create_index('sk', { parts =3D parts }) --- And then set format with no nullable. -s:format(format) -- Fail. -format[2].is_nullable =3D true -s:format(format) -- Ok. =20 -- Test insert. =20 @@ -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 =3D box.schema.space.create('test', {engine=3Dengine}) +pk =3D s:create_index('primary', {parts=3D{1, 'unsigned'}}) +sk =3D s:create_index('secondary', {parts=3D{2, 'unsigned', = is_nullable=3Dfalse}}) +format =3D {} +format[1] =3D {name =3D 'first', type =3D 'unsigned', is_nullable =3D = false} +format[2] =3D {name =3D 'second', type =3D 'unsigned', is_nullable =3D = false} +s:format(format) +-- Field 2 is not nullable. +s:insert{5} +s:insert{5, box.NULL} + +s.index.secondary:alter{parts=3D{{2, 'unsigned', is_nullable=3Dtrue}}} = -- This is allowed. +-- Without space format setting this fails. +s:insert{5, box.NULL} +s:insert{5} + +s.index.secondary:alter{parts=3D{{2, 'unsigned', is_nullable=3Dfalse}}} +format[2].is_nullable =3D 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=3D{{2, 'unsigned', is_nullable=3Dtrue}}} +-- 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 =3D false +s:format(format) -- Fail. +s.index.secondary:alter{parts=3D{{2, 'unsigned', is_nullable=3Dfalse}}} = -- Fail. +_ =3D s:delete{5} +s:format(format) -- Still fail. +s.index.secondary:alter{parts=3D{{2, 'unsigned', is_nullable=3Dfalse}}} = -- Still fail. +-- Now check we can set nullability to false step by step. +_ =3D s:delete{6} + +format[2].is_nullable =3D false +s:format(format) +s:insert{5, box.NULL} -- Fail. +s:insert{5} -- Fail. +format[2].is_nullable =3D true +s:format(format) +s.index.secondary:alter{parts=3D{{2, 'unsigned', is_nullable=3Dfalse}}} +s:insert{5, box.NULL} -- Fail. +s:insert{5} -- Fail. +format[2].is_nullable =3D false +s:format(format) +s:select{} +s:insert{5} -- Fail. +s:insert{9, 10} -- Success. + +s:drop() --=20 2.15.2 (Apple Git-101.1)