Re: [Tarantool-patches] [PATCH 1/3] box: check whether box is loaded in box.execute()

[Igor Munkin <imun@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
> <lbox_execute>. 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!
> 

<snipped>


-- 
Best regards,
IM