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

[Nikita Pettik <korablev@tarantool.org>]

On 10 Dec 16:28, Mergen Imeev wrote:
> Hi! Thank you for the review. I added test. I'm sorry that I didn't
> thought about this test earlier, it was actually quite obvious.

LGTM for now. Alexander, could we push this patch to master?
Some tests are failing, but those fails seem to be unrelated to changes:
https://github.com/tarantool/tarantool/tree/imeevma/gh-5592-remove-unnecessary-diag-set
 
> 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.
> >  
> > > >  
> > > > > 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
> > > > > 
> 
> New patch:
> 
> 
> From c23af3159152f1f42d03ad779862e743c591c1ca Mon Sep 17 00:00:00 2001
> Message-Id: <c23af3159152f1f42d03ad779862e743c591c1ca.1607606743.git.imeevma@gmail.com>
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Fri, 4 Dec 2020 17:09:17 +0300
> Subject: [PATCH v1 1/1] sql: remove unecessary execute of space_cache_find()
> 
> 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
> ---
>  src/box/sql/where.c    |  2 +-
>  test/sql/misc.result   | 21 +++++++++++++++++++++
>  test/sql/misc.test.lua |  6 ++++++
>  3 files changed, 28 insertions(+), 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 &&
> diff --git a/test/sql/misc.result b/test/sql/misc.result
> index df2fdde81..d57c087a5 100644
> --- a/test/sql/misc.result
> +++ b/test/sql/misc.result
> @@ -234,3 +234,24 @@ box.execute('EXPLAIN QUERY PLAN SELECT a, b FROM t1, t2 WHERE a = b;')
>    - [0, 0, 0, 'SCAN TABLE T1 (~1048576 rows)']
>    - [0, 1, 1, 'SEARCH TABLE T2 USING EPHEMERAL INDEX (B=?) (~20 rows)']
>  ...
> +-- gh-5592: Make sure that diag is not changed with the correct query.
> +box.execute('SELECT a;')
> +---
> +- null
> +- Can’t resolve field 'A'
> +...
> +diag = box.error.last()
> +---
> +...
> +box.execute('SELECT * FROM (VALUES(true));')
> +---
> +- metadata:
> +  - name: COLUMN_1
> +    type: boolean
> +  rows:
> +  - [true]
> +...
> +diag == box.error.last()
> +---
> +- true
> +...
> diff --git a/test/sql/misc.test.lua b/test/sql/misc.test.lua
> index 44945eace..ce5c1f32e 100644
> --- a/test/sql/misc.test.lua
> +++ b/test/sql/misc.test.lua
> @@ -73,3 +73,9 @@ for i = 1, 10240 do\
>  	box.execute('INSERT INTO t2 VALUES ($1, $1);', {i})\
>  end
>  box.execute('EXPLAIN QUERY PLAN SELECT a, b FROM t1, t2 WHERE a = b;')
> +
> +-- gh-5592: Make sure that diag is not changed with the correct query.
> +box.execute('SELECT a;')
> +diag = box.error.last()
> +box.execute('SELECT * FROM (VALUES(true));')
> +diag == box.error.last()
> -- 
> 2.25.1
>