From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Serge Petrenko Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_39D1083D-D255-4091-A602-70F919D5D03A" Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: Re: [tarantool-patches] [PATCH] memtx: add yields during index build Date: Thu, 23 May 2019 17:13:21 +0300 In-Reply-To: <20190522162821.dahml7osnimcqnqy@esperanza> References: <20190522161142.44456-1-sergepetrenko@tarantool.org> <20190522162821.dahml7osnimcqnqy@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: --Apple-Mail=_39D1083D-D255-4091-A602-70F919D5D03A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 22 =D0=BC=D0=B0=D1=8F 2019 =D0=B3., =D0=B2 19:28, Vladimir Davydov = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0= ): >=20 > On Wed, May 22, 2019 at 07:11:42PM +0300, Serge Petrenko wrote: >> Memtx index build used to stall event loop for all the build period. >> Add occasional yields so that the loop is not blocked for too long. >>=20 >> Closes #3976 >> --- >> https://github.com/tarantool/tarantool/issues/3976 >> = https://github.com/tarantool/tarantool/tree/sp/gh-3976-background-index-bu= ild >>=20 >> src/box/memtx_space.c | 13 +++++ >> test/box/memtx_background_index_build.result | 55 = +++++++++++++++++++ >> .../box/memtx_background_index_build.test.lua | 32 +++++++++++ >> test/box/suite.ini | 2 +- >> 4 files changed, 101 insertions(+), 1 deletion(-) >> create mode 100644 test/box/memtx_background_index_build.result >> create mode 100644 test/box/memtx_background_index_build.test.lua >>=20 >> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c >> index 5ddb4f7ee..b90e2707e 100644 >> --- a/src/box/memtx_space.c >> +++ b/src/box/memtx_space.c >> @@ -874,6 +874,15 @@ static int >> memtx_space_build_index(struct space *src_space, struct index = *new_index, >> struct tuple_format *new_format) >> { >> + /* >> + * Yield every 1K tuples. >> + * In debug mode yield more often for testing purposes. >> + */ >> +#ifdef NDEBUG >> + enum { YIELD_LOOPS =3D 1000 }; >> +#else >> + enum { YIELD_LOOPS =3D 10 }; >> +#endif >> /** >> * If it's a secondary key, and we're not building them >> * yet (i.e. it's snapshot recovery for memtx), do nothing. >> @@ -909,6 +918,7 @@ memtx_space_build_index(struct space *src_space, = struct index *new_index, >> /* Build the new index. */ >> int rc; >> struct tuple *tuple; >> + size_t count =3D 0; >> while ((rc =3D iterator_next(it, &tuple)) =3D=3D 0 && tuple !=3D = NULL) { >> /* >> * Check that the tuple is OK according to the >> @@ -933,6 +943,9 @@ memtx_space_build_index(struct space *src_space, = struct index *new_index, >> */ >> if (new_index->def->iid =3D=3D 0) >> tuple_ref(tuple); >> + if (++count % YIELD_LOOPS =3D=3D 0) { >> + fiber_sleep(0); >> + } >=20 > This isn't enough as tuples may be inserted into / deleted from the > space while DDL is in progress. We need to propagate these changes > to the new index using on_replace trigger. Take a look at how vinyl > handles it. Thank you for pointing this out! Please see v2 in the mailing list.= --Apple-Mail=_39D1083D-D255-4091-A602-70F919D5D03A Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

22 =D0=BC=D0=B0=D1=8F 2019 =D0=B3., =D0=B2 19:28, Vladimir = Davydov <vdavydov.dev@gmail.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0):

On Wed, May = 22, 2019 at 07:11:42PM +0300, Serge Petrenko wrote:
Memtx = index build used to stall event loop for all the build period.
Add occasional yields so that the loop is not blocked for too = long.

Closes #3976
---
https://github.com/tarantool/tarantool/issues/3976
https://github.com/tarantool/tarantool/tree/sp/gh-3976-backgrou= nd-index-build

src/box/memtx_space.c = &n= bsp; | = 13 +++++
test/box/memtx_background_index_build.result = | 55 +++++++++++++++++++
.../box/memtx_background_index_build.test.lua | 32 = +++++++++++
test/box/suite.ini = &n= bsp; &nbs= p; | 2 +-
4 files changed, 101 = insertions(+), 1 deletion(-)
create mode 100644 = test/box/memtx_background_index_build.result
create mode = 100644 test/box/memtx_background_index_build.test.lua

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 5ddb4f7ee..b90e2707e 100644
--- = a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -874,6 +874,15 @@ static int
memtx_space_build_index(struct space *src_space, struct index = *new_index,
struct tuple_format = *new_format)
{
+ /*
+ * Yield every 1K tuples.
+ = * In debug = mode yield more often for testing purposes.
+ */
+#ifdef = NDEBUG
+ enum { YIELD_LOOPS =3D 1000 };
+#else
+ = enum { YIELD_LOOPS =3D 10 };
+#endif
= /**
* If it's a secondary key, = and we're not building them
* yet (i.e. it's snapshot = recovery for memtx), do nothing.
@@ -909,6 +918,7 @@ = memtx_space_build_index(struct space *src_space, struct index = *new_index,
/* Build the new index. */
= int rc;
struct tuple *tuple;
+ = size_t count =3D 0;
while ((rc =3D iterator_next(it, = &tuple)) =3D=3D 0 && tuple !=3D NULL) {
/*
= = * Check that = the tuple is OK according to the
@@ -933,6 +943,9 @@ = memtx_space_build_index(struct space *src_space, struct index = *new_index,
*/
if = (new_index->def->iid =3D=3D 0)
= tuple_ref(tuple);
+ if (++count % YIELD_LOOPS =3D=3D = 0) {
+ fiber_sleep(0);
+ }

This isn't enough as tuples may be inserted into / deleted = from the
space while = DDL is in progress. We need to propagate these changes
to the new index using = on_replace trigger. Take a look at how vinyl
handles = it.

Thank = you for pointing this out! Please see v2 in the mailing = list.

= --Apple-Mail=_39D1083D-D255-4091-A602-70F919D5D03A--