From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 15 Oct 2018 13:08:15 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH v2 1/2] Add space names cache Message-ID: <20181015100815.n4oxc3d4sv4ids7m@esperanza> References: <32338133284dcb28db1b85654680cdece188904a.1539344128.git.kyukhin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <32338133284dcb28db1b85654680cdece188904a.1539344128.git.kyukhin@tarantool.org> To: Kirill Yukhin Cc: korablev@tarantool.org, tarantool-patches@freelists.org List-ID: 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 _by_name and _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 _cache_find and _find, which typically work as wrappers around _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 _by_id and _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.