From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Fri, 5 Jul 2019 12:00:17 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH] lua/trigger: cleanup lua stack after trigger run
Message-ID: <20190705090017.2qwzcfbyiwlkkjbj@esperanza>
References: <20190625143250.97948-1-sergepetrenko@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190625143250.97948-1-sergepetrenko@tarantool.org>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Tue, Jun 25, 2019 at 05:32:50PM +0300, Serge Petrenko wrote:
> diff --git a/test/box/before_replace.test.lua b/test/box/before_replace.test.lua
> index c46ec25a0..688b012d0 100644
> --- a/test/box/before_replace.test.lua
> +++ b/test/box/before_replace.test.lua
> @@ -306,3 +306,30 @@ _ = s:insert{1}
>  save_type
>  
>  s:drop()
> +
> +--
> +-- gh-4275 segfault if before_replace
> +-- trigger set in on_schema_init triggers runs on recovery.
> +--
> +s = box.schema.space.create('test_on_schema_init')
> +_ = s:create_index('pk')
> +test_run:cmd('setopt delimiter ";"')
> +function gen_inserts()
> +    box.begin()
> +    for i = 1,1000 do
> +        s:upsert({1, 1}, {{'+', 2, 1}})
> +    end
> +    box.commit()
> +end;
> +test_run:cmd('setopt delimiter ""');
> +for i = 1,66 do gen_inserts() end
> +test_run:cmd('restart server default with args="true"')
> +s = box.space.test_on_schema_init
> +s:count()
> +-- insertion: 65999 updates from gen_inserts(), starting value is 1.
> +-- recovery: 66000 invocations of before replace trigger, each
> +-- invocation updates the tuple, but only in memory: the WAL remains
> +-- unchanged.
> +-- A total of 1 + 65999 + 66000 = 132000
> +s:get{1}[2] == 2 * 66000 or s:get{1}[2]
> +test_run:cmd('restart server default with cleanup=1')
> diff --git a/test/box/box.lua b/test/box/box.lua
> index 2a8e0e4fa..7eaa5043e 100644
> --- a/test/box/box.lua
> +++ b/test/box/box.lua
> @@ -1,5 +1,28 @@
>  #!/usr/bin/env tarantool
>  os = require('os')
> +local enable_before_replace = arg[1] ~= nil
> +
> +print('\n\n\n', enable_before_replace, '\n\n\n')
> +
> +function test_before_replace_trig(old, new)
> +    return new:update{{'+', 2, 1}}
> +end
> +
> +function space_on_replace_trig(old, new)
> +    if new and new[3] == 'test_on_schema_init' then
> +        box.on_commit(function()
> +            box.space.test_on_schema_init:before_replace(test_before_replace_trig)
> +        end)
> +    end
> +end
> +
> +function on_init_trig()
> +    if enable_before_replace then
> +        box.space._space:on_replace(space_on_replace_trig)
> +    end
> +end
> +
> +box.ctl.on_schema_init(on_init_trig)
>  
>  box.cfg{
>      listen              = os.getenv("LISTEN"),

Please run the test in a separate instance instead of patching the
default configuration. This will allow us to remove one 'restart',
thus decreasing the test execution time.

Other than that, the patch looks fine to me.