Re: [Tarantool-patches] [PATCH v3 11/13] txm: introduce txm_story

[Aleksandr Lyapunov <alyapunov@tarantool.org>]

Hello, thank for the review!

On 7/16/20 3:20 AM, Vladislav Shpilevoy wrote:
> 1. Why is the hash called 'history', but the objects are 'story'?
> Or as 'history' you mean a list of stories of the same key in one
> index?
I mean history is a set of all stories.
Like a big book with a lot of paragraphs and stories about things in time.
> 2. difference -> different.
thanks
>
> 3. The iterator is initialized and assigned to something, but is
> never used for anything meaningful. The same for all_stories list.
> At least on top of this commit.
Implemented garbage collector.
> +			       cord_slab_cache(), item_size);
> 4. Is it correct you also create a pool for 0 index count?
> Why?
>
> Probably would be much simpler to just create one mempool with
> objects having BOX_INDEX_MAX records in them. It would also solve
> the DDL problem. At least partially.
>
> Can the story objects be allocated on txn's region? AFAIU,
> stories are created by transactions, and can't outlive their
> creator.
Discussed in call, that would be too much memory wasted.
>
> 5. Diag OOM takes size, allocator, and variable name. Here the
> allocator is "mempool_alloc", and the variable name is "story".
> Lets be consistent with other diag_sets. The same for all the
> other new places.
OK
>
> 6. I know story is NULL, but why not to return NULL explicitly?
> This looks confusing.
OK
>
> 7. Nit: would be better to call tuple_ref() right near this assignment.
> For the sake of OOM handler below, worth moving this line down to the
> tuple_ref() call below.
The hash table requires tuple to be set.
>
> 8. Comments for functions should use imperative mood. The same
> in other similar places.
OK
>
> +static struct tuple *
> +txm_story_older_tuple(struct txm_story_link *link)
> +{
> +	return link->older.is_story ? link->older.story->tuple
> +				    : link->older.tuple;
> 9. Why so? If older.is_story, it means it stores a story,
> not a tuple. But you return vice versa.
I don't see any mistake. I return tuple in any case.
>
> 10. Why do you need all the new structs in txn.h if they aren't
> used out of txn.c anyway. At least some of them.
OK, I'll try to.

I have to make a pause, will return later.