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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.