From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 9CA0C40F3AE for ; Thu, 11 Jun 2020 20:53:00 +0300 (MSK) Date: Thu, 11 Jun 2020 20:43:51 +0300 From: Igor Munkin Message-ID: <20200611174351.GU5745@tarantool.org> References: <20200603215835.GF5745@tarantool.org> <20200608185857.udveavi2fsztazo5@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200608185857.udveavi2fsztazo5@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH 1/3] box: check whether box is loaded in box.execute() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Sasha, Thanks for your clarification! AFAIK, the patch is going to be applied anyway, so here is my LGTM. On 08.06.20, Alexander Turenko wrote: > On Thu, Jun 04, 2020 at 12:58:35AM +0300, Igor Munkin wrote: > > Sasha, > > > > Thanks for your patch! I read the cover letter too, but decided to group > > all my comments regarding this patch below. > > > > No doubt, it is a bug to be fixed in the nearest stable release (2.3.3) > > and we need to fix it despite other issues. Furthermore, the patch > > doesn't break existing behaviour, so nothing is changed in "user space". > > > > But it simply looks irrational to me. All these changes with a pile of > > clarifying comments alongside only confirm once again, that this > > solution is an overkill and too fragile for this dubious feature. We > > have already discussed how to call box.cfg prior to box.execute here[1], > > but discussion has been stalled and nobody has made the final decision > > regarding implicit box.cfg. Unfortunately #4726 is in wishlist now and > > seems to have a little prio for being done (or even discussed) in this > > release. > > > > [1]: https://github.com/tarantool/tarantool/issues/4726#issuecomment-575344974 > > (TL;DR: I would take those problems separately.) > > Even if we'll disable implicit box configuration, it will not land to > the stable release (2.3.3). Current behaviour is buggy and should be > fixed. > > I agree that the solution is complex, but now this complexity is much > more explicit. > > Also I'm not sure that an implementation of #4726 will allow to > completely get rid of pre-box-cfg box.execute variant. We should check > whether box is loaded, because now it seems that SQL code may work > incorrectly otherwise (at least select from _vindex may return just > `nil`, not even {metadata = <...>, rows = <...>} table with empty > `rows`). It seems we need to verify whether box is configured before > execute SQL statements. Sounds valid. > > OTOH, this check should be cheap and may be performed within > . It seems box loading was not done this way not due to > performance reasons (as I think before), but because load_cfg() is in > Lua and it is tricky to call it from C. Anyway, I'm still not sure that > #4726 will completely free us from this locking / checking code: maybe > there are more pitfalls. > > Also I'm not sure that disabling implicit box.cfg() is the right > direction. Maybe we should make all box.cfg() options dynamically > configurable, allow to join a cluster with initial snapshot (after very > first box.cfg()), dynamically adjust memtx_memory if it is not set > explicitly and make other improvements for user's convenience. Well, this question is definitely out of this issue scope. > > I have no opinion here, just noted that it is not so clear as it may > look at first glance. > > > Hence, we can either close two issues with one simple shot or introduce > > complex and fragile (but thanks to you well-described) behaviour that > > might be dropped in the nearest future. > > It is completely okay to drop it if there will be the decision to change > the behaviour. But it would be good to have a correct implementation at > least for reference how to properly implement such things, to have > comments about guratantees of different functions and to have tests for > tricky situations that will show how exactly the behaviour will be > changed (if they will be adopted, not removed). > > > The patch itself is great, except the several cosmetic nits I left > > below. I also Cced Vlad to take a look on the changes. > > Thanks! > -- Best regards, IM