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

[Mergen Imeev <imeevma@tarantool.org>]

On Mon, Dec 07, 2020 at 02:57:39PM +0000, Nikita Pettik wrote:
> 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.
>  
That is partly so. These segfaults appeared in issue #5537. However,
there is no stable reproducer. These segfaults appeared due to no diag
is set in os_unix.c when open(), write() and other such functions fails.
I have no idea how to make stable reproducer for this beside using error
injection.

> > >  
> > > > 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
> > > >