From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 647A4430408 for ; Fri, 14 Aug 2020 17:51:48 +0300 (MSK) From: Ilya Kosarev Date: Fri, 14 Aug 2020 17:51:43 +0300 Message-Id: <20200814145143.23720-1-i.kosarev@tarantool.org> Subject: [Tarantool-patches] [PATCH v3] tuple: drop extra restrictions for multikey index List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: korablev@tarantool.org Cc: tarantool-patches@dev.tarantool.org Multikey index did not work properly with nullable root field in tuple_raw_multikey_count(). Now it is fixed and corresponding restrictions are dropped. This also means that we can drop implicit nullability update for array/map fields and make all fields nullable by default, as it was until e1d3fe8ab8eed65394ad17409401a93b6fcdc435 (tuple format: don't allow null where array/map is expected), as far as default non-nullability itself doesn't solve any real problems while providing confusing behavior (gh-5027). Follow-up #5027 Closes #5192 @TarantoolBot document Title: tuple: allow nullable multikey index root Update the documentation for space_object:create_index() to reflect that it is now safe for multikey index root to be nullable. Previously it was not safe to insert, for example, box.NULL as an array field which elements are the part of multikey index due to error in counter of multikey index keys in tuple. It was partly fixed using default non-nullability for tuple fields. In 2.3.3 the restriction on nullable multikey index root was introduced. Now the counter problem is fixed and all tuple fields are nullable by default as before 2.2.1. --- Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions Issue: https://github.com/tarantool/tarantool/issues/5192 @ChangeLog: * Dropped restrictions on nullable multikey index root. They were introduced due to inaccuracy in multikey index realization. It is now fixed. Also all fields are now nullable by default as it was before 2.2.1 (gh-5192). Changes in v2: - removed insignificant changes - fixed tests comments Changes in v3: - fixed some nits - added docbot request src/box/memtx_space.c | 8 --- src/box/tuple.c | 5 +- src/box/tuple_format.c | 20 ------ src/box/vinyl.c | 8 --- test/engine/gh-5027-fields-nullability.result | 63 ++----------------- .../gh-5027-fields-nullability.test.lua | 27 ++------ .../gh-5192-multikey-root-nullability.result | 59 +++++++++++++++++ ...gh-5192-multikey-root-nullability.test.lua | 19 ++++++ test/engine/json.result | 20 +++--- test/engine/json.test.lua | 6 +- test/engine/multikey.result | 8 +-- test/engine/multikey.test.lua | 4 +- 12 files changed, 111 insertions(+), 136 deletions(-) create mode 100644 test/engine/gh-5192-multikey-root-nullability.result create mode 100644 test/engine/gh-5192-multikey-root-nullability.test.lua diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index 8452ab4303..d30ce44b82 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -604,14 +604,6 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def) return -1; } } - if (key_def->is_multikey && - key_def->multikey_fieldno < space->def->field_count && - space->def->fields[key_def->multikey_fieldno].is_nullable) { - diag_set(ClientError, ER_UNSUPPORTED, - "multikey index", - "nullable root field"); - return -1; - } switch (index_def->type) { case HASH: if (! index_def->opts.is_unique) { diff --git a/src/box/tuple.c b/src/box/tuple.c index e77753a5ba..f3965476ea 100644 --- a/src/box/tuple.c +++ b/src/box/tuple.c @@ -555,7 +555,10 @@ tuple_raw_multikey_count(struct tuple_format *format, const char *data, NULL, MULTIKEY_NONE); if (array_raw == NULL) return 0; - assert(mp_typeof(*array_raw) == MP_ARRAY); + enum mp_type type = mp_typeof(*array_raw); + if (type == MP_NIL) + return 0; + assert(type == MP_ARRAY); return mp_decode_array(&array_raw); } diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index bae6c67cda..9b817d3cf7 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -242,24 +242,6 @@ tuple_field_ensure_child_compatibility(struct tuple_field *parent, return 0; } -/** - * Tuple fields are nullable by default. However, it is not ok - * for array/map fields, as far as their implicit nullability - * might break field accessors expectations, provide confusing - * error messages and cause incorrect behaviour of - * tuple_multikey_count(). Thus array/map fields have to be - * non-nullable by default, which means we have to update default - * nullability for them. - */ -static void -tuple_field_update_nullability(struct tuple_field *field) -{ - if ((field->type == FIELD_TYPE_ARRAY || - field->type == FIELD_TYPE_MAP) && - tuple_field_is_nullable(field)) - field->nullable_action = ON_CONFLICT_ACTION_DEFAULT; -} - /** * Given a field number and a path, add the corresponding field * to the tuple format, allocating intermediate fields if @@ -335,13 +317,11 @@ tuple_format_add_field(struct tuple_format *format, uint32_t fieldno, parent->offset_slot = *current_slot; } } - tuple_field_update_nullability(parent); parent->is_key_part = true; next->is_multikey_part = is_multikey; parent = next; token_count++; } - tuple_field_update_nullability(parent); /* * The path has already been verified by the * key_def_decode_parts function. diff --git a/src/box/vinyl.c b/src/box/vinyl.c index 32301d7bab..5fa3ea3a54 100644 --- a/src/box/vinyl.c +++ b/src/box/vinyl.c @@ -658,14 +658,6 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def) diag_set(ClientError, ER_NULLABLE_PRIMARY, space_name(space)); return -1; } - if (key_def->is_multikey && - key_def->multikey_fieldno < space->def->field_count && - space->def->fields[key_def->multikey_fieldno].is_nullable) { - diag_set(ClientError, ER_UNSUPPORTED, - "multikey index", - "nullable root field"); - return -1; - } /* Check that there are no ANY, ARRAY, MAP parts */ for (uint32_t i = 0; i < key_def->part_count; i++) { struct key_part *part = &key_def->parts[i]; diff --git a/test/engine/gh-5027-fields-nullability.result b/test/engine/gh-5027-fields-nullability.result index 1121727f6f..1e2709a24e 100644 --- a/test/engine/gh-5027-fields-nullability.result +++ b/test/engine/gh-5027-fields-nullability.result @@ -41,79 +41,26 @@ _ = s:create_index('i1', {parts={{1, 'unsigned'}}}) _ = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}}) | --- | ... -s:replace{1} +s:replace{1} -- error | --- | - error: Tuple field 5 required by space format is missing | ... -s:replace{1, box.NULL} +s:replace{1, box.NULL} -- error | --- | - error: Tuple field 5 required by space format is missing | ... -s:replace{1, box.NULL, box.NULL} +s:replace{1, box.NULL, box.NULL} -- error | --- | - error: Tuple field 5 required by space format is missing | ... -s:replace{1, box.NULL, box.NULL, box.NULL} +s:replace{1, box.NULL, box.NULL, box.NULL} -- error | --- | - error: Tuple field 5 required by space format is missing | ... -s:replace{1, box.NULL, box.NULL, box.NULL, 5} +s:replace{1, box.NULL, box.NULL, box.NULL, 5} -- ok | --- | - [1, null, null, null, 5] | ... s:drop() | --- | ... - -s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) - | --- - | ... -_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}}) - | --- - | ... -_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) - | --- - | ... -s:replace{1, box.NULL} - | --- - | - [1, null] - | ... -_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) - | --- - | - error: multikey index does not support nullable root field - | ... -s:replace{2, box.NULL} - | --- - | - [2, null] - | ... -s:drop() - | --- - | ... - -s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) - | --- - | ... -_ = s:format({{name='id'}, {name='data', type='array'}}) - | --- - | ... -_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) - | --- - | ... -s:replace{1, box.NULL} - | --- - | - error: 'Tuple field 2 type does not match one required by operation: expected array' - | ... -_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) - | --- - | ... -s:replace{2, box.NULL} - | --- - | - error: 'Tuple field 2 type does not match one required by operation: expected array' - | ... -s:replace{3, {}} - | --- - | - [3, []] - | ... -s:drop() - | --- - | ... diff --git a/test/engine/gh-5027-fields-nullability.test.lua b/test/engine/gh-5027-fields-nullability.test.lua index 960103d6c4..8553d488c0 100644 --- a/test/engine/gh-5027-fields-nullability.test.lua +++ b/test/engine/gh-5027-fields-nullability.test.lua @@ -12,26 +12,9 @@ s:drop() s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) _ = s:create_index('i1', {parts={{1, 'unsigned'}}}) _ = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}}) -s:replace{1} -s:replace{1, box.NULL} -s:replace{1, box.NULL, box.NULL} -s:replace{1, box.NULL, box.NULL, box.NULL} -s:replace{1, box.NULL, box.NULL, box.NULL, 5} -s:drop() - -s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) -_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}}) -_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) -s:replace{1, box.NULL} -_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) -s:replace{2, box.NULL} -s:drop() - -s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) -_ = s:format({{name='id'}, {name='data', type='array'}}) -_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) -s:replace{1, box.NULL} -_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) -s:replace{2, box.NULL} -s:replace{3, {}} +s:replace{1} -- error +s:replace{1, box.NULL} -- error +s:replace{1, box.NULL, box.NULL} -- error +s:replace{1, box.NULL, box.NULL, box.NULL} -- error +s:replace{1, box.NULL, box.NULL, box.NULL, 5} -- ok s:drop() diff --git a/test/engine/gh-5192-multikey-root-nullability.result b/test/engine/gh-5192-multikey-root-nullability.result new file mode 100644 index 0000000000..2a9cd66ed6 --- /dev/null +++ b/test/engine/gh-5192-multikey-root-nullability.result @@ -0,0 +1,59 @@ +-- test-run result file version 2 +test_run = require('test_run').new() + | --- + | ... + +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) + | --- + | ... +_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}}) + | --- + | ... +_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) + | --- + | ... +s:replace{1, box.NULL} -- ok + | --- + | - [1, null] + | ... +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) + | --- + | ... +s:replace{2, box.NULL} -- ok + | --- + | - [2, null] + | ... +_ = s:delete(2) + | --- + | ... +s:drop() + | --- + | ... + +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) + | --- + | ... +_ = s:format({{name='id'}, {name='data', type='array'}}) + | --- + | ... +_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) + | --- + | ... +s:replace{1, box.NULL} -- error + | --- + | - error: 'Tuple field 2 type does not match one required by operation: expected array' + | ... +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) + | --- + | ... +s:replace{2, box.NULL} -- error + | --- + | - error: 'Tuple field 2 type does not match one required by operation: expected array' + | ... +s:replace{3, {}} -- ok + | --- + | - [3, []] + | ... +s:drop() + | --- + | ... diff --git a/test/engine/gh-5192-multikey-root-nullability.test.lua b/test/engine/gh-5192-multikey-root-nullability.test.lua new file mode 100644 index 0000000000..6eb0eaf059 --- /dev/null +++ b/test/engine/gh-5192-multikey-root-nullability.test.lua @@ -0,0 +1,19 @@ +test_run = require('test_run').new() + +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) +_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}}) +_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) +s:replace{1, box.NULL} -- ok +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) +s:replace{2, box.NULL} -- ok +_ = s:delete(2) +s:drop() + +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) +_ = s:format({{name='id'}, {name='data', type='array'}}) +_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) +s:replace{1, box.NULL} -- error +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) +s:replace{2, box.NULL} -- error +s:replace{3, {}} -- ok +s:drop() diff --git a/test/engine/json.result b/test/engine/json.result index 2175266fcc..b8fd9a1b69 100644 --- a/test/engine/json.result +++ b/test/engine/json.result @@ -738,12 +738,11 @@ _ = s:create_index('sk', {parts = {{'[2][1].a', 'unsigned'}}}) ... s:insert{1, box.NULL} -- error --- -- error: 'Tuple field 2 type does not match one required by operation: expected array' +- error: Tuple field [2][1]["a"] required by space format is missing ... s:insert{2, {box.NULL}} -- error --- -- error: 'Tuple field [2][1] type does not match one required by operation: expected - map' +- error: Tuple field [2][1]["a"] required by space format is missing ... s:insert{3} -- error --- @@ -764,16 +763,15 @@ s:insert{6, {{a = 1}}} -- ok s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}} --- ... -s:insert{7, box.NULL} -- error +s:insert{7, box.NULL} -- ok --- -- error: 'Tuple field 2 type does not match one required by operation: expected array' +- [7, null] ... -s:insert{8, {box.NULL}} -- error +s:insert{8, {box.NULL}} -- ok --- -- error: 'Tuple field [2][1] type does not match one required by operation: expected - map' +- [8, [null]] ... --- Skipping nullable fields is okay though. +-- Skipping nullable fields is also okay. s:insert{9} -- ok --- - [9] @@ -792,7 +790,9 @@ s:insert{12, {{a = box.NULL}}} -- ok ... s.index.sk:select() --- -- - [9] +- - [7, null] + - [8, [null]] + - [9] - [10, []] - [11, [{'b': 1}]] - [12, [{'a': null}]] diff --git a/test/engine/json.test.lua b/test/engine/json.test.lua index 4771c3162a..371bbad91d 100644 --- a/test/engine/json.test.lua +++ b/test/engine/json.test.lua @@ -220,9 +220,9 @@ s:insert{4, {}} -- error s:insert{5, {{b = 1}}} -- error s:insert{6, {{a = 1}}} -- ok s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}} -s:insert{7, box.NULL} -- error -s:insert{8, {box.NULL}} -- error --- Skipping nullable fields is okay though. +s:insert{7, box.NULL} -- ok +s:insert{8, {box.NULL}} -- ok +-- Skipping nullable fields is also okay. s:insert{9} -- ok s:insert{10, {}} -- ok s:insert{11, {{b = 1}}} -- ok diff --git a/test/engine/multikey.result b/test/engine/multikey.result index 968be4cc3b..ff72983ecd 100644 --- a/test/engine/multikey.result +++ b/test/engine/multikey.result @@ -794,9 +794,9 @@ _ = s:create_index('pk') _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}}) --- ... -s:insert{1, box.NULL} -- error +s:insert{1, box.NULL} -- ok --- -- error: 'Tuple field 2 type does not match one required by operation: expected array' +- [1, null] ... s:insert{2, {box.NULL}} -- error --- @@ -814,9 +814,9 @@ s:insert{4, {1}} -- ok s.index.sk:alter{parts = {{'[2][*]', 'unsigned', is_nullable = true}}} --- ... -s:insert{5, box.NULL} -- still error +s:insert{5, box.NULL} -- ok --- -- error: 'Tuple field 2 type does not match one required by operation: expected array' +- [5, null] ... s:insert{6, {box.NULL}} -- ok --- diff --git a/test/engine/multikey.test.lua b/test/engine/multikey.test.lua index ed70334949..653a9c9b17 100644 --- a/test/engine/multikey.test.lua +++ b/test/engine/multikey.test.lua @@ -214,12 +214,12 @@ s:drop() s = box.schema.space.create('test', {engine = engine}) _ = s:create_index('pk') _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}}) -s:insert{1, box.NULL} -- error +s:insert{1, box.NULL} -- ok s:insert{2, {box.NULL}} -- error s:insert{3, {}} -- ok s:insert{4, {1}} -- ok s.index.sk:alter{parts = {{'[2][*]', 'unsigned', is_nullable = true}}} -s:insert{5, box.NULL} -- still error +s:insert{5, box.NULL} -- ok s:insert{6, {box.NULL}} -- ok s:insert{7, {}} -- ok s:insert{8, {2}} -- ok -- 2.17.1