[Tarantool-patches] [PATCH 0/3] box.execute() and box.cfg() idempotence and locking

[Tarantool-patches] [PATCH 0/3] box.execute() and box.cfg() idempotence and locking

[Alexander Turenko]

This patchset provides several fixes for box.execute() and box.cfg()
functions when they are called under various circumstances: when a
function is saved before box loading and called after it, when a
function is called during box loading.

Existence of this patchset does not mean that we'll not implement
https://github.com/tarantool/tarantool/issues/4726: I don't know whether
we will do or will not, but I intend to fix bugs in the existing code.

https://github.com/tarantool/tarantool/issues/4231
Totktonada/gh-4231-box-execute-idempotence

My review is not more sufficient, because I became co-author of the
patchset. Igor, can you, please, review it and pass to a second reviewer
(I suggest Vlad)?

Alexander Turenko (1):
  box: always wait box loading in box.execute()

Maria (2):
  box: check whether box is loaded in box.execute()
  box: always reconfigure box at non-first box.cfg()

 src/box/lua/load_cfg.lua                      | 97 +++++++++++++++++--
 .../gh-4231-box-cfg-idempotence.test.lua      | 34 +++++++
 .../gh-4231-box-execute-idempotence.test.lua  | 37 +++++++
 .../gh-4231-box-execute-locking.test.lua      | 69 +++++++++++++
 test/box-tap/suite.cfg                        |  6 ++
 test/box-tap/suite.ini                        |  1 +
 6 files changed, 237 insertions(+), 7 deletions(-)
 create mode 100755 test/box-tap/gh-4231-box-cfg-idempotence.test.lua
 create mode 100755 test/box-tap/gh-4231-box-execute-idempotence.test.lua
 create mode 100755 test/box-tap/gh-4231-box-execute-locking.test.lua
 create mode 100644 test/box-tap/suite.cfg

-- 
2.25.0

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

[Alexander Turenko]

From: Maria <maria.khaydich@tarantool.org>

box.execute() initializes box if it is not initialized. For this sake,
box.execute() is another function (so called <box_load_and_execute>)
when box is not loaded: it loads box and calls 'real' box.execute().
However it is not enough: <box_load_and_execute> may be saved by a user
before box initialization and called after box loading or during box
loading from a separate fiber.

Note: calling <box_load_and_execute> during box loading is safe now, but
calling of box.execute() is not: the 'real' box.execute() does not
verify whether box is configured. It will be fixed in a further commit.

This commit changes <box_load_and_execute> to verify whether box is
initialized and to load box only when it is not loaded. Also it adds
appropriate locking around load_cfg() invocation from
<box_load_and_execute>.

While we're here, clarified contracts of functions that set box
configuration options.

Closes #4231

Co-developed-by: Alexander Turenko <alexander.turenko@tarantool.org>
---
 src/box/lua/load_cfg.lua                      | 69 +++++++++++++++++--
 .../gh-4231-box-execute-idempotence.test.lua  | 37 ++++++++++
 .../gh-4231-box-execute-locking.test.lua      | 46 +++++++++++++
 3 files changed, 148 insertions(+), 4 deletions(-)
 create mode 100755 test/box-tap/gh-4231-box-execute-idempotence.test.lua
 create mode 100755 test/box-tap/gh-4231-box-execute-locking.test.lua

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 5d818addf..0b4a14377 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -230,6 +230,11 @@ local function check_replicaset_uuid()
 end
 
 -- dynamically settable options
+--
+-- Note: An option should be in `dynamic_cfg_skip_at_load` table
+-- or should be verified in box_check_config(). Otherwise
+-- load_cfg() may report an error, but box will be configured in
+-- fact.
 local dynamic_cfg = {
     listen                  = private.cfg_set_listen,
     replication             = private.cfg_set_replication,
@@ -550,6 +555,16 @@ setmetatable(box, {
      end
 })
 
+-- Whether box is loaded.
+--
+-- `false` when box is not configured or when the initialization
+-- is in progress.
+--
+-- `true` when box is configured.
+--
+-- Use locked() wrapper to obtain reliable results.
+local box_is_configured = false
+
 local function load_cfg(cfg)
     cfg = upgrade_cfg(cfg, translate_cfg)
     cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
@@ -560,6 +575,16 @@ local function load_cfg(cfg)
         box.cfg = locked(load_cfg) -- restore original box.cfg
         return box.error() -- re-throw exception from check_cfg()
     end
+
+    -- NB: After this point the function should not raise an
+    -- error.
+    --
+    -- This is important to have right `box_is_configured` (this
+    -- file) and `is_box_configured` (box.cc) values.
+    --
+    -- It also would be counter-intuitive to receive an error from
+    -- box.cfg({<...>}), but find that box is actually configured.
+
     -- Restore box members after initial configuration
     for k, v in pairs(box_configured) do
         box[k] = v
@@ -573,7 +598,13 @@ local function load_cfg(cfg)
             end,
             __call = locked(reload_cfg),
         })
