From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Serge Petrenko Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_A389DEF2-D52D-45FB-85A0-90304FFD26E1" Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: [tarantool-patches] Re: [PATCH] lua/trigger: cleanup lua stack after trigger run Date: Fri, 5 Jul 2019 14:20:47 +0300 In-Reply-To: <20190705090017.2qwzcfbyiwlkkjbj@esperanza> References: <20190625143250.97948-1-sergepetrenko@tarantool.org> <20190705090017.2qwzcfbyiwlkkjbj@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: --Apple-Mail=_A389DEF2-D52D-45FB-85A0-90304FFD26E1 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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=3D 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] =3D=3D 1 + 16999 + 17000 + 1 or s:get{1}[2] -- Serge Petrenko sergepetrenko@tarantool.org > 5 =D0=B8=D1=8E=D0=BB=D1=8F 2019 =D0=B3., =D0=B2 12:00, Vladimir = Davydov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= (=D0=B0): >=20 > 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 @@ _ =3D s:insert{1} >> save_type >>=20 >> s:drop() >> + >> +-- >> +-- gh-4275 segfault if before_replace >> +-- trigger set in on_schema_init triggers runs on recovery. >> +-- >> +s =3D box.schema.space.create('test_on_schema_init') >> +_ =3D s:create_index('pk') >> +test_run:cmd('setopt delimiter ";"') >> +function gen_inserts() >> + box.begin() >> + for i =3D 1,1000 do >> + s:upsert({1, 1}, {{'+', 2, 1}}) >> + end >> + box.commit() >> +end; >> +test_run:cmd('setopt delimiter ""'); >> +for i =3D 1,66 do gen_inserts() end >> +test_run:cmd('restart server default with args=3D"true"') >> +s =3D 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 =3D 132000 >> +s:get{1}[2] =3D=3D 2 * 66000 or s:get{1}[2] >> +test_run:cmd('restart server default with cleanup=3D1') >> 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 =3D require('os') >> +local enable_before_replace =3D arg[1] ~=3D 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] =3D=3D '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) >>=20 >> box.cfg{ >> listen =3D os.getenv("LISTEN"), >=20 > 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. >=20 > Other than that, the patch looks fine to me. >=20 --Apple-Mail=_A389DEF2-D52D-45FB-85A0-90304FFD26E1 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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=3D 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] =3D=3D = 1 + 16999 + 17000 + 1 or s:get{1}[2]




5= =D0=B8=D1=8E=D0=BB=D1=8F 2019 =D0=B3., =D0=B2 12:00, Vladimir Davydov = <vdavydov.dev@gmail.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0):

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 @@ _ = =3D s:insert{1}
save_type

= s:drop()
+
+--
+-- gh-4275 = segfault if before_replace
+-- trigger set in = on_schema_init triggers runs on recovery.
+--
+s =3D box.schema.space.create('test_on_schema_init')
+_ =3D s:create_index('pk')
+test_run:cmd('setopt= delimiter ";"')
+function gen_inserts()
+ =    box.begin()
+    for i =3D = 1,1000 do
+ =        s:upsert({1, 1}, {{'+', 2, = 1}})
+    end
+ =    box.commit()
+end;
+test_run:cmd('setopt delimiter ""');
+for i =3D = 1,66 do gen_inserts() end
+test_run:cmd('restart server = default with args=3D"true"')
+s =3D = 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 =3D 132000
+s:get{1}[2] =3D=3D 2 * = 66000 or s:get{1}[2]
+test_run:cmd('restart server default = with cleanup=3D1')
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 =3D require('os')
+local enable_before_replace =3D arg[1] ~=3D 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] =3D=3D = 'test_on_schema_init' then
+ =        box.on_commit(function()
+ =            box.spac= e.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(spac= e_on_replace_trig)
+    end
+end
+
+box.ctl.on_schema_init(on_init_trig)

box.cfg{
    listen =             &n= bsp;=3D 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.


= --Apple-Mail=_A389DEF2-D52D-45FB-85A0-90304FFD26E1--