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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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.

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.

 - 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 */

 - I guess that space_cache_replace shouldn't return anything.

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

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