Hi! Thanks for the patch!
See 2 comments below.
On 03/07/2020 15:32, Ilya Kosarev wrote:
> Since 9fba29abb4e05babb9b23b4413bd8083f0fba933 (memtx: introduce tuple
> compare hint) index_get uses key_hint to get a comparison hint for the
> key. It requires key type to be validated. However in
> space_before_replace index_get was used without key validation on some
> execution paths. Now it is fixed and validation is being performed if
> needed. Corresponding test case is introduced.
>
> Closes #5093
> ---
> Branch:
https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5093-before_replace-key-validation> Issue:
https://github.com/tarantool/tarantool/issues/5093>
> @ChangeLog:
> * Add needed key validation to space_before_replace.
>
> src/box/space.c | 6 +++---
> .../gh-5093-before_replace-key-type.result | 21 +++++++++++++++++++
> .../gh-5093-before_replace-key-type.test.lua | 7 +++++++
> 3 files changed, 31 insertions(+), 3 deletions(-)
> create mode 100644 test/engine/gh-5093-before_replace-key-type.result
> create mode 100644 test/engine/gh-5093-before_replace-key-type.test.lua
>
> diff --git a/src/box/space.c b/src/box/space.c
> index eecbde7fa7..66768208b3 100644
> --- a/src/box/space.c
> +++ b/src/box/space.c
> @@ -359,9 +359,6 @@ space_before_replace(struct space *space, struct txn *txn,
> return -1;
> key = request->key;
> part_count = mp_decode_array(&key);
> - if (exact_key_validate(index->def->key_def,
> - key, part_count) != 0)
> - return -1;
> break;
> case IPROTO_INSERT:
> case IPROTO_REPLACE:
> @@ -380,6 +377,9 @@ space_before_replace(struct space *space, struct txn *txn,
> /* Unknown request type, nothing to do. */
> return 0;
> }
> + if (key != NULL &&
> + exact_key_validate(index->def->key_def, key, part_count) != 0)
> + return -1;
1. The patch is technically correct. But I don't like that you
add key != NULL check to the hot path. Even though it is always != NULL
except blackhole engine space, which doesn't have indexes either.
Consider the refactoring below. I know it causes bigger diff, but
I consider it ok for the sake performance. As we learned recently,
even +1 or 2 ifs in a hot path may cost.