[PATCH v1 1/1] box: fix assert with multikey hybrid index

[PATCH v1 1/1] box: fix assert with multikey hybrid index

[Kirill Shcherbatov]

Tarantool used to assume that offset_slot has an extension
iff field_map_get_offset is called with multikey_idx >= 0.
In fact, when some part of the index contains a multikey index
placeholder, tuple_compare_* routines pass a tuple_hint in
meaning of multikey index for each tuple_field_raw_by_part call,
even for regular key_part that doesn't have array index
placeholder (and, correspondingly, field_map extension).
Thus this assumption is invalid.

This patch uses the fact that field_map slots that have extensoin
store negative offset to distinguish multikey and normal usage
of the field_map_get_offset routine.

Closes #4234
---
http://github.com/tarantool/tarantool/tree/kshch/gh-4234-hybrid-multikey-index-parts-assert
https://github.com/tarantool/tarantool/issues/4234

 src/box/field_map.h           |  4 ++--
 test/engine/multikey.result   | 27 +++++++++++++++++++++++++++
 test/engine/multikey.test.lua | 10 ++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/src/box/field_map.h b/src/box/field_map.h
index b0dfeb4e4..2e2eea035 100644
--- a/src/box/field_map.h
+++ b/src/box/field_map.h
@@ -152,8 +152,8 @@ field_map_get_offset(const uint32_t *field_map, int32_t offset_slot,
 		     int multikey_idx)
 {
 	uint32_t offset;
-	if (multikey_idx != MULTIKEY_NONE && field_map[offset_slot] > 0) {
-		assert((int32_t)field_map[offset_slot] < 0);
+	if (multikey_idx != MULTIKEY_NONE && field_map[offset_slot] > 0 &&
+	    (int32_t)field_map[offset_slot] < 0) {
 		/**
 		 * The field_map extent has the following
 		 * structure: [size=N|slot1|slot2|..|slotN]
diff --git a/test/engine/multikey.result b/test/engine/multikey.result
index 1d5d9e200..6c763b2dd 100644
--- a/test/engine/multikey.result
+++ b/test/engine/multikey.result
@@ -753,3 +753,30 @@ i2:select()
 s:drop()
 ---
 ...
+-- Hybrid multikey index definiton.
+box.cfg{}
+---
+...
+s = box.schema.space.create('clients')
+---
+...
+s:format({{name='name', type='string'}, {name='phone', type='array'}, {name='p', type='string'}})
+---
+...
+name_idx = s:create_index('name_idx', {parts = {{'name', 'string'}}})
+---
+...
+phone_idx = s:create_index('phone_idx', {parts = {{'phone[*]', 'string'}, {'p', 'string'}}, unique=false})
+---
+...
+s:insert({"Genadiy", {"911"}, 'b'})
+---
+- ['Genadiy', ['911'], 'b']
+...
+s:insert({"Jorge", {"911", "89457609234"}, 'a'})
+---
+- ['Jorge', ['911', '89457609234'], 'a']
+...
+s:drop()
+---
+...
diff --git a/test/engine/multikey.test.lua b/test/engine/multikey.test.lua
index f32f49d2b..b236938cd 100644
--- a/test/engine/multikey.test.lua
+++ b/test/engine/multikey.test.lua
@@ -194,3 +194,13 @@ s:replace{2, {{2, 3}}}
 i2 = s:create_index('sk', {parts = {{2, 'unsigned', path = '[1][*]'}}})
 i2:select()
 s:drop()
+
+-- Hybrid multikey index definiton.
+box.cfg{}
+s = box.schema.space.create('clients')
+s:format({{name='name', type='string'}, {name='phone', type='array'}, {name='p', type='string'}})
+name_idx = s:create_index('name_idx', {parts = {{'name', 'string'}}})
+phone_idx = s:create_index('phone_idx', {parts = {{'phone[*]', 'string'}, {'p', 'string'}}, unique=false})
+s:insert({"Genadiy", {"911"}, 'b'})
+s:insert({"Jorge", {"911", "89457609234"}, 'a'})
+s:drop()
-- 
2.21.0

Re: [PATCH v1 1/1] box: fix assert with multikey hybrid index

[Vladimir Davydov]

On Tue, May 21, 2019 at 06:39:53PM +0300, Kirill Shcherbatov wrote:
> Tarantool used to assume that offset_slot has an extension
> iff field_map_get_offset is called with multikey_idx >= 0.
> In fact, when some part of the index contains a multikey index
> placeholder, tuple_compare_* routines pass a tuple_hint in
> meaning of multikey index for each tuple_field_raw_by_part call,
> even for regular key_part that doesn't have array index
> placeholder (and, correspondingly, field_map extension).
> Thus this assumption is invalid.
> 
> This patch uses the fact that field_map slots that have extensoin
> store negative offset to distinguish multikey and normal usage
> of the field_map_get_offset routine.
> 
> Closes #4234
> ---
> http://github.com/tarantool/tarantool/tree/kshch/gh-4234-hybrid-multikey-index-parts-assert
> https://github.com/tarantool/tarantool/issues/4234
> 
>  src/box/field_map.h           |  4 ++--
>  test/engine/multikey.result   | 27 +++++++++++++++++++++++++++
>  test/engine/multikey.test.lua | 10 ++++++++++
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/field_map.h b/src/box/field_map.h
> index b0dfeb4e4..2e2eea035 100644
> --- a/src/box/field_map.h
> +++ b/src/box/field_map.h
> @@ -152,8 +152,8 @@ field_map_get_offset(const uint32_t *field_map, int32_t offset_slot,
>  		     int multikey_idx)
>  {
>  	uint32_t offset;
> -	if (multikey_idx != MULTIKEY_NONE && field_map[offset_slot] > 0) {
> -		assert((int32_t)field_map[offset_slot] < 0);
> +	if (multikey_idx != MULTIKEY_NONE && field_map[offset_slot] > 0 &&
> +	    (int32_t)field_map[offset_slot] < 0) {

	if (field_map[offset_slot] > 0 && (int32_t)field_map[offset_slot] < 0)

Why not simply

	if ((int32_t)field_map[offset_slot] < 0)

?



>  		/**
>  		 * The field_map extent has the following
>  		 * structure: [size=N|slot1|slot2|..|slotN]
> diff --git a/test/engine/multikey.result b/test/engine/multikey.result
> index 1d5d9e200..6c763b2dd 100644
> --- a/test/engine/multikey.result
> +++ b/test/engine/multikey.result
> @@ -753,3 +753,30 @@ i2:select()
>  s:drop()
>  ---
>  ...
> +-- Hybrid multikey index definiton.

What's 'hybrid'? Please write a comment that wouldn't raise any
questions.

> +box.cfg{}

This box.cfg{} is pointless. Please remove.

> +---
> +...
> +s = box.schema.space.create('clients')
> +---
> +...
> +s:format({{name='name', type='string'}, {name='phone', type='array'}, {name='p', type='string'}})

Format isn't necessary to reproduce the issue. Please strip the test
case of anything unnecessary. Also, please double-check that it passes
with your fix and fails without it.

> +---
> +...
> +name_idx = s:create_index('name_idx', {parts = {{'name', 'string'}}})
> +---
> +...
> +phone_idx = s:create_index('phone_idx', {parts = {{'phone[*]', 'string'}, {'p', 'string'}}, unique=false})
> +---
> +...
> +s:insert({"Genadiy", {"911"}, 'b'})
> +---
> +- ['Genadiy', ['911'], 'b']
> +...
> +s:insert({"Jorge", {"911", "89457609234"}, 'a'})
> +---
> +- ['Jorge', ['911', '89457609234'], 'a']
> +...
> +s:drop()
> +---
> +...

Re: [tarantool-patches] Re: [PATCH v1 1/1] box: fix assert with multikey hybrid index

[Kirill Shcherbatov]

Thank you for your feedback! Done.
====================================================

Tarantool used to assume that offset_slot has an extension
iff field_map_get_offset is called with multikey_idx >= 0.
In fact, when some part of the index contains a multikey index
placeholder, tuple_compare_* routines pass a tuple_hint in
meaning of multikey index for each tuple_field_raw_by_part call,
even for regular key_part that doesn't have array index
placeholder (and, correspondingly, field_map extension).
Thus this assumption is invalid.

This patch uses the fact that field_map slots that have extensoin
store negative offset to distinguish multikey and normal usage
of the field_map_get_offset routine.

Closes #4234
---
 src/box/field_map.h           |  4 ++--
 test/engine/multikey.result   | 24 ++++++++++++++++++++++++
 test/engine/multikey.test.lua | 11 +++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/src/box/field_map.h b/src/box/field_map.h
index b0dfeb4e4..a1a5a9dba 100644
--- a/src/box/field_map.h
+++ b/src/box/field_map.h
@@ -152,8 +152,8 @@ field_map_get_offset(const uint32_t *field_map, int32_t offset_slot,
 		     int multikey_idx)
 {
 	uint32_t offset;
-	if (multikey_idx != MULTIKEY_NONE && field_map[offset_slot] > 0) {
-		assert((int32_t)field_map[offset_slot] < 0);
+	if (multikey_idx != MULTIKEY_NONE &&
+	    (int32_t) field_map[offset_slot] < 0) {
 		/**
 		 * The field_map extent has the following
 		 * structure: [size=N|slot1|slot2|..|slotN]
diff --git a/test/engine/multikey.result b/test/engine/multikey.result
index 1d5d9e200..7301081fe 100644
--- a/test/engine/multikey.result
+++ b/test/engine/multikey.result
@@ -753,3 +753,27 @@ i2:select()
 s:drop()
 ---
 ...
+--
+-- gh-4234: Assert when using indexes containing both multikey
+--          and regular key_parts.
+--
+s = box.schema.space.create('clients')
+---
+...
+name_idx = s:create_index('name_idx', {parts = {{1, 'string'}}})
+---
+...
+phone_idx = s:create_index('phone_idx', {parts = {{'[2][*]', 'string'}, {3, 'string'}}, unique=false})
+---
+...
+s:insert({"Genadiy", {"911"}, 'b'})
+---
+- ['Genadiy', ['911'], 'b']
+...
+s:insert({"Jorge", {"911", "89457609234"}, 'a'})
+---
+- ['Jorge', ['911', '89457609234'], 'a']
+...
+s:drop()
+---
+...
diff --git a/test/engine/multikey.test.lua b/test/engine/multikey.test.lua
index f32f49d2b..f2392b840 100644
--- a/test/engine/multikey.test.lua
+++ b/test/engine/multikey.test.lua
@@ -194,3 +194,14 @@ s:replace{2, {{2, 3}}}
 i2 = s:create_index('sk', {parts = {{2, 'unsigned', path = '[1][*]'}}})
 i2:select()
 s:drop()
+
+--
+-- gh-4234: Assert when using indexes containing both multikey
+--          and regular key_parts.
+--
+s = box.schema.space.create('clients')
+name_idx = s:create_index('name_idx', {parts = {{1, 'string'}}})
+phone_idx = s:create_index('phone_idx', {parts = {{'[2][*]', 'string'}, {3, 'string'}}, unique=false})
+s:insert({"Genadiy", {"911"}, 'b'})
+s:insert({"Jorge", {"911", "89457609234"}, 'a'})
+s:drop()
-- 
2.21.0

[tarantool-patches] Re: [PATCH v1 1/1] box: fix assert with multikey hybrid index

[Vladislav Shpilevoy]

On 21/05/2019 19:59, Kirill Shcherbatov wrote:
> Thank you for your feedback! Done.
> ====================================================
> 
> Tarantool used to assume that offset_slot has an extension
> iff field_map_get_offset is called with multikey_idx >= 0.
> In fact, when some part of the index contains a multikey index
> placeholder, tuple_compare_* routines pass a tuple_hint in
> meaning of multikey index for each tuple_field_raw_by_part call,
> even for regular key_part that doesn't have array index
> placeholder (and, correspondingly, field_map extension).
> Thus this assumption is invalid.
> 
> This patch uses the fact that field_map slots that have extensoin

Typo "extensoin". 

> store negative offset to distinguish multikey and normal usage
> of the field_map_get_offset routine.
> 
> Closes #4234
> ---

Re: [tarantool-patches] Re: [PATCH v1 1/1] box: fix assert with multikey hybrid index

[Vladimir Davydov]

On Tue, May 21, 2019 at 07:59:37PM +0300, Kirill Shcherbatov wrote:
> +s = box.schema.space.create('clients')

It's an engine test => it should test both vinyl and memtx.

Fixed it by myself and pushed to master.

> +---
> +...
> +name_idx = s:create_index('name_idx', {parts = {{1, 'string'}}})
> +---
> +...
> +phone_idx = s:create_index('phone_idx', {parts = {{'[2][*]', 'string'}, {3, 'string'}}, unique=false})
> +---
> +...
> +s:insert({"Genadiy", {"911"}, 'b'})
> +---
> +- ['Genadiy', ['911'], 'b']
> +...
> +s:insert({"Jorge", {"911", "89457609234"}, 'a'})
> +---
> +- ['Jorge', ['911', '89457609234'], 'a']
> +...
> +s:drop()
> +---
> +...