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

[Nikita Pettik <korablev@tarantool.org>]

On 07 Dec 17:28, Mergen Imeev wrote:
> Hi! Thank you for the review. My answer below.
> 
> On Mon, Dec 07, 2020 at 02:17:24PM +0000, Nikita Pettik wrote:
> > On 05 Dec 12:35, Mergen Imeev via Tarantool-patches 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.
> > 
> > Patch is OK but I don't understand how could it be flaky...
> > I mean calling space_cache_find() is deterministic (at least here: query plan
> > shouldn't change from call to call). So I suggest to put assert(space != NULL)
> > after space_cache_find(), then run test case (as I understand it is query from
> > Mike Siomkin) and make sure that it always fails. After that, apply your
> > patch and verify that it no longer fails.
> It was already done. However, instead of error we now getting segfault
> due to no diag set. And it is right. So, after this patch #5592 becomes
> #5537. Fix for #5537 already in work and will be done by Leonid.

So you didn't include test because with fix being applied it results
in segfault (due to missing diag somewhere in vdbe internals). Is this true?
If so, could you please attach ready-to-copy-paste test case so that I
verify it myself (just in case)? After that, I guess I can push patch
to all branches.
 
> >  
> > > 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;
> > >  		if (space_def->id == 0 || space_def->opts.is_view) {
> > >  			/* Do nothing */
> > >  		} else if ((pLoop->wsFlags & WHERE_IDX_ONLY) == 0 &&
> > > -- 
> > > 2.25.1
> > >