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.