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

Vladimir Davydov vdavydov.dev at gmail.com
Mon Oct 15 13:08:15 MSK 2018

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

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.

More information about the Tarantool-patches mailing list