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

Fri Dec 11 17:30:59 MSK 2020

On 11 Dec 16:43, Alexander V. Tikhonov wrote:
> 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

Pushed to master, 2.6 and 2.5. Original branch is dropped. Updated changelogs.

> 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 at gmail.com>
> > > From: Mergen Imeev <imeevma at 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
> > > 