Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove unecessary execute of space_cache_find()

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for the review. My answers below. Sorry that I didn't
gave you anough information about why this fix is actually needed.

The main idea is that the problem #5592 is the same as #5537, however
it isn't obvoius for now. This patch makes #5592 and #5537 identical.
A bit more below.

On Sat, Dec 05, 2020 at 06:06:26PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 05.12.2020 10:35, imeevma@tarantool.org wrote:
> > Due to the fact that space_cache_find () is called unnecessarily, it is
> > possible to set diag "Space '0' does not exist", although in this case
> > it is not a wrong situation when the space id is 0.
> > 
> > Part of #5592
> > ---
> > https://github.com/tarantool/tarantool/issues/5592
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-5592-remove-unnecessary-diag-set
> > 
> >  src/box/sql/where.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> > index 0d7590f0e..65d4197f2 100644
> > --- a/src/box/sql/where.c
> > +++ b/src/box/sql/where.c
> > @@ -4581,7 +4581,7 @@ sqlWhereBegin(Parse * pParse,	/* The parser context */
> >  		struct SrcList_item *pTabItem = &pTabList->a[pLevel->iFrom];
> >  		struct space_def *space_def = pTabItem->space->def;
> >  		pLoop = pLevel->pWLoop;
> > -		struct space *space = space_cache_find(space_def->id);
> > +		struct space *space = pTabItem->space;
> 
> Please, provide a test. It looks like a bug, and it is worth fixing,
> but I don't understand how an excess diag can matter here. And if it
> matters, it seems not to be covered with a test.
> 
I have no idea how to test this. However, this problem appeared in a
rather unexpected place: there is a problem #5537, where in some cases
we get a segfault because we do not set diag in some places in
os_unix.c. Also, sometimes we get a "Space '0' does not exist" error.
For a while, we had no idea that the two problems were related.

I mean: diag_set() in place where it shouldn't be + (sometimes) no
diag_set() in place where it should be = unexpected error.

Should I think about test here? I think it is possible to test using
error injections, but not sure if this is worth it.

> If parsing and VDBE build return 0, we don't look at diag. At least we
> should not. Therefore this diag_set should not matter.
That is usually true, except there is some cases when there is no
diag_set() in case of error. This happened in issue #5537.

> 
> >  		if (space_def->id == 0 || space_def->opts.is_view) {
> >  			/* Do nothing */
> >  		} else if ((pLoop->wsFlags & WHERE_IDX_ONLY) == 0 &&
> Also I suggest to get a second review from Nikita, because he knows
> SQL code better than Leonid.
Thanks, I will. I included Leonid since he works with issue #5537.