Re: [tarantool-patches] [PATCH v2 1/2] Add space names cache

[Kirill Yukhin <kyukhin@tarantool.org>]

On 15 Oct 13:08, Vladimir Davydov wrote:
> On Fri, Oct 12, 2018 at 02:38:18PM +0300, Kirill Yukhin wrote:
> > Since SQL is heavily using name -> space mapping,
> > introduce (instead of scanning _space space) dedicated
> > cache, which maps space name to space pointer.
> > Update SQL sources thoroughly.
> > Remove function which deletes from cache, making replace
> > more general: it might be used for both insertions,
> > deletions and replaces.
> 
> Overall, I like the reasoning behind the patch, but IMO before doing
> this we need to cleanup the mess we have with schema object lookup
> functions.
> 
> See, we have <class>_by_name and <class>_by_id which look up an object
> of the corresponding class by the given name or id. Some of them set
> diag if the object isn't found, others not. I find it ugly. I think that
> either all or none of them should set diag.
> 
> Next, we have <class>_cache_find and <class>_find, which typically work
> as wrappers around <class>_by_id setting diag if the object isn't found.
> Sometimes (space_cache_find) they also cache the last returned object in
> a static variable. If we make <class>_by_id and <class>_by_name set
> diag, we won't be needing most of them. I think that we should only
> leave space_cache_find. May be, it's worth renaming it to make its
> purpose more obvious.
> 
> Please consider doing this cleanup first in a separate patch. It should
> be trivial. This should be done on top of 1.10 IMO.
We've discussed that verbally and came to the conclusion that its not
about time to do so. Need to evict EH first.


> Regarding the patch itself.
> 
>  - space_cache_replace should take the old space as the first argument
>    and the new space as the second argument, not vice versa. This would
>    be consistent with other 'replace' functions we have in Tarantool.

Done.

>  - Why would you need space_cache_delete and space_name_cache_delete if
>    you could use space_cache_replace instead. I mean,
> 
>    space_cache_replace(space, NULL); /* deletes the space */

Done.

>  - I guess that space_cache_replace shouldn't return anything.

Done.

>  - I think the change of space_cache_replace API should be done in a
>    separate patch.

Done.

>  - IMO you should replace box_space_id_by_name with space_by_name in a
>    separate patch.

Done.

I'll re-send updated 4-patch patchset as v2.

--
Regards, Kirill Yukhin