[tarantool-patches] Re: [PATCH v2 5/9] schema: add new system space for CHECK constraints
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Mar 22 12:29:02 MSK 2019
Thanks for the patch! See 9 comments below. It is not a
complete review, because Kirill Y. said, that the patchset
is not finished (then why is it in the patches mailing list?).
On 30/01/2019 11:59, Kirill Shcherbatov wrote:
> This patch introduces new system space to persist check
> constraints. Format of the space:
>
> _ck_constraint (space id = 357)
> [<constraint name> STR, <space id> UINT, <expression string>STR]
>
> CK constraint is local to space, so every pair <CK name, space id>
> is unique (and it is PK in _ck_constraint space).
>
> After insertion into this space, a new instance describing check
> constraint is created. Check constraint held Expr tree.
1. held -> hold
> Until space features check constraints, it isn't allowed to
> be dropped.
2. Sorry, the sentence is invalid. In Russian you said literally
this: "Пока спейс не поддерживает check ограничение, он не может
быть удален". But why? If a space does not have a check, it is ok
to drop the space. Probably you thought that 'Until' == 'While'?
Vice versa, rather 'While' == 'Until not'.
> The :drop() space method firstly deletes all check
> constraints and than removes entry from _space.
3. than -> then.
My piece of advice - go to English courses. Fortunately, Kirill
and Kostja appreciate such activity and can reimburse your
expenses (likely that they will).
>
> We use BuildCkConstraints Alter operation object because space
> definition may be modified and check AST must be recreated.
>
> Needed for #3691
> ---
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index eff3524cf..3ba604ca6 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1351,6 +1341,56 @@ TruncateIndex::commit(struct alter_space *alter, int64_t signature)
> index_commit_create(new_index, signature);
> }
>
> +/** BuildCkConstraints - rebuild ck_constraints on alter. */
4. Instead of the comments like you wrote below, it would be
much better to focus your efforts on the code like this. Here
I do not understand what is happening, especially in prepare().
> +class BuildCkConstraints: public AlterSpaceOp
> +{
> +public:
> + BuildCkConstraints(struct alter_space *alter)
> + : AlterSpaceOp(alter),
> + ck_constraint(RLIST_HEAD_INITIALIZER(ck_constraint)) {}
> + struct rlist ck_constraint;
> + virtual void prepare(struct alter_space *alter);
> + virtual void alter(struct alter_space *alter);
> + virtual void rollback(struct alter_space *alter);
> + virtual ~BuildCkConstraints();
> +};
> +
> +void
> +BuildCkConstraints::prepare(struct alter_space *alter)
> +{
> + struct ck_constraint *old_ck_constraint;
> + rlist_foreach_entry(old_ck_constraint, &alter->old_space->ck_constraint,
> + link) {
> + struct ck_constraint *new_ck_constraint =
> + ck_constraint_new(old_ck_constraint->def,
> + alter->new_space->def);
> + if (new_ck_constraint == NULL)
> + diag_raise();
> + rlist_add_entry(&ck_constraint, new_ck_constraint, link);
> + }
> +}
> +
> +void
> +BuildCkConstraints::alter(struct alter_space *alter)
> +{
> + rlist_swap(&alter->new_space->ck_constraint, &ck_constraint);
> + rlist_swap(&ck_constraint, &alter->old_space->ck_constraint);
> +}
> +
> +void
> +BuildCkConstraints::rollback(struct alter_space *alter)
> +{
> + rlist_swap(&alter->old_space->ck_constraint, &ck_constraint);
> + rlist_swap(&ck_constraint, &alter->new_space->ck_constraint);
> +}
> +
> +BuildCkConstraints::~BuildCkConstraints()
> +{
> + struct ck_constraint *old_ck_constraint, *tmp;
> + rlist_foreach_entry_safe(old_ck_constraint, &ck_constraint, link, tmp)
> + ck_constraint_delete(old_ck_constraint);
> +}
> +
> /**
> * UpdateSchemaVersion - increment schema_version. Used on
> * in alter_space_do(), i.e. when creating or dropping
> @@ -1757,6 +1797,12 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
> space_name(old_space),
> "the space has foreign key constraints");
> }
> + /* Can't drop space having check constraints. */
> + if (!rlist_empty(&old_space->ck_constraint)) {
> + tnt_raise(ClientError, ER_DROP_SPACE,
> + space_name(old_space),
> + "the space has check constraints");
> + }
> /**
> * The space must be deleted from the space
> * cache right away to achieve linearisable
> @@ -1854,6 +1900,8 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
> def->field_count);
> (void) new CheckSpaceFormat(alter);
> (void) new ModifySpace(alter, def);
> + /* Add an op to rebuild check constraints. */
> + (void) new BuildCkConstraints(alter);
> def_guard.is_active = false;
> /* Create MoveIndex ops for all space indexes. */
> alter_space_move_indexes(alter, 0, old_space->index_id_max + 1);
> @@ -2096,6 +2144,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
> * old space.
> */
> alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1);
> + /* Add an op to rebuild check constraints. */
> + (void) new BuildCkConstraints(alter);
5. Why do you need checks rebuild on an index update? As I know, checks
depend on space format only. Strictly speaking, even on space alter you
do not need to rebuild checks, if the space format is unchanged. However,
I am not sure, if checks AST does not contain space pointers.
> /* Add an op to update schema_version on commit. */
> (void) new UpdateSchemaVersion(alter);
> alter_space_do(txn, alter);
> @@ -2163,7 +2213,8 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event)
> struct index *old_index = old_space->index[i];
> (void) new TruncateIndex(alter, old_index->def->iid);
> }
> -
> + /* Add an op to rebuild check constraints. */
> + (void) new BuildCkConstraints(alter);
6. Comments like that, and the 2 same ones above looks like this, sorry:
https://github.com/Gerold103/tarantool-memes/blob/master/CAPTAIN.png
How is such a comment supposed to help a reader?
> alter_space_do(txn, alter);
> scoped_guard.is_active = false;
> }
> @@ -4051,6 +4102,144 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
> +/**
> + * Trigger invoked on commit in the _ck_constraint space.
> + * Drop useless old check constraint object if exists.
> + */
> +static void
> +on_replace_ck_constraint_commit(struct trigger *trigger, void * /* event */)
> +{
> + struct ck_constraint *old_ck_constraint =
> + (struct ck_constraint *)trigger->data;
> + if (old_ck_constraint != NULL)
> + ck_constraint_delete(old_ck_constraint);
> + ++schema_version;
7. We have UpdateSchemaVersion for that. It can be touched
direcrtly only by _space triggers.
> +}
> +
> +/** A trigger invoked on replace in the _ck_constraint space. */
> +static void
> +on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
> +{
> + struct txn *txn = (struct txn *) event;
> + txn_check_singlestatement_xc(txn, "Space _ck_constraint");
> + struct txn_stmt *stmt = txn_current_stmt(txn);
> + struct tuple *old_tuple = stmt->old_tuple;
> + struct tuple *new_tuple = stmt->new_tuple;
> + uint32_t space_id =
> + tuple_field_u32_xc(old_tuple != NULL ? old_tuple : new_tuple,
> + BOX_CK_CONSTRAINT_FIELD_SPACE_ID);
> + struct space *space = space_cache_find_xc(space_id);
> + access_check_ddl(space->def->name, space->def->id, space->def->uid,
> + SC_SPACE, PRIV_A);
> +
> + struct trigger *on_rollback =
> + txn_alter_trigger_new(on_replace_ck_constraint_rollback, NULL);
> + struct trigger *on_commit =
> + txn_alter_trigger_new(on_replace_ck_constraint_commit, NULL);
> +
> + if (new_tuple != NULL) {
> + /* Create or replace check constraint. */
> + struct ck_constraint_def *ck_constraint_def =
> + ck_constraint_def_decode(new_tuple, &fiber()->gc);
> + struct ck_constraint *new_ck_constraint =
> + ck_constraint_new(ck_constraint_def, space->def);
> + if (new_ck_constraint == NULL)
> + diag_raise();
> + const char *space_name = new_ck_constraint->def->name;
> + struct ck_constraint *old_ck_constraint =
> + space_ck_constraint_by_name(space, space_name,
> + strlen(space_name));
> + if (old_ck_constraint != NULL)
> + rlist_del_entry(old_ck_constraint, link);
> + rlist_add_entry(&space->ck_constraint, new_ck_constraint, link);
> + on_commit->data = old_ck_constraint;
> + on_rollback->data = old_tuple == NULL ? new_ck_constraint :
> + old_ck_constraint;
> + } else if (new_tuple == NULL && old_tuple != NULL) {
8. As I understand, it should be an assertion, not 'if'. When new_tuple == NULL,
it is impossible that old_tuple == NULL too. What is more, if you reached 'else'
branch, then 'new_tuple == NULL' is already true, and you do not need to check
that again.
> + /* Drop check constraint. */
> + uint32_t name_len;
> + const char *name =
> + tuple_field_str_xc(old_tuple,
> + BOX_CK_CONSTRAINT_FIELD_NAME,
> + &name_len);
> + struct ck_constraint *old_ck_constraint =
> + space_ck_constraint_by_name(space, name, name_len);
> + assert(old_ck_constraint != NULL);
> + rlist_del_entry(old_ck_constraint, link);
> + on_commit->data = old_ck_constraint;
> + on_rollback->data = old_ck_constraint;
> + }
> +
> + txn_on_rollback(txn, on_rollback);
> + txn_on_commit(txn, on_commit);
> +}
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index cc172dc15..a304290a2 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -629,10 +631,46 @@ local function upgrade_priv_to_2_1_1()
> local function upgrade_to_2_1_1()
> log.info("started upgrade_to_2_1_1")
> + local _space = box.space[box.schema.SPACE_ID]
> + local _index = box.space[box.schema.INDEX_ID]
> + local _ck_constraint = box.space[box.schema.CK_CONSTRAINT_ID]
> + local MAP = setmap({})
> +
> + log.info("create space _ck_constraint")
> + local format = {{name='name', type='string'},
> + {name='space_id', type='unsigned'},
> + {name='expr_str', type='str'}}
9. Why a name is mandatory? As I know, I can omit the name, according to
the standard.
> + _space:insert{_ck_constraint.id, ADMIN, '_ck_constraint', 'memtx', 0, MAP, format}
> +
> + log.info("create index primary on _ck_constraint")
> + _index:insert{_ck_constraint.id, 0, 'primary', 'tree',
> + {unique = true}, {{0, 'string'}, {1, 'unsigned'}}}
> +
> + log.info("create secondary index child_id on _ck_constraint")
> + _index:insert{_ck_constraint.id, 1, 'space_id', 'tree',
> + {unique = false}, {{1, 'unsigned'}}}
> +
> upgrade_priv_to_2_1_1()
> end
>
More information about the Tarantool-patches
mailing list