[tarantool-patches] Re: [PATCH v2 2/5] box: introduce slot_cache in key_part
Kirill Shcherbatov
kshcherbatov at tarantool.org
Mon Aug 27 10:37:14 MSK 2018
> 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.
More information about the Tarantool-patches
mailing list