<HTML><BODY><div><div> Hi!<br> </div><div>Thanks for the review.</div><div> </div><div>Send v2 of the the patch considering your comments, answers below.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Суббота, 4 июля 2020, 0:49 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15938129540306309762_BODY">Hi! Thanks for the patch!<br><br>See 2 comments below.<br><br>On 03/07/2020 15:32, Ilya Kosarev wrote:<br>> Since 9fba29abb4e05babb9b23b4413bd8083f0fba933 (memtx: introduce tuple<br>> compare hint) index_get uses key_hint to get a comparison hint for the<br>> key. It requires key type to be validated. However in<br>> space_before_replace index_get was used without key validation on some<br>> execution paths. Now it is fixed and validation is being performed if<br>> needed. Corresponding test case is introduced.<br>><br>> Closes #5093<br>> ---<br>> Branch: <a href="https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5093-before_replace-key-validation" target="_blank">https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5093-before_replace-key-validation</a><br>> Issue: <a href="https://github.com/tarantool/tarantool/issues/5093" target="_blank">https://github.com/tarantool/tarantool/issues/5093</a><br>><br>> @ChangeLog:<br>> * Add needed key validation to space_before_replace.<br>><br>> src/box/space.c | 6 +++---<br>> .../gh-5093-before_replace-key-type.result | 21 +++++++++++++++++++<br>> .../gh-5093-before_replace-key-type.test.lua | 7 +++++++<br>> 3 files changed, 31 insertions(+), 3 deletions(-)<br>> create mode 100644 test/engine/gh-5093-before_replace-key-type.result<br>> create mode 100644 test/engine/gh-5093-before_replace-key-type.test.lua<br>><br>> diff --git a/src/box/space.c b/src/box/space.c<br>> index eecbde7fa7..66768208b3 100644<br>> --- a/src/box/space.c<br>> +++ b/src/box/space.c<br>> @@ -359,9 +359,6 @@ space_before_replace(struct space *space, struct txn *txn,<br>> return -1;<br>> key = request->key;<br>> part_count = mp_decode_array(&key);<br>> - if (exact_key_validate(index->def->key_def,<br>> - key, part_count) != 0)<br>> - return -1;<br>> break;<br>> case IPROTO_INSERT:<br>> case IPROTO_REPLACE:<br>> @@ -380,6 +377,9 @@ space_before_replace(struct space *space, struct txn *txn,<br>> /* Unknown request type, nothing to do. */<br>> return 0;<br>> }<br>> + if (key != NULL &&<br>> + exact_key_validate(index->def->key_def, key, part_count) != 0)<br>> + return -1;<br><br>1. The patch is technically correct. But I don't like that you<br>add key != NULL check to the hot path. Even though it is always != NULL<br>except blackhole engine space, which doesn't have indexes either.<br><br>Consider the refactoring below. I know it causes bigger diff, but<br>I consider it ok for the sake performance. As we learned recently,<br>even +1 or 2 ifs in a hot path may cost.</div></div></div></div></blockquote></div><div>Right, I totally agree here, didn’t give this enough thought.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>====================<br>diff --git a/src/box/space.c b/src/box/space.c<br>index 66768208b..73172ddd2 100644<br>--- a/src/box/space.c<br>+++ b/src/box/space.c<br>@@ -347,6 +347,7 @@ space_before_replace(struct space *space, struct txn *txn,<br> const char *key = NULL;<br> uint32_t part_count = 0;<br> struct index *index = NULL;<br>+ struct tuple *old_tuple = NULL;<br> <br> /*<br> * Lookup the old tuple.<br>@@ -364,7 +365,7 @@ space_before_replace(struct space *space, struct txn *txn,<br> case IPROTO_REPLACE:<br> case IPROTO_UPSERT:<br> if (pk == NULL)<br>- break;<br>+ goto after_getting_old;<br> index = pk;<br> key = tuple_extract_key_raw(request->tuple, request->tuple_end,<br> index->def->key_def, MULTIKEY_NONE,<br>@@ -377,15 +378,11 @@ space_before_replace(struct space *space, struct txn *txn,<br> /* Unknown request type, nothing to do. */<br> return 0;<br> }<br>- if (key != NULL &&<br>- exact_key_validate(index->def->key_def, key, part_count) != 0)<br>+ if (exact_key_validate(index->def->key_def, key, part_count) != 0)<br> return -1;<br>-<br>- struct tuple *old_tuple = NULL;<br>- if (index != NULL &&<br>- index_get(index, key, part_count, &old_tuple) != 0)<br>+ if (index_get(index, key, part_count, &old_tuple) != 0)<br> return -1;<br>-<br>+after_getting_old:;<br> /*<br> * Create the new tuple.<br> */<br>====================<br><br>> diff --git a/test/engine/gh-5093-before_replace-key-type.test.lua b/test/engine/gh-5093-before_replace-key-type.test.lua<br>> new file mode 100644<br>> index 0000000000..9cba3c1597<br>> --- /dev/null<br>> +++ b/test/engine/gh-5093-before_replace-key-type.test.lua<br>> @@ -0,0 +1,7 @@<br>> +test_run = require('test_run').new()<br>> +trigger = function() end<br>> +<br>> +s = box.schema.space.create('gh-5093', {engine=test_run:get_cfg('engine')})<br>> +_ = s:create_index('value', {parts={{1, type='decimal'}}})<br>> +_ = s:before_replace(trigger)<br>> +s:replace{'1111.1111'}<br><br>2. Please, cleanup. Do s:drop().</div></div></div></div></blockquote></div><div>Right.</div><div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Ilya Kosarev</div></div></div><div> </div></div></BODY></HTML>