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

[Alexander Turenko <alexander.turenko@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
<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.

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 <var> 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
'<var1> and <var2> values' rather than to a variable and a function
return value.

For this variable I just picked up the naming that I feel better:
<module>_<verb phrase>.

> > +    -- 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 <var> 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.