[tarantool-patches] Re: [PATCH v2 2/5] box: introduce slot_cache in key_part

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 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.