Hi! Thank you for review!

The changes are on the branch. I also added a more informative comment to the test case.

+...
+-- For this test the number of invocations of the before_replace
+-- trigger during recovery multiplied by the amount of return values
+-- in before_replace trigger must be greater than 66000.
+-- A total of 1 + 16999 + 17000 + 1= 34001
+-- insertion: 16999 updates from gen_inserts(), starting value is 1,
+-- plus additional 17000 runs of before_replace trigger, each updating
+-- the value by adding 1.
+-- recovery: 17000 invocations of before replace trigger.
+-- Each invocation updates the recovered tuple, but only in-memory, the
+-- WAL remains intact. Thus, we only see the last update. During recovery
+-- the value is only increased by 1, and this change is visible only in memory.
+s:get{1}[2] == 1 + 16999 + 17000 + 1 or s:get{1}[2]

--
Serge Petrenko
sergepetrenko@tarantool.org




> 5 июля 2019 г., в 12:00, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> 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.
>