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 BE6A0294EA for ; Mon, 27 Aug 2018 03:37:17 -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 imddt6WE18sa for ; Mon, 27 Aug 2018 03:37:17 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 7BECC294AA for ; Mon, 27 Aug 2018 03:37:16 -0400 (EDT) From: Kirill Shcherbatov Subject: [tarantool-patches] Re: [PATCH v2 2/5] box: introduce slot_cache in key_part References: <79ae3b382f52fe1b38d21542a1d34079c0e68934.1534332920.git.kshcherbatov@tarantool.org> Message-ID: <40d4f349-d660-88c9-a125-f352905b5b72@tarantool.org> Date: Mon, 27 Aug 2018 10:37:14 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: tarantool-patches@freelists.org, Vladislav Shpilevoy > 1. As I remember, we already had this talk during SQL triggers > development, but lets repeat. Please, do not put any operation > specific things onto the main path. Either use ModifySpace, > ModifyIndex, RebuildIndex, CreateIndex, DropIndex etc classes, > or change ModifySpace so that it can be called from _index > trigger, or introduce ModifySpaceFormat or something. > Alter_space_do should not be changed. Done. I've introduced new ModifySpaceFormat that is called at the end on_replace_index_trigger. > 2. I do not see where do you update cmp_def in this file. Exactly > cmp_def is used in comparators, not key_def. key_def_merge makes key_def_set_part that setup invalid epoch 0 for every key part. > 3. From these comments and names it is very hard to understand > anything. > 3.1. When I grep 'slot_offset' I see the only match here. For a > novice it would be difficult to find its origin. > 3.2. I believe key_part should not depend on a format even in > member names. Please, rename 'format_epoch' to offset_slot_epoch. > 3.3. slot_cache -> offset_slot. > 3.4. In comments, please, describe what is epoch, where are > offset_slot and epoch from? How epoch works? @@ -74,10 +74,17 @@ struct key_part { + /** + * Epoch of offset slot cache. Initialized with + * incremental epoch of format on caching it's field's + * offset_slot via tuple_field_by_part_raw to speed up + * access on subsequent calls with same format. + * Cache use "the eldest format is most relevant" + * strategy. + */ + uint64_t offset_slot_epoch; + /** Cache with format's field offset slot. */ + int32_t offset_slot; struct tuple_format { /** + * Counter that grows incrementally on space rebuild if + * format has other distribution of offset slots comparing + * with previous one. */ uint64_t epoch; > 4. This SQLite style of NULLs handling is not ok, I think. Just > do not call this function, when the format is NULL. Ok, fixed. > 5. This function is a tuple format method and should not take > a space in the arguments. Please, pass the format without space > intermediation, and put it into tuple_format.c/.h. > 6. Parameter name and description shall not be mixed. > 7. Typo: 'epo'. Ok, fixed. > 8. List of which index_defs? Old, new? Besides, > as I see in the alter code, neither of them. Alter > can drop some index_defs from this list before you > update the space_format, for example, via > DropIndex::alter_defList of new space keys. I need the same keys that was used to build new space. > 9. Do not call tuple_format on each iteration. It can not change > in between. Same about tuple_data, tuple_field_map. As I said in > the previous review, these functions are not free. + struct tuple_format *tuple_a_format = tuple_format(tuple_a); + struct tuple_format *tuple_b_format = tuple_format(tuple_b); + const char *tuple_a_raw = tuple_data(tuple_a); + const char *tuple_b_raw = tuple_data(tuple_b); + const uint32_t *tuple_a_field_map = tuple_field_map(tuple_a); + const uint32_t *tuple_b_field_map = tuple_field_map(tuple_b); for (i = 0; i < key_def->part_count; i++) { struct key_part *part = (struct key_part *)&key_def->parts[i]; const char *field_a = - tuple_field_by_part(tuple_format(tuple_a), - tuple_data(tuple_a), - tuple_field_map(tuple_a), - part); + tuple_field_by_part(tuple_a_format, tuple_a_raw, + tuple_a_field_map, part); const char *field_b = - tuple_field_by_part(tuple_format(tuple_b), - tuple_data(tuple_b), - tuple_field_map(tuple_b), - part); + tuple_field_by_part(tuple_b_format, tuple_b_raw, + tuple_b_field_map, part); > 10. As you see, any function retrieving tuple field from const char * has > suffix '_raw'. Please, rename tuple_field_by_part to tuple_field_by_part_raw. const char * -tuple_field_by_part(const struct tuple_format *format, const char *data, - const uint32_t *field_map, struct key_part *part); +tuple_field_by_part_raw(const struct tuple_format *format, const char *data, + const uint32_t *field_map, struct key_part *part); > 10. Broken indentation. It is not a single place. Please, fix in others too. Fixed. > 11. All the tree parameter names (def, idx, tuple) do not > exist in this function. Done. > 12. Please, start a sentence with a capital letter and finish > with the dot. In other places and commits too. Ok, I'll try. > 13. Why do you need this cast to struct key_part *? I removed it > and nothing changed. > 14. Why do you need non-const struct key_part in > tuple_field_by_part? Because I am going to update key_part offset_slot and offset_slot_epoch in tuple_field_by_part_raw routine. > 15. Please, do not call tuple_format, tuple_data and tuple_field_map > multiple times. Done, similar to 9.