[tarantool-patches] Re: [PATCH 1/3] vinyl: use update_def index method to update vy_lsm on ddl

[Vladimir Davydov]

On Thu, Jul 25, 2019 at 01:25:01AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev at gmail.com> [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.