From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A92AF45C305 for ; Mon, 7 Dec 2020 17:57:40 +0300 (MSK) Date: Mon, 7 Dec 2020 14:57:39 +0000 From: Nikita Pettik Message-ID: <20201207145739.GD21104@tarantool.org> References: <20201207141724.GC21104@tarantool.org> <20201207142827.GA125167@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201207142827.GA125167@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove unecessary execute of space_cache_find() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org 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 > > >