From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 25 Jul 2019 11:38:17 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH 1/3] vinyl: use update_def index method to update vy_lsm on ddl Message-ID: <20190725083817.GD24631@esperanza> References: <20a1aafbfba6068f17a07b01e4c829db0d429fe0.1563895921.git.vdavydov.dev@gmail.com> <20190724222500.GA13150@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190724222500.GA13150@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Thu, Jul 25, 2019 at 01:25:01AM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [19/07/23 18:49]: > > When an index definition is modified by DDL in such a way that doesn't > > require index rebuild (e.g. a key part type is changed from unsigned to > > integer), we move the index from the old space container to the new one, > > see ModifyIndex. In case of Vinyl we also need to update key definitions > > and options stored in vy_lsm. We do that in swap_index space method, but > > this isn't entirely correct, as this method is also called when an index > > is moved without any modifications, see MoveIndex. Let's do this from > > update_def index method instead, which seems to be more suitable. > > > > The only reason this code lives in swap_index space method now is that > > we didn't have update_def index method when this functionality was > > introduced in the first place. > > First of all, it is extremely annoying that instead of testing > this properly despite my humble, repeated requests we waited for > these bugs to surface and now address them with urgent > patches. Well, that's how things typically go. We write code, we find bugs, we fix them. It's impossible to write bug-free code. Tests can reduce the possibility of a bug, but they aren't a silver bullet, especially when written by a developer whose goal is to commit a feature ;) Regarding this particular bug, to tell you the truth, I knew it existed when I submitted the original patch (it was me who filed the ticket after all), but I also knew that it could be fixed without redesign so I proceeded with commit ;) I often deliberately commit buggy code, because it's easier to proceed knowing that the design is agreed upon and committed, easier to build on top of that. > > I see if I could find anything wrong with this patch but I can't. > > > +static struct key_def * > > +key_def_do_copy(struct key_def *res, const struct key_def *src, size_t sz) > > I personally prefer _impl to do_. NP, will rename if you like.