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

[Mergen Imeev <imeevma@tarantool.org>]

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.

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