[Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery

[Nikita Pettik]

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.