[tarantool-patches] [PATCH v4 3/4] box: do not evaluate ck constraints on recovery
Kirill Shcherbatov
kshcherbatov at tarantool.org
Tue Oct 8 16:02:16 MSK 2019
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 | 12 +++++-
src/box/box.cc | 1 +
src/box/ck_constraint.c | 5 ++-
src/box/ck_constraint.h | 9 ++++-
src/box/engine.c | 21 +++++++++++
src/box/schema.cc | 2 +
src/box/schema.h | 3 ++
test/sql/checks.result | 79 ++++++++++++++++++++++++++++++++++++++++
test/sql/checks.test.lua | 30 +++++++++++++++
9 files changed, 157 insertions(+), 5 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index f8f1802d0..1f3d540b5 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -54,6 +54,7 @@
#include "xrow.h"
#include "iproto_constants.h"
#include "identifier.h"
+#include "memtx_engine.h"
#include "version.h"
#include "sequence.h"
#include "sql.h"
@@ -1558,7 +1559,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);
@@ -4705,6 +4706,9 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
* ck constraint doesn't require the object
* rebuilding.
*/
+ struct memtx_engine *metmx_engine =
+ (struct memtx_engine *)engine_by_name("memtx");
+ bool is_force_recovery = metmx_engine->snap_dir.force_recovery;
const char *name = ck_def->name;
struct ck_constraint *old_ck_constraint =
space_ck_constraint_by_name(space, name, strlen(name));
@@ -4716,6 +4720,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 +4736,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 40920e649..771217020 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2139,6 +2139,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 fafa7be12..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,7 @@ 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 =
diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
index 6761318d6..f1deed465 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 overriden 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/engine.c b/src/box/engine.c
index 8dc0df1d0..2d5ff3c89 100644
--- a/src/box/engine.c
+++ b/src/box/engine.c
@@ -32,6 +32,9 @@
#include <stdint.h>
#include <string.h>
+#include "schema.h"
+#include "trigger.h"
+#include "ck_constraint.h"
#include <small/rlist.h>
RLIST_HEAD(engines);
@@ -113,6 +116,17 @@ engine_begin_final_recovery(void)
return 0;
}
+static int
+space_enable_ck_constraints(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;
+}
+
int
engine_end_recovery(void)
{
@@ -125,6 +139,13 @@ engine_end_recovery(void)
if (engine->vtab->end_recovery(engine) != 0)
return -1;
}
+ /*
+ * We also have to iterate over all spaces an turn
+ * constraints on - they were disabled in case of normal
+ * recovery.
+ */
+ if (space_foreach(space_enable_ck_constraints, NULL) != 0)
+ unreachable();
return 0;
}
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 8d8aae448..c4cde0fd6 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -77,6 +77,8 @@ struct rlist on_alter_func = RLIST_HEAD_INITIALIZER(on_alter_func);
struct entity_access entity_access;
+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..8898a6e42 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -48,6 +48,9 @@ extern uint32_t space_cache_version;
/** Triggers invoked after schema initialization. */
extern struct rlist on_schema_init;
+/** 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.
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