[tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery
Kirill Shcherbatov
kshcherbatov at tarantool.org
Tue Oct 15 14:13:49 MSK 2019
> Please add an engine wrapper around memtx_engine.h
>
> engine.h:
>
> engine_is_force_recovery() {
> return memtx_engine->snap_dir.is_force_recovery;
> }
>> + /**
Discussed with Kostya in T.; I've implemented it another way.
>> + * object and may be overriden manually.
>
> overridden
Fixed.
> Having to add 3 header files for 3 lines of code should raise a
> flag.
>
> Why do you define this method in engine.h? Can you define it
> elsewhere so that there are fewer dependencies? schema.h? space.h?
Introduced a new helper in space.h
void space_initialize(void)
=============================================
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
---
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;
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"),
@@ -1713,7 +1713,7 @@ engine_init()
cfg_geti64("vinyl_memory"),
cfg_geti("vinyl_read_threads"),
cfg_geti("vinyl_write_threads"),
- cfg_geti("force_recovery"));
+ is_force_recovery);
engine_register((struct engine *)vinyl);
box_set_vinyl_max_tuple_size();
box_set_vinyl_cache();
@@ -1805,6 +1805,7 @@ bootstrap_from_master(struct replica *master)
/* Finalize the new replica */
engine_end_recovery_xc();
+ space_initialize();
/* Switch applier to initial state */
applier_resume_to_state(applier, APPLIER_READY, TIMEOUT_INFINITY);
@@ -2147,6 +2148,7 @@ box_cfg_xc(void)
bootstrap(&instance_uuid, &replicaset_uuid,
&is_bootstrap_leader);
}
+ is_recovery_complete = true;
fiber_gc();
/* Check for correct registration of the instance in _cluster */
diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
index d21717f94..d7a5be7fb 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -202,7 +202,7 @@ ck_constraint_on_replace_trigger(struct trigger *trigger, void *event)
struct ck_constraint *ck_constraint;
rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) {
- if (ck_constraint->def->is_enabled &&
+ if (ck_constraint->is_enabled &&
ck_constraint_program_run(ck_constraint, field_ref) != 0)
diag_raise();
}
@@ -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;
/**
* Organize check constraint structs into linked list
* with space::ck_constraint.
@@ -176,12 +182,13 @@ ck_constraint_def_delete(struct ck_constraint_def *ck_def);
* creation.
* @param space_def The space definition of the space this check
* constraint must be constructed for.
+ * @param is_enabled Whether this ck constraint should be fired.
* @retval not NULL Check constraint object pointer on success.
* @retval NULL Otherwise. The diag message is set.
*/
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);
/**
* Destroy check constraint memory, release acquired resources.
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 8d8aae448..fd3eca0a0 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -77,6 +77,9 @@ struct rlist on_alter_func = RLIST_HEAD_INITIALIZER(on_alter_func);
struct entity_access entity_access;
+bool is_force_recovery = false;
+bool is_recovery_complete = false;
+
bool
space_is_system(struct space *space)
{
diff --git a/src/box/schema.h b/src/box/schema.h
index f9d15b38d..efe3c93a1 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -48,6 +48,12 @@ extern uint32_t space_cache_version;
/** Triggers invoked after schema initialization. */
extern struct rlist on_schema_init;
+/** True when engine is initialized in force recovery mode. */
+extern bool is_force_recovery;
+
+/** True when recovery is finished. */
+extern bool is_recovery_complete;
+
/**
* Try to look up a space by space number in the space cache.
* FFI-friendly no-exception-thrown space lookup function.
@@ -139,6 +145,9 @@ int
schema_find_id(uint32_t system_space_id, uint32_t index_id, const char *name,
uint32_t len, uint32_t *object_id);
+int
+space_enable_ck_constraints(struct space *space, void *param);
+
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/space.c b/src/box/space.c
index 042be042c..562ce43ff 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -617,6 +617,28 @@ space_remove_ck_constraint(struct space *space, struct ck_constraint *ck)
}
}
+static inline int
+space_initialize_cb(struct space *space, void *param)
+{
+ (void) param;
+ struct ck_constraint *ck;
+ rlist_foreach_entry(ck, &space->ck_constraint, link)
+ ck->is_enabled = ck->def->is_enabled;
+ (void) trigger_run(&on_alter_space, space);
+ return 0;
+}
+
+void
+space_initialize(void)
+{
+ /**
+ * Reset each ck constraint is_enabled state to
+ * a default value defined in ck constraint definition.
+ */
+ if (space_foreach(space_initialize_cb, NULL) != 0)
+ unreachable();
+}
+
/* {{{ Virtual method stubs */
size_t
diff --git a/src/box/space.h b/src/box/space.h
index 7926aa65e..2b31caea7 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -516,6 +516,13 @@ space_add_ck_constraint(struct space *space, struct ck_constraint *ck);
void
space_remove_ck_constraint(struct space *space, struct ck_constraint *ck);
+/**
+ * Iterate over all spaces and initialize all related runtime
+ * objects.
+ */
+void
+space_initialize(void);
+
/*
* Virtual method stubs.
*/
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 6d584c301..b8bd19a84 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -787,9 +787,88 @@ box.space.TEST:insert({13})
---
- error: 'Check constraint failed ''CK'': a < 5'
...
+-- Ensure that ck constraints are not validated during
+-- normal recovery. Now TEST space has a tuple {12} violating
+-- defined CK constraint.
+test_run:cmd("restart server default")
+box.space.TEST:select()
+---
+- - [11]
+ - [12]
+...
box.space.TEST:drop()
---
...
+-- Ensure that ck constraints are validated during
+-- force recovery.
+test_run:cmd('create server test with script = "xlog/force_recovery.lua"')
+---
+- true
+...
+test_run:cmd("start server test")
+---
+- true
+...
+test_run:cmd("switch test")
+---
+- true
+...
+engine = test_run:get_cfg('engine')
+---
+...
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+---
+- row_count: 0
+...
+box.execute("CREATE TABLE test(a INT PRIMARY KEY);");
+---
+- row_count: 1
+...
+box.execute('ALTER TABLE test ADD CONSTRAINT CK CHECK(a < 10);')
+---
+- row_count: 1
+...
+box.space.TEST.ck_constraint.CK:enable(false)
+---
+...
+box.space.TEST:insert({11})
+---
+- error: 'Check constraint failed ''CK'': a < 10'
+...
+box.space.TEST:insert({2})
+---
+- [2]
+...
+box.space.TEST.ck_constraint.CK:enable(true)
+---
+...
+box.space.TEST:insert({33})
+---
+- error: 'Check constraint failed ''CK'': a < 10'
+...
+test_run:cmd("restart server test")
+box.space.TEST:select()
+---
+- - [2]
+...
+test_run:wait_log('test', 'ER_CK_CONSTRAINT_FAILED: Check constraint failed \'CK\': a < 10', nil, 5)
+---
+...
+box.execute("DROP TABLE test;")
+---
+- row_count: 1
+...
+test_run = require('test_run').new()
+---
+...
+test_run:cmd('switch default')
+---
+- true
+...
+test_run:cmd('stop server test')
+---
+- true
+...
--
-- Test ENABLE/DISABLE CK constraints from SQL works.
--
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index f8dc5d363..832322190 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -255,8 +255,38 @@ box.space.TEST:insert({12})
box.space.TEST.ck_constraint.CK:enable(true)
assert(box.space.TEST.ck_constraint.CK.is_enabled == true)
box.space.TEST:insert({13})
+-- Ensure that ck constraints are not validated during
+-- normal recovery. Now TEST space has a tuple {12} violating
+-- defined CK constraint.
+test_run:cmd("restart server default")
+box.space.TEST:select()
box.space.TEST:drop()
+-- Ensure that ck constraints are validated during
+-- force recovery.
+test_run:cmd('create server test with script = "xlog/force_recovery.lua"')
+test_run:cmd("start server test")
+test_run:cmd("switch test")
+
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+
+box.execute("CREATE TABLE test(a INT PRIMARY KEY);");
+box.execute('ALTER TABLE test ADD CONSTRAINT CK CHECK(a < 10);')
+box.space.TEST.ck_constraint.CK:enable(false)
+box.space.TEST:insert({11})
+box.space.TEST:insert({2})
+box.space.TEST.ck_constraint.CK:enable(true)
+box.space.TEST:insert({33})
+test_run:cmd("restart server test")
+box.space.TEST:select()
+test_run:wait_log('test', 'ER_CK_CONSTRAINT_FAILED: Check constraint failed \'CK\': a < 10', nil, 5)
+box.execute("DROP TABLE test;")
+
+test_run = require('test_run').new()
+test_run:cmd('switch default')
+test_run:cmd('stop server test')
+
--
-- Test ENABLE/DISABLE CK constraints from SQL works.
--
--
2.23.0
More information about the Tarantool-patches
mailing list