+
+    -- This call either succeeds or calls to panic() / exit().
     private.cfg_load()
+
+    -- This block does not raise an error: all necessary checks
+    -- already performed in private.cfg_check(). See `dynamic_cfg`
+    -- comment.
     for key, fun in pairs(dynamic_cfg) do
         local val = cfg[key]
         if val ~= nil then
@@ -588,21 +619,51 @@ local function load_cfg(cfg)
             end
         end
     end
+
     if not box.cfg.read_only and not box.cfg.replication then
         box.schema.upgrade{auto = true}
     end
+
+    box_is_configured = true
 end
 box.cfg = locked(load_cfg)
 
 --
 -- This makes possible do box.execute without calling box.cfg
--- manually. The load_cfg call overwrites following table and
--- metatable.
+-- manually. The load_cfg() call overwrites box.execute, see
+-- <box_configured> variable.
+--
+-- box.execute is <box_load_and_execute> when box is not loaded,
+-- <lbox_execute> otherwise. <box_load_and_execute> loads box and
+-- calls <lbox_execute>.
 --
-function box.execute(...)
-    load_cfg()
+local box_load_and_execute
+box_load_and_execute = function(...)
+    -- Configure box if it is not configured, no-op otherwise (not
+    -- reconfiguration).
+    --
+    -- We should check whether box is configured, because a user
+    -- may save <box_load_and_execute> function before box loading
+    -- and call it afterwards.
+    if not box_is_configured then
+        locked(function()
+            -- Re-check, because after releasing of the lock the
+            -- box state may be changed. We should call load_cfg()
+            -- only once.
+            if not box_is_configured then
+                load_cfg()
+            end
+        end)()
+    end
+
+    -- At this point box should be configured and box.execute()
+    -- function should be replaced with lbox_execute().
+    assert(type(box.cfg) ~= 'function')
+    assert(box.execute ~= box_load_and_execute)
+
     return box.execute(...)
 end
+box.execute = box_load_and_execute
 
 -- gh-810:
 -- hack luajit default cpath
diff --git a/test/box-tap/gh-4231-box-execute-idempotence.test.lua b/test/box-tap/gh-4231-box-execute-idempotence.test.lua
new file mode 100755
index 000000000..940451521
--- /dev/null
+++ b/test/box-tap/gh-4231-box-execute-idempotence.test.lua
@@ -0,0 +1,37 @@
+#!/usr/bin/env tarantool
+
+--
+-- gh-4231: box.execute should be an idempotent function meaning
+-- its effect should be the same if the user chooses to save it
+-- before explicit box.cfg{} invocation and use the saved version
+-- afterwards.
+--
+
+local tap = require('tap')
+local test = tap.test('gh-4231-box-execute-idempotence')
+test:plan(3)
+
+local box_load_and_execute = box.execute
+
+-- box is not initialized after invalid box.cfg() call and
+-- box.execute() should initialize it first.
+pcall(box.cfg, {listen = 'invalid'})
+local ok, res = pcall(box.execute, 'SELECT 1')
+test:ok(ok, 'verify box.execute() after invalid box.cfg() call', {err = res})
+
+-- Make test cases independent: if the box.execute() call above
+-- fails, initialize box explicitly for the next test cases.
+box.cfg{}
+
+-- This call should be successful and should skip box
+-- (re)initialization.
+local ok, res = pcall(box_load_and_execute, 'SELECT 1')
+test:ok(ok, 'verify box_load_and_execute after successful box.cfg() call',
+        {err = res})
+
+-- Just in case: verify usual box.execute() after
+-- box_load_and_execute().
+local ok, res = pcall(box.execute, 'SELECT 1')
+test:ok(ok, 'verify box.execute() after box_load_and_execute()', {err = res})
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/box-tap/gh-4231-box-execute-locking.test.lua b/test/box-tap/gh-4231-box-execute-locking.test.lua
new file mode 100755
index 000000000..52c60f764
--- /dev/null
+++ b/test/box-tap/gh-4231-box-execute-locking.test.lua
@@ -0,0 +1,46 @@
+#!/usr/bin/env tarantool
+
+-- gh-4231: run box_load_and_execute() (it is box.execute value
+-- before box will be loaded) in several fibers in parallel and
+-- ensure that it returns correct results (i.e. that the function
+-- waits until box will be fully configured).
+
+local fiber = require('fiber')
+local tap = require('tap')
+
+local box_load_and_execute = box.execute
+local fiber_count = 10
+local results = fiber.channel(fiber_count)
+
+local function select_from_vindex()
+    local res = box_load_and_execute('SELECT * FROM "_vindex"')
+    results:put(res)
+end
+
+local test = tap.test('gh-4231-box-execute-locking')
+
+test:plan(fiber_count)
+
+local exp_metadata = {
+    {name = 'id',    type = 'unsigned'},
+    {name = 'iid',   type = 'unsigned'},
+    {name = 'name',  type = 'string'},
+    {name = 'type',  type = 'string'},
+    {name = 'opts',  type = 'map'},
+    {name = 'parts', type = 'array'},
+}
+
+for _ = 1, fiber_count do
+    fiber.create(select_from_vindex)
+end
+for i = 1, fiber_count do
+    local res = results:get()
+    test:test(('result %d'):format(i), function(test)
+        test:plan(2)
+        test:is_deeply(res.metadata, exp_metadata, 'verify metadata')
+        local rows_is_ok = type(res.rows) == 'table' and #res.rows > 0
+        test:ok(rows_is_ok, 'verify rows')
+    end)
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

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

[lvasiliev]

Hi! Thanks for the patch.
LGTM.

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

[Igor Munkin]

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.

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.

The patch itself is great, except the several cosmetic nits I left
below. I also Cced Vlad to take a look on the changes.

On 13.05.20, Alexander Turenko wrote:
> From: Maria <maria.khaydich@tarantool.org>
> 
> box.execute() initializes box if it is not initialized. For this sake,
> box.execute() is another function (so called <box_load_and_execute>)
> when box is not loaded: it loads box and calls 'real' box.execute().
> However it is not enough: <box_load_and_execute> may be saved by a user
> before box initialization and called after box loading or during box
> loading from a separate fiber.
> 
> Note: calling <box_load_and_execute> during box loading is safe now, but
> calling of box.execute() is not: the 'real' box.execute() does not
> verify whether box is configured. It will be fixed in a further commit.
> 
> This commit changes <box_load_and_execute> to verify whether box is
> initialized and to load box only when it is not loaded. Also it adds
> appropriate locking around load_cfg() invocation from
> <box_load_and_execute>.
> 
> While we're here, clarified contracts of functions that set box
> configuration options.
> 
> Closes #4231
> 
> Co-developed-by: Alexander Turenko <alexander.turenko@tarantool.org>
> ---
>  src/box/lua/load_cfg.lua                      | 69 +++++++++++++++++--
>  .../gh-4231-box-execute-idempotence.test.lua  | 37 ++++++++++
>  .../gh-4231-box-execute-locking.test.lua      | 46 +++++++++++++
>  3 files changed, 148 insertions(+), 4 deletions(-)
>  create mode 100755 test/box-tap/gh-4231-box-execute-idempotence.test.lua
>  create mode 100755 test/box-tap/gh-4231-box-execute-locking.test.lua
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 5d818addf..0b4a14377 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -230,6 +230,11 @@ local function check_replicaset_uuid()
>  end
>  
>  -- dynamically settable options
> +--
> +-- Note: An option should be in `dynamic_cfg_skip_at_load` table

Typo: everywhere below you use <var> instead of `var`.

> +-- or should be verified in box_check_config(). Otherwise
> +-- load_cfg() may report an error, but box will be configured in
> +-- fact.
>  local dynamic_cfg = {
>      listen                  = private.cfg_set_listen,
>      replication             = private.cfg_set_replication,
> @@ -550,6 +555,16 @@ setmetatable(box, {
>       end
>  })
>  
> +-- Whether box is loaded.
> +--
> +-- `false` when box is not configured or when the initialization
> +-- is in progress.
> +--
> +-- `true` when box is configured.
> +--
> +-- Use locked() wrapper to obtain reliable results.
> +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?

> +
>  local function load_cfg(cfg)
>      cfg = upgrade_cfg(cfg, translate_cfg)
>      cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
> @@ -560,6 +575,16 @@ local function load_cfg(cfg)
>          box.cfg = locked(load_cfg) -- restore original box.cfg
>          return box.error() -- re-throw exception from check_cfg()
>      end
> +
> +    -- NB: After this point the function should not raise an
> +    -- error.
> +    --
> +    -- This is important to have right `box_is_configured` (this
> +    -- file) and `is_box_configured` (box.cc) values.
> +    --
> +    -- It also would be counter-intuitive to receive an error from
> +    -- box.cfg({<...>}), but find that box is actually configured.
> +
>      -- Restore box members after initial configuration
>      for k, v in pairs(box_configured) do
>          box[k] = v
> @@ -573,7 +598,13 @@ local function load_cfg(cfg)
>              end,
>              __call = locked(reload_cfg),
>          })
> +
> +    -- This call either succeeds or calls to panic() / exit().

Typo: s/calls to/calls/.

>      private.cfg_load()
> +
> +    -- 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`

> +    -- comment.
>      for key, fun in pairs(dynamic_cfg) do
>          local val = cfg[key]
>          if val ~= nil then
> @@ -588,21 +619,51 @@ local function load_cfg(cfg)
>              end
>          end
>      end
> +

Typo: excess whitespace change.

>      if not box.cfg.read_only and not box.cfg.replication then
>          box.schema.upgrade{auto = true}
>      end
> +
> +    box_is_configured = true
>  end
>  box.cfg = locked(load_cfg)
>  
>  --
>  -- This makes possible do box.execute without calling box.cfg
> --- manually. The load_cfg call overwrites following table and
> --- metatable.
> +-- manually. The load_cfg() call overwrites box.execute, see
> +-- <box_configured> variable.
> +--
> +-- box.execute is <box_load_and_execute> when box is not loaded,
> +-- <lbox_execute> otherwise. <box_load_and_execute> loads box and
> +-- calls <lbox_execute>.
>  --
> -function box.execute(...)
> -    load_cfg()
> +local box_load_and_execute
> +box_load_and_execute = function(...)
> +    -- Configure box if it is not configured, no-op otherwise (not

Typo: s/not/no/.

> +    -- reconfiguration).
> +    --
> +    -- We should check whether box is configured, because a user
> +    -- may save <box_load_and_execute> function before box loading
> +    -- and call it afterwards.
> +    if not box_is_configured then
> +        locked(function()
> +            -- Re-check, because after releasing of the lock the
> +            -- box state may be changed. We should call load_cfg()
> +            -- only once.
> +            if not box_is_configured then
> +                load_cfg()
> +            end
> +        end)()
> +    end
> +
> +    -- 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 :)

> +    -- function should be replaced with lbox_execute().
> +    assert(type(box.cfg) ~= 'function')
> +    assert(box.execute ~= box_load_and_execute)
> +
>      return box.execute(...)
>  end
> +box.execute = box_load_and_execute
>  
>  -- gh-810:
>  -- hack luajit default cpath

<snipped>

> -- 
> 2.25.0
> 

[1]: https://github.com/tarantool/tarantool/issues/4726#issuecomment-575344974

-- 
Best regards,
IM

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

[Alexander Turenko]

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.

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

[Igor Munkin]

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

[Tarantool-patches] [PATCH 2/3] box: always wait box loading in box.execute()

[Alexander Turenko]

<box_load_and_execute> checks whether box is configured with appropriate
locking and configures it when necessary. However it is not so for
<lbox_execute>. We should replace the former with the latter only when
box is fully loaded.

Follow-up #4231
---
 src/box/lua/load_cfg.lua                      | 20 ++++++++++++---
 .../gh-4231-box-execute-locking.test.lua      | 25 ++++++++++++++++++-
 test/box-tap/suite.cfg                        |  6 +++++
 test/box-tap/suite.ini                        |  1 +
 4 files changed, 48 insertions(+), 4 deletions(-)
 create mode 100644 test/box-tap/suite.cfg

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 0b4a14377..9a7b57cd3 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -538,6 +538,11 @@ local box_cfg_guard_whitelist = {
     NULL = true;
 };
 
+-- List of box members that requires full box loading.
+local box_restore_after_full_load_list = {
+    execute = true,
+}
+
 local box = require('box')
 -- Move all box members except the whitelisted to box_configured
 local box_configured = {}
@@ -585,12 +590,13 @@ local function load_cfg(cfg)
     -- It also would be counter-intuitive to receive an error from
     -- box.cfg({<...>}), but find that box is actually configured.
 
-    -- Restore box members after initial configuration
+    -- Restore box members after initial configuration.
     for k, v in pairs(box_configured) do
-        box[k] = v
+        if not box_restore_after_full_load_list[k] then
+            box[k] = v
+        end
     end
     setmetatable(box, nil)
-    box_configured = nil
     box.cfg = setmetatable(cfg,
         {
             __newindex = function(table, index)
@@ -624,6 +630,14 @@ local function load_cfg(cfg)
         box.schema.upgrade{auto = true}
     end
 
+    -- Restore box members that requires full box loading.
+    for k, v in pairs(box_configured) do
+        if box_restore_after_full_load_list[k] then
+            box[k] = v
+        end
+    end
+    box_configured = nil
+
     box_is_configured = true
 end
 box.cfg = locked(load_cfg)
diff --git a/test/box-tap/gh-4231-box-execute-locking.test.lua b/test/box-tap/gh-4231-box-execute-locking.test.lua
index 52c60f764..f3df2eb0f 100755
--- a/test/box-tap/gh-4231-box-execute-locking.test.lua
+++ b/test/box-tap/gh-4231-box-execute-locking.test.lua
@@ -4,20 +4,43 @@
 -- before box will be loaded) in several fibers in parallel and
 -- ensure that it returns correct results (i.e. that the function
 -- waits until box will be fully configured).
+--
+-- The test can be configured to call box.execute() from a fiber
+-- instead of box_load_and_execute(): it can be done either via
+-- test-run using confguration feature or using an argument when
+-- the test is invoked directly.
 
 local fiber = require('fiber')
 local tap = require('tap')
 
+-- Determine configuration.
+local conf = 'box_load_and_execute'
+local ok, test_run = pcall(require, 'test_run')
+if ok then
+    test_run = test_run.new()
+    conf = test_run:get_cfg('conf')
+elseif #arg >= 1 then
+    conf = arg[1]
+end
+assert(conf == 'box_load_and_execute' or
+       conf == 'box.execute')
+
 local box_load_and_execute = box.execute
 local fiber_count = 10
 local results = fiber.channel(fiber_count)
 
 local function select_from_vindex()
-    local res = box_load_and_execute('SELECT * FROM "_vindex"')
+    local box_execute_func =
+        conf == 'box_load_and_execute' and box_load_and_execute or
+        conf == 'box.execute' and box.execute or
+        function() return false end
+
+    local res = box_execute_func('SELECT * FROM "_vindex"')
     results:put(res)
 end
 
 local test = tap.test('gh-4231-box-execute-locking')
+test:diag('configuration: %s', conf)
 
 test:plan(fiber_count)
 
diff --git a/test/box-tap/suite.cfg b/test/box-tap/suite.cfg
new file mode 100644
index 000000000..91a721d47
--- /dev/null
+++ b/test/box-tap/suite.cfg
@@ -0,0 +1,6 @@
+{
+    "gh-4231-box-execute-locking.test.lua": {
+        "box_load_and_execute": {"conf": "box_load_and_execute"},
+        "box.execute": {"conf": "box.execute"}
+    }
+}
diff --git a/test/box-tap/suite.ini b/test/box-tap/suite.ini
index 8d9e32d3f..5b764593a 100644
--- a/test/box-tap/suite.ini
+++ b/test/box-tap/suite.ini
@@ -5,3 +5,4 @@ is_parallel = True
 pretest_clean = True
 fragile = cfg.test.lua     ; gh-4344
           key_def.test.lua ; gh-4252
+config = suite.cfg
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 2/3] box: always wait box loading in box.execute()

[lvasiliev]

Hi! Thanks for the patch.
LGTM.

Re: [Tarantool-patches] [PATCH 2/3] box: always wait box loading in box.execute()

[Igor Munkin]

Sasha,

Thanks for the patch! It LGTM, but is "blocked" since it strongly
depends on the previous one.

On 13.05.20, Alexander Turenko wrote:
> <box_load_and_execute> checks whether box is configured with appropriate
> locking and configures it when necessary. However it is not so for
> <lbox_execute>. We should replace the former with the latter only when
> box is fully loaded.
> 
> Follow-up #4231
> ---
>  src/box/lua/load_cfg.lua                      | 20 ++++++++++++---
>  .../gh-4231-box-execute-locking.test.lua      | 25 ++++++++++++++++++-
>  test/box-tap/suite.cfg                        |  6 +++++
>  test/box-tap/suite.ini                        |  1 +
>  4 files changed, 48 insertions(+), 4 deletions(-)
>  create mode 100644 test/box-tap/suite.cfg
> 

<snipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
IM

[Tarantool-patches] [PATCH 3/3] box: always reconfigure box at non-first box.cfg()

[Alexander Turenko]

From: Maria <maria.khaydich@tarantool.org>

Calling box.cfg{} more than once does not normally cause any errors
(even though it might not have any effect). In contrast, assigning
it to some variable and then using it after the box was configured
caused an error since the method was overwritten by the initial call
of <load_cfg>.

The patch fixes this issue making box.cfg behave consistently in both
scenarios.

Follow-up #4231

Co-developed-by: Alexander Turenko <alexander.turenko@tarantool.org>
---
 src/box/lua/load_cfg.lua                      |  8 +++++
 .../gh-4231-box-cfg-idempotence.test.lua      | 34 +++++++++++++++++++
 2 files changed, 42 insertions(+)
 create mode 100755 test/box-tap/gh-4231-box-cfg-idempotence.test.lua

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 9a7b57cd3..014379826 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -571,6 +571,14 @@ setmetatable(box, {
 local box_is_configured = false
 
 local function load_cfg(cfg)
+    -- A user may save box.cfg (this function) before box loading
+    -- and call it afterwards. We should reconfigure box in the
+    -- case.
+    if box_is_configured then
+        reload_cfg(box.cfg, cfg)
+        return
+    end
+
     cfg = upgrade_cfg(cfg, translate_cfg)
     cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
     apply_default_cfg(cfg, default_cfg);
diff --git a/test/box-tap/gh-4231-box-cfg-idempotence.test.lua b/test/box-tap/gh-4231-box-cfg-idempotence.test.lua
new file mode 100755
index 000000000..4f3ba68a6
--- /dev/null
+++ b/test/box-tap/gh-4231-box-cfg-idempotence.test.lua
@@ -0,0 +1,34 @@
+#!/usr/bin/env tarantool
+
+--
+-- gh-4231: box.cfg is another function (so called <load_cfg>)
+-- before box is loaded. Usually a user calls box.cfg({<...>}),
+-- it configures box and replaces box.cfg implementation to one
+-- that performs box reconfiguration: so further calls to
+-- box.cfg({<...>}) reconfigures box.
+--
+-- However it is possible to save box.cfg value (<load_cfg>)
+-- before box loading and call it after box loading: the behaviour
+-- should be the same as for box.cfg call: box should be
+-- reconfigured.
+--
+
+local tap = require('tap')
+local test = tap.test('gh-4231-box-cfg-idempotence')
+test:plan(4)
+
+local load_cfg = box.cfg
+
+box.cfg{}
+
+-- This call should be successful and should reinitialize box.
+local ok, res = pcall(load_cfg, {read_only = true})
+test:ok(ok, 'verify load_cfg after box.cfg() call', {err = res})
+test:is(box.cfg.read_only, true, 'verify that load_cfg reconfigures box')
+
+-- Just in case: verify usual box.cfg() after load_cfg().
+local ok, res = pcall(box.cfg, {read_only = false})
+test:ok(ok, 'verify box.cfg() after load_cfg()', {err = res})
+test:is(box.cfg.read_only, false, 'verify that box.cfg() reconfigures box')
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 3/3] box: always reconfigure box at non-first box.cfg()

[lvasiliev]

Hi! Thanks for the patch!
LGTM.

Re: [Tarantool-patches] [PATCH 3/3] box: always reconfigure box at non-first box.cfg()

[Igor Munkin]

Sasha,

Thanks for the patch, LGTM, except two typos below.

On 13.05.20, Alexander Turenko wrote:
> From: Maria <maria.khaydich@tarantool.org>
> 
> Calling box.cfg{} more than once does not normally cause any errors
> (even though it might not have any effect). In contrast, assigning
> it to some variable and then using it after the box was configured
> caused an error since the method was overwritten by the initial call
> of <load_cfg>.
> 
> The patch fixes this issue making box.cfg behave consistently in both
> scenarios.
> 
> Follow-up #4231
> 
> Co-developed-by: Alexander Turenko <alexander.turenko@tarantool.org>
> ---
>  src/box/lua/load_cfg.lua                      |  8 +++++
>  .../gh-4231-box-cfg-idempotence.test.lua      | 34 +++++++++++++++++++
>  2 files changed, 42 insertions(+)
>  create mode 100755 test/box-tap/gh-4231-box-cfg-idempotence.test.lua
> 

<snipped>

> diff --git a/test/box-tap/gh-4231-box-cfg-idempotence.test.lua b/test/box-tap/gh-4231-box-cfg-idempotence.test.lua
> new file mode 100755
> index 000000000..4f3ba68a6
> --- /dev/null
> +++ b/test/box-tap/gh-4231-box-cfg-idempotence.test.lua
> @@ -0,0 +1,34 @@
> +#!/usr/bin/env tarantool
> +
> +--
> +-- gh-4231: box.cfg is another function (so called <load_cfg>)
> +-- before box is loaded. Usually a user calls box.cfg({<...>}),
> +-- it configures box and replaces box.cfg implementation to one
> +-- that performs box reconfiguration: so further calls to
> +-- box.cfg({<...>}) reconfigures box.

Typo: s/reconfigures/reconfigure/.

> +--
> +-- However it is possible to save box.cfg value (<load_cfg>)
> +-- before box loading and call it after box loading: the behaviour
> +-- should be the same as for box.cfg call: box should be

Typo: There are two colons in one sentence, so I propose simply to
s/:/and/ the last one.

> +-- reconfigured.
> +--
> +

<snipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 3/3] box: always reconfigure box at non-first box.cfg()

[Igor Munkin]

Feel free to push this patch out of order.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 3/3] box: always reconfigure box at non-first box.cfg()

[Alexander Turenko]

> > +-- gh-4231: box.cfg is another function (so called <load_cfg>)
> > +-- before box is loaded. Usually a user calls box.cfg({<...>}),
> > +-- it configures box and replaces box.cfg implementation to one
> > +-- that performs box reconfiguration: so further calls to
> > +-- box.cfg({<...>}) reconfigures box.
> 
> Typo: s/reconfigures/reconfigure/.

Fixed.

> 
> > +--
> > +-- However it is possible to save box.cfg value (<load_cfg>)
> > +-- before box loading and call it after box loading: the behaviour
> > +-- should be the same as for box.cfg call: box should be
> 
> Typo: There are two colons in one sentence, so I propose simply to
> s/:/and/ the last one.

Replaced the first colon with a period.

Re: [Tarantool-patches] [PATCH 3/3] box: always reconfigure box at non-first box.cfg()

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

>  src/box/lua/load_cfg.lua                      |  8 +++++
>  .../gh-4231-box-cfg-idempotence.test.lua      | 34 +++++++++++++++++++
>  2 files changed, 42 insertions(+)
>  create mode 100755 test/box-tap/gh-4231-box-cfg-idempotence.test.lua
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 9a7b57cd3..014379826 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -571,6 +571,14 @@ setmetatable(box, {
>  local box_is_configured = false
>  
>  local function load_cfg(cfg)
> +    -- A user may save box.cfg (this function) before box loading
> +    -- and call it afterwards. We should reconfigure box in the
> +    -- case.
> +    if box_is_configured then
> +        reload_cfg(box.cfg, cfg)

Shouldn't you do 'locked(reload_cfg, box.cfg, cfg)'? Otherwise
you have the same problem as solved in the first commit.

> +        return
> +    end

Re: [Tarantool-patches] [PATCH 3/3] box: always reconfigure box at non-first box.cfg()

[Alexander Turenko]

On Thu, Jun 18, 2020 at 12:26:39AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> >  src/box/lua/load_cfg.lua                      |  8 +++++
> >  .../gh-4231-box-cfg-idempotence.test.lua      | 34 +++++++++++++++++++
> >  2 files changed, 42 insertions(+)
> >  create mode 100755 test/box-tap/gh-4231-box-cfg-idempotence.test.lua
> > 
> > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> > index 9a7b57cd3..014379826 100644
> > --- a/src/box/lua/load_cfg.lua
> > +++ b/src/box/lua/load_cfg.lua
> > @@ -571,6 +571,14 @@ setmetatable(box, {
> >  local box_is_configured = false
> >  
> >  local function load_cfg(cfg)
> > +    -- A user may save box.cfg (this function) before box loading
> > +    -- and call it afterwards. We should reconfigure box in the
> > +    -- case.
> > +    if box_is_configured then
> > +        reload_cfg(box.cfg, cfg)
> 
> Shouldn't you do 'locked(reload_cfg, box.cfg, cfg)'? Otherwise
> you have the same problem as solved in the first commit.

We should not. load_cfg() is always called under the lock unlike
box.execute().

Re: [Tarantool-patches] [PATCH 3/3] box: always reconfigure box at non-first box.cfg()

[Vladislav Shpilevoy]

On 18/06/2020 10:41, Alexander Turenko wrote:
> On Thu, Jun 18, 2020 at 12:26:39AM +0200, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch!
>>
>>>  src/box/lua/load_cfg.lua                      |  8 +++++
>>>  .../gh-4231-box-cfg-idempotence.test.lua      | 34 +++++++++++++++++++
>>>  2 files changed, 42 insertions(+)
>>>  create mode 100755 test/box-tap/gh-4231-box-cfg-idempotence.test.lua
>>>
>>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>>> index 9a7b57cd3..014379826 100644
>>> --- a/src/box/lua/load_cfg.lua
>>> +++ b/src/box/lua/load_cfg.lua
>>> @@ -571,6 +571,14 @@ setmetatable(box, {
>>>  local box_is_configured = false
>>>  
>>>  local function load_cfg(cfg)
>>> +    -- A user may save box.cfg (this function) before box loading
>>> +    -- and call it afterwards. We should reconfigure box in the
>>> +    -- case.
>>> +    if box_is_configured then
>>> +        reload_cfg(box.cfg, cfg)
>>
>> Shouldn't you do 'locked(reload_cfg, box.cfg, cfg)'? Otherwise
>> you have the same problem as solved in the first commit.
> 
> We should not. load_cfg() is always called under the lock unlike
> box.execute().

Ok, I see. Then the patchset LGTM.

Re: [Tarantool-patches] [PATCH 0/3] box.execute() and box.cfg() idempotence and locking

[lvasiliev]

Hi! Thanks for the patch.
Please add @ChangeLog .

Re: [Tarantool-patches] [PATCH 0/3] box.execute() and box.cfg() idempotence and locking

[Alexander Turenko]

On Fri, May 22, 2020 at 10:06:54AM +0300, lvasiliev wrote:
> Hi! Thanks for the patch.
> Please add @ChangeLog .

Thanks for the reminder!

@ChangeLog

- Fixed races and corner cases in box (re)configuration (gh-4221).

Re: [Tarantool-patches] [PATCH 0/3] box.execute() and box.cfg() idempotence and locking

[Vladislav Shpilevoy]

Generally the patchset is fine, except one commit for the last
commit.

However the 'feature' about implicit box.cfg call should be
deleted, IMO. It only brings complexity to the code. No one
ever asked for it. Its purpose was to allow to do box.cfg from
SQL console, but there is no a problem in doing it manually
before touching SQL. Anyway nobody uses box.cfg without parameters
except for small manual tests. Note,
box.execute('SELECT lua("box.cfg ...")') won't work, because VDBE
and the parser heavily depend on the initialized box.

Re: [Tarantool-patches] [PATCH 0/3] box.execute() and box.cfg() idempotence and locking

[Kirill Yukhin]

Hello,

On 13 май 01:18, Alexander Turenko wrote:
> This patchset provides several fixes for box.execute() and box.cfg()
> functions when they are called under various circumstances: when a
> function is saved before box loading and called after it, when a
> function is called during box loading.
> 
> Existence of this patchset does not mean that we'll not implement
> https://github.com/tarantool/tarantool/issues/4726: I don't know whether
> we will do or will not, but I intend to fix bugs in the existing code.
> 
> https://github.com/tarantool/tarantool/issues/4231
> Totktonada/gh-4231-box-execute-idempotence
> 
> My review is not more sufficient, because I became co-author of the
> patchset. Igor, can you, please, review it and pass to a second reviewer
> (I suggest Vlad)?
> 
> Alexander Turenko (1):
>   box: always wait box loading in box.execute()
> 
> Maria (2):
>   box: check whether box is loaded in box.execute()
>   box: always reconfigure box at non-first box.cfg()

I've checked your patchset into 2.3, 2.4 and master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 0/3] box.execute() and box.cfg() idempotence and locking

[Alexander Turenko]

On Mon, Jun 22, 2020 at 01:11:12PM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 13 май 01:18, Alexander Turenko wrote:
> > This patchset provides several fixes for box.execute() and box.cfg()
> > functions when they are called under various circumstances: when a
> > function is saved before box loading and called after it, when a
> > function is called during box loading.
> > 
> > Existence of this patchset does not mean that we'll not implement
> > https://github.com/tarantool/tarantool/issues/4726: I don't know whether
> > we will do or will not, but I intend to fix bugs in the existing code.
> > 
> > https://github.com/tarantool/tarantool/issues/4231
> > Totktonada/gh-4231-box-execute-idempotence
> > 
> > My review is not more sufficient, because I became co-author of the
> > patchset. Igor, can you, please, review it and pass to a second reviewer
> > (I suggest Vlad)?
> > 
> > Alexander Turenko (1):
> >   box: always wait box loading in box.execute()
> > 
> > Maria (2):
> >   box: check whether box is loaded in box.execute()
> >   box: always reconfigure box at non-first box.cfg()
> 
> I've checked your patchset into 2.3, 2.4 and master.

Pushed 'box: always reconfigure box at non-first box.cfg()' to 1.10.

Borrowed <box_is_configured> variable and related comments from 'box:
check whether box is loaded in box.execute()' to correctly apply the
change.

Added an entry to the future 1.10.7 release notes.

Note: Also fixed gh-4221 with gh-4231 in other release notes, I mistyped
it in [1]. Sorry.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017543.html

WBR, Alexander Turenko.