[tarantool-patches] [PATCH v3 3/4] box: do not evaluate ck constraints on recovery
Kirill Shcherbatov
kshcherbatov at tarantool.org
Mon Sep 16 15:47:11 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/box.cc | 1 +
src/box/ck_constraint.c | 4 ++-
src/box/ck_constraint.h | 2 ++
src/box/engine.c | 21 ++++++++++++
src/box/schema.cc | 2 ++
src/box/schema.h | 3 ++
src/box/sql/alter.c | 4 +--
test/sql/checks.result | 71 ++++++++++++++++++++++++++++++++++++++++
test/sql/checks.test.lua | 27 +++++++++++++++
9 files changed, 132 insertions(+), 3 deletions(-)
diff --git a/src/box/box.cc b/src/box/box.cc
index 40920e649..0f4582815 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_completed = 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 b422c2f3e..dabd4b1b5 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -31,6 +31,7 @@
#include "memtx_engine.h"
#include "box/session.h"
#include "bind.h"
+#include "cfg.h"
#include "ck_constraint.h"
#include "errcode.h"
#include "schema.h"
@@ -225,7 +226,8 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def,
}
ck_constraint->def = NULL;
ck_constraint->stmt = NULL;
- ck_constraint->is_enabled = true;
+ ck_constraint->is_enabled = is_recovery_completed ||
+ cfg_geti("force_recovery");
rlist_create(&ck_constraint->link);
struct Expr *expr =
diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
index 8fc2c5789..9b930a5f7 100644
--- a/src/box/ck_constraint.h
+++ b/src/box/ck_constraint.h
@@ -96,6 +96,8 @@ struct ck_constraint {
/**
* Per constraint option regulating its execution: it is
* disabled (set to false) contraint won't be fired.
+ * Note that during ordinary recovery they are turned off;
+ * during force recovery they are enabled.
*/
bool is_enabled;
/**
diff --git a/src/box/engine.c b/src/box/engine.c
index 8dc0df1d0..45ff399d4 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 = true;
+ (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..1f174f264 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_completed = false;
+
bool
space_is_system(struct space *space)
{
diff --git a/src/box/schema.h b/src/box/schema.h
index f9d15b38d..7b3cd378e 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_completed;
+
/**
* Try to look up a space by space number in the space cache.
* FFI-friendly no-exception-thrown space lookup function.
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 963d77a0a..e63fbdf2f 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -102,8 +102,8 @@ sql_alter_ck_constraint_enable(struct Parse *parse)
if (constraint_name == NULL)
goto tnt_error;
- if (space_ck_constraint_enable(space, constraint_name,
- enable_def->new_status) != 0) {
+ if (space_ck_constraint_set_state(space, constraint_name,
+ enable_def->new_status) != 0) {
diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, constraint_name);
goto tnt_error;
}
diff --git a/test/sql/checks.result b/test/sql/checks.result
index e766f88b8..121e5c061 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -762,10 +762,81 @@ box.space.TEST:insert({12})
---
- error: 'Check constraint failed ''ck_unnamed_TEST_1'': 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]
+...
+box.space.TEST:insert({12})
+---
+- error: 'Check constraint failed ''ck_unnamed_TEST_1'': a < 5'
+...
+box.execute("DROP TABLE test;")
+---
+- row_count: 1
+...
+-- 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 CHECK (a < 10) primary key);");
+---
+- row_count: 1
+...
+box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
+---
+...
+box.space.TEST:insert({11})
+---
+- [11]
+...
+box.space.TEST:insert({2})
+---
+- [2]
+...
+test_run:cmd("restart server test")
+box.space.TEST:select()
+---
+- - [2]
+...
+test_run:wait_log('default', 'ER_CK_CONSTRAINT_FAILED: Check constraint failed \'ck_unnamed_TEST_1\': 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 442c50eaa..70c5a794d 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -247,8 +247,35 @@ box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
box.space.TEST:insert({11})
box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(true)
box.space.TEST:insert({12})
+-- 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:insert({12})
box.execute("DROP TABLE test;")
+-- 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 CHECK (a < 10) primary key);");
+box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
+box.space.TEST:insert({11})
+box.space.TEST:insert({2})
+test_run:cmd("restart server test")
+box.space.TEST:select()
+test_run:wait_log('default', 'ER_CK_CONSTRAINT_FAILED: Check constraint failed \'ck_unnamed_TEST_1\': 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