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

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

[imeevma]

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

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 05.12.2020 10:35, imeevma@tarantool.org 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.
> 
> 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;

Please, provide a test. It looks like a bug, and it is worth fixing,
but I don't understand how an excess diag can matter here. And if it
matters, it seems not to be covered with a test.

If parsing and VDBE build return 0, we don't look at diag. At least we
should not. Therefore this diag_set should not matter.

>  		if (space_def->id == 0 || space_def->opts.is_view) {
>  			/* Do nothing */
>  		} else if ((pLoop->wsFlags & WHERE_IDX_ONLY) == 0 &&
Also I suggest to get a second review from Nikita, because he knows
SQL code better than Leonid.

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

[Mergen Imeev]

Hi! Thank you for the review. My answers below. Sorry that I didn't
gave you anough information about why this fix is actually needed.

The main idea is that the problem #5592 is the same as #5537, however
it isn't obvoius for now. This patch makes #5592 and #5537 identical.
A bit more below.

On Sat, Dec 05, 2020 at 06:06:26PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 05.12.2020 10:35, imeevma@tarantool.org 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.
> > 
> > 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;
> 
> Please, provide a test. It looks like a bug, and it is worth fixing,
> but I don't understand how an excess diag can matter here. And if it
> matters, it seems not to be covered with a test.
> 
I have no idea how to test this. However, this problem appeared in a
rather unexpected place: there is a problem #5537, where in some cases
we get a segfault because we do not set diag in some places in
os_unix.c. Also, sometimes we get a "Space '0' does not exist" error.
For a while, we had no idea that the two problems were related.

I mean: diag_set() in place where it shouldn't be + (sometimes) no
diag_set() in place where it should be = unexpected error.

Should I think about test here? I think it is possible to test using
error injections, but not sure if this is worth it.

> If parsing and VDBE build return 0, we don't look at diag. At least we
> should not. Therefore this diag_set should not matter.
That is usually true, except there is some cases when there is no
diag_set() in case of error. This happened in issue #5537.

> 
> >  		if (space_def->id == 0 || space_def->opts.is_view) {
> >  			/* Do nothing */
> >  		} else if ((pLoop->wsFlags & WHERE_IDX_ONLY) == 0 &&
> Also I suggest to get a second review from Nikita, because he knows
> SQL code better than Leonid.
Thanks, I will. I included Leonid since he works with issue #5537.

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

[Vladislav Shpilevoy]

Sounds reasonable. Then I am ok with leaving it without a test.
LGTM, proceed to the next review.

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

[Nikita Pettik]

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

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

[Mergen Imeev]

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.

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

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

[Nikita Pettik]

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

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

[Mergen Imeev]

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

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

[Mergen Imeev]

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

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

[Nikita Pettik]

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
> 

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

[Alexander V. Tikhonov]

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

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

[Nikita Pettik]

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