[tarantool-patches] Re: [PATCH v2 5/9] schema: add new system space for CHECK constraints

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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
>