From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8644D41D0BD for ; Thu, 17 Oct 2019 16:58:26 +0300 (MSK) Date: Thu, 17 Oct 2019 16:58:25 +0300 From: Nikita Pettik Message-ID: <20191017135825.GF23167@tarantool.org> References: <4eb8f545449842bc4c468ccf50c494e4c44c32d6.1570539526.git.kshcherbatov@tarantool.org> <20191013125109.GA24391@atlas> <7114925b-190a-4f0d-409f-974d2e6a65dd@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <7114925b-190a-4f0d-409f-974d2e6a65dd@tarantool.org> Subject: Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery List-Id: Tarantool development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@freelists.org Cc: tarantool-patches@dev.tarantool.org On 15 Oct 14:13, Kirill Shcherbatov wrote: > > Tarantool used to validate ck constraints for all spaces on each > recovery. It is not good practice, because the recovery process > should be fast and data in snapshot is typically valid. > The new practice coincides with secondary keys building practice: > we do not perform consistency checks by default, except the > force_recovery = true option is specified. > > Closes #4176 Note that I haven't pushed this patch. It is not required to fix #4176, it is rather optimization issue. And there are still things to work on them. > --- > src/box/alter.cc | 8 +++- > src/box/box.cc | 8 ++-- > src/box/ck_constraint.c | 6 ++- > src/box/ck_constraint.h | 9 ++++- > src/box/schema.cc | 3 ++ > src/box/schema.h | 9 +++++ > src/box/space.c | 22 +++++++++++ > src/box/space.h | 7 ++++ > test/sql/checks.result | 79 ++++++++++++++++++++++++++++++++++++++++ > test/sql/checks.test.lua | 30 +++++++++++++++ > 10 files changed, 173 insertions(+), 8 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index f8f1802d0..83d297665 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1558,7 +1558,7 @@ RebuildCkConstraints::prepare(struct alter_space *alter) > link) { > struct ck_constraint *new_ck_constraint = > ck_constraint_new(old_ck_constraint->def, > - alter->new_space->def); > + alter->new_space->def, true); > if (new_ck_constraint == NULL) > diag_raise(); > rlist_add_entry(&ck_constraint, new_ck_constraint, link); > @@ -4716,6 +4716,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) > if (old_def->language == ck_def->language && > strcmp(old_def->expr_str, ck_def->expr_str) == 0) { > old_def->is_enabled = ck_def->is_enabled; > + old_ck_constraint->is_enabled = > + is_force_recovery || ck_def->is_enabled; You made is_force_recovery be global. However, there's a lot of functions taking force_recovery param. Please, refactor them: remove that argument and use instead global status. What is more, you don't set is_force_recovery to false after recovery process is finished. What is more, AFAIR we discussed replacement of memtx_recovery_state with one global state shared among all engines (instead, you added is_recovery_complete). > trigger_run_xc(&on_alter_space, space); > return; > } > @@ -4730,7 +4732,9 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) > name, "referencing space must be empty"); > } > struct ck_constraint *new_ck_constraint = > - ck_constraint_new(ck_def, space->def); > + ck_constraint_new(ck_def, space->def, > + is_recovery_complete || > + is_force_recovery); > if (new_ck_constraint == NULL) > diag_raise(); > ck_def_guard.is_active = false; > diff --git a/src/box/box.cc b/src/box/box.cc > index baf029a09..85ae78b57 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1692,9 +1692,9 @@ engine_init() > * in checkpoints (in enigne_foreach order), > * so it must be registered first. > */ > + is_force_recovery = cfg_geti("force_recovery"); > struct memtx_engine *memtx; > - memtx = memtx_engine_new_xc(cfg_gets("memtx_dir"), > - cfg_geti("force_recovery"), > + memtx = memtx_engine_new_xc(cfg_gets("memtx_dir"), is_force_recovery, > cfg_getd("memtx_memory"), > cfg_geti("memtx_min_tuple_size"), > cfg_geti("strip_core"), > @@ -210,7 +210,7 @@ ck_constraint_on_replace_trigger(struct trigger *trigger, void *event) > > struct ck_constraint * > ck_constraint_new(struct ck_constraint_def *ck_constraint_def, > - struct space_def *space_def) > + struct space_def *space_def, bool is_enabled) > { > if (space_def->field_count == 0) { > diag_set(ClientError, ER_UNSUPPORTED, "Tarantool", > @@ -225,6 +225,8 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def, > } > ck_constraint->def = NULL; > ck_constraint->stmt = NULL; > + ck_constraint->is_enabled = ck_constraint_def->is_enabled && is_enabled; > + > rlist_create(&ck_constraint->link); > struct Expr *expr = > sql_expr_compile(sql_get(), ck_constraint_def->expr_str, > diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h > index 6761318d6..836ff2822 100644 > --- a/src/box/ck_constraint.h > +++ b/src/box/ck_constraint.h > @@ -98,6 +98,12 @@ struct ck_constraint { > * message when ck condition unsatisfied. > */ > struct sql_stmt *stmt; > + /** > + * Dispite ck_constraint_def::is_enabled property this > + * option defines a state of the runtine ck constraint > + * object and may be overridden manually. > + */ > + bool is_enabled; Duplication of is_enabled status is total mess IMHO. It is required only for recovery. Please, avoid introducing this flag.