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 E39D5469710 for ; Mon, 8 Jun 2020 21:59:11 +0300 (MSK) Date: Mon, 8 Jun 2020 21:58:57 +0300 From: Alexander Turenko Message-ID: <20200608185857.udveavi2fsztazo5@tkn_work_nb> References: <20200603215835.GF5745@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200603215835.GF5745@tarantool.org> 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: Igor Munkin Cc: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org 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. 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. 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! > > +-- Note: An option should be in `dynamic_cfg_skip_at_load` table > > Typo: everywhere below you use instead of `var`. Fixed. > > +local box_is_configured = false > > Minor: This name is such close to the corresponding variable in box.cc > (is_box_configured) ergo a bit misleading. Why they can't be the same? But box.{cc,h} exposes it via box_is_configured() function (don't ask me about these names) :) It just easier to write a sentence that refer to ' and values' rather than to a variable and a function return value. For this variable I just picked up the naming that I feel better: _. > > + -- This call either succeeds or calls to panic() / exit(). > > Typo: s/calls to/calls/. Fixed. > > + -- This block does not raise an error: all necessary checks > > + -- already performed in private.cfg_check(). See `dynamic_cfg` > > Typo: everywhere below you use instead of `var` Fixed. > > @@ -588,21 +619,51 @@ local function load_cfg(cfg) > > end > > end > > end > > + > > Typo: excess whitespace change. It is not a typo. I added a comment for this block (above it) and so I separated it from the one below: otherwise the comment may be interpreted as if it would about both blocks. > > + -- Configure box if it is not configured, no-op otherwise (not > > + -- reconfiguration). > > Typo: s/not/no/. (It is about 'not reconfiguration'.) It is not a typo. I want to express that reconfiguration must not be performed here at all ('do no-op, not reconfiguration'). Not 'no reconfiguration is performed in the case'. Added the indefinite article. Maybe it'll be more clear if it will be more grammatical :) > > + -- At this point box should be configured and box.execute() > > Minor: I guess assertions below insist on the fact box *must* be > configured at this point :) It is not a standard, but the comment how the code should work. It is quite natural to use the word 'should' here. Other comment (not only mine) are formulated in the same way.