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

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi All, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/227954475

On Thu, Dec 10, 2020 at 01:38:59PM +0000, Nikita Pettik wrote:
> 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
> >