[tarantool-patches] Re: [PATCH v2 1/3] box: an ability to disable CK constraints
Nikita Pettik
korablev at tarantool.org
Thu Sep 12 17:00:45 MSK 2019
On 12 Sep 11:06, Kirill Shcherbatov wrote:
> @TarantoolBot document
> Title: an ability to disable CK constraints
>
> Now it is possible to disable and enable ck constraints.
> This option is not persistent. All ck constraints are enabled
> by default when Tarantool is configured. Ck constraints checks
> are not performed during standard recovery, but performed during
> force_recovery - all conflicting tuples are skipped in case of
> ck_constraint conflict.
Please, add reasoning. Please, explain why do we need per constraint
options? Why not disable them all at once? How to disable constraint
in SQL?
What is more, I'd split this patch into two: first one introduces
'is_enabled' property; second one provides check execution during
force recovery.
>
> Example:
> box.space.T6.ck_constraint.ck_unnamed_T6_1:enable(false)
> box.space.T6.ck_constraint.ck_unnamed_T6_1
> - space_id: 512
> is_enabled: false
> name: ck_unnamed_T6_1
> expr: a < 10
> x.space.T6:insert({11})
> -- passed
>
> Closes #4244
> ---
> extra/exports | 1 +
> src/box/ck_constraint.c | 23 +++++++-
> src/box/ck_constraint.h | 19 +++++++
> src/box/lua/schema.lua | 13 +++++
> src/box/lua/space.cc | 3 ++
> src/box/memtx_engine.c | 15 ++++++
> test/sql/checks.result | 112 +++++++++++++++++++++++++++++++++++++++
> test/sql/checks.test.lua | 41 ++++++++++++++
> 8 files changed, 226 insertions(+), 1 deletion(-)
>
> diff --git a/extra/exports b/extra/exports
> index 7b84a1452..ecc7d102b 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -78,6 +78,7 @@ tarantool_exit
> log_pid
> space_by_id
> space_run_triggers
> +space_ck_constraint_enable
> space_bsize
> box_schema_version
>
> diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
> index 1cde27022..f65715096 100644
> --- a/src/box/ck_constraint.c
> +++ b/src/box/ck_constraint.c
> @@ -28,6 +28,7 @@
> * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> * SUCH DAMAGE.
> */
> +#include "memtx_engine.h"
> #include "box/session.h"
> #include "bind.h"
> #include "ck_constraint.h"
> @@ -201,7 +202,8 @@ 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_program_run(ck_constraint, field_ref) != 0)
> + if (ck_constraint->is_enabled &&
> + ck_constraint_program_run(ck_constraint, field_ref) != 0)
> diag_raise();
> }
> }
> @@ -223,6 +225,13 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def,
> }
> ck_constraint->def = NULL;
> ck_constraint->stmt = NULL;
> +
> + struct memtx_engine *memtx =
> + (struct memtx_engine *)engine_by_name("memtx");
> + assert(memtx != NULL);
Wait a second, how it is supposed to work with vinyl?
> + bool is_recovery = memtx->state != MEMTX_OK;
Supply this code with comment.
> + ck_constraint->is_enabled = !is_recovery;
> +
> rlist_create(&ck_constraint->link);
> struct Expr *expr =
> sql_expr_compile(sql_get(), ck_constraint_def->expr_str,
> @@ -269,3 +278,15 @@ space_ck_constraint_by_name(struct space *space, const char *name,
> }
> return NULL;
> }
> +
> +int
> +space_ck_constraint_enable(struct space *space, const char *name,
> + bool is_enabled)
> +{
Nit: method is calles "enable", but it can also disable trigger.
I'd rename it to "_set_status"/"set_state" etc
> + struct ck_constraint *ck =
> + space_ck_constraint_by_name(space, name, strlen(name));
> + if (ck == NULL)
> + return -1;
> + ck->is_enabled = is_enabled;
> + return 0;
> +}
> diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
> index f26f77a38..4f2a3c20e 100644
> --- a/src/box/ck_constraint.h
> +++ b/src/box/ck_constraint.h
> @@ -93,6 +93,12 @@ struct ck_constraint {
> * message when ck condition unsatisfied.
> */
> struct sql_stmt *stmt;
> + /**
> + * Whether the CK constraint object is enabled: the
> + * checks wouldn't be performed when this flag is
> + * set 'false'.
->
/**
* 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;
> /**
> * Organize check constraint structs into linked list
> * with space::ck_constraint.
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index d0a7e7815..2c686e818 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -205,6 +205,9 @@ lbox_push_ck_constraint(struct lua_State *L, struct space *space, int i)
> lua_pushstring(L, ck_constraint->def->expr_str);
> lua_setfield(L, -2, "expr");
>
> + lua_pushboolean(L, ck_constraint->is_enabled);
> + lua_setfield(L, -2, "is_enabled");
> +
> lua_setfield(L, -2, ck_constraint->def->name);
> }
> lua_pop(L, 1);
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index f6a33282c..819571819 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -35,6 +35,7 @@
> #include <small/small.h>
> #include <small/mempool.h>
>
> +#include "ck_constraint.h"
> #include "fiber.h"
> #include "errinj.h"
> #include "coio_file.h"
> @@ -86,6 +87,18 @@ memtx_end_build_primary_key(struct space *space, void *param)
> return 0;
> }
>
> +static int
> +space_run_ck_constraints(struct space *space, void *param)
> +{
> + if (space_is_system(space))
> + return 0;
> + struct ck_constraint *ck;
> + rlist_foreach_entry(ck, &space->ck_constraint, link)
> + ck->is_enabled = (bool)param;
> + (void)trigger_run(&on_alter_space, space);
> + return 0;
> +}
> +
> /**
> * Secondary indexes are built in bulk after all data is
> * recovered. This function enables secondary keys on a space.
> @@ -315,6 +328,8 @@ memtx_engine_end_recovery(struct engine *engine)
> if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
> return -1;
> }
> + if (space_foreach(space_run_ck_constraints, (void *)true) != 0)
> + unreachable();
What does this change do? Why do you need param if it is always true?
Is space_is_system() check vital?
> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index cde213f8b..9716647d0 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -234,4 +234,45 @@ s2:drop()
> physics_ck
> physics_ck:drop()
>
> +--
> +-- gh-4244: An ability to disable CK constraints
Nit: add an ability ...
> +-- Make shure that ck constraints are turning on and of with
-> sure; are turned on/off
> +-- :enable configurator.
> +--
> +engine = test_run:get_cfg('engine')
> +box.execute('pragma sql_default_engine=\''..engine..'\'')
> +box.execute("CREATE TABLE test(a INT CHECK (a < 5) primary key);");
> +box.space.TEST:insert({10})
> +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})
> +-- Enshure that ck constraints are not validated during
-> Ensure
> +-- 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;")
> +
> +-- Enshure 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")
Could you make result of test be clearer? From .result file it is not
obvious that check verification failed.
> +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()
> +box.execute("DROP TABLE test;")
> +
> +test_run = require('test_run').new()
> +test_run:cmd('switch default')
> +test_run:cmd('stop server test')
> +
> test_run:cmd("clear filter")
More information about the Tarantool-patches
mailing list