[tarantool-patches] Re: [PATCH v5 4/6] schema: add new system space for CHECK constraints

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

Thanks for the patch! See 12 comments below.

> @@ -1387,6 +1377,79 @@ UpdateSchemaVersion::alter(struct alter_space *alter)
>      ++schema_version;
>  }
>  
> +/**
> + * As ck_constraint object depends on space_def we must rebuild
> + * all ck constraints on space alter.
> + *
> + * To perform it transactionally, we create a list of new ck
> + * constraints objects in ::prepare method that is fault-tolerant.
> + * Finally in ::alter or ::rollback methods we only swap thouse

1. 'thouse' word does not exist, it is clearly visible with a
spell checker, which usually highlights unknown words.

> + * lists securely.
> + */
> +class RebuildCkConstraints: public AlterSpaceOp
> +{
> +	void space_swap_ck_constraint(struct space *old_space,
> +				      struct space *new_space);
> +public:
> +	RebuildCkConstraints(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 ~RebuildCkConstraints();
> +};
> @@ -4040,6 +4111,176 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
> +
> +/** 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);
> +	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) {
> +		bool is_deferred =
> +			tuple_field_bool_xc(new_tuple,
> +					    BOX_CK_CONSTRAINT_FIELD_DEFERRED);
> +		if (is_deferred) {
> +			tnt_raise(ClientError, ER_UNSUPPORTED, "Tarantool",
> +				  "deferred ck constraints");
> +		}
> +		/* Create or replace check constraint. */
> +		struct ck_constraint_def *ck_def =
> +			ck_constraint_def_new_from_tuple(new_tuple);
> +		auto ck_guard = make_scoped_guard([=] { free(ck_def); });

2. Please, implement and use ck_constraint_def_delete. Free() will leak, when
ck_constraint will become more complex.

> +		/*
> +		 * FIXME: Ck constraint creation on non-empty
> +		 * space must be implemented as preparatory
> +		 * step for ALTER SPACE ADD CONSTRAINT feature.
> +		 */
> +		struct index *pk = space_index(space, 0);
> +		if (pk != NULL && index_size(pk) > 0) {
> +			tnt_raise(ClientError, ER_CREATE_CK_CONSTRAINT,
> +				  ck_def->name,
> +				  "referencing space must be empty");
> +		}
> +		struct ck_constraint *new_ck_constraint =
> +			ck_constraint_new(ck_def, space->def);
> +		if (new_ck_constraint == NULL)
> +			diag_raise();
> +		ck_guard.is_active = false;
> +		const char *name = new_ck_constraint->def->name;
> +		struct ck_constraint *old_ck_constraint =
> +			space_ck_constraint_by_name(space, name, strlen(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_tuple == NULL ? new_ck_constraint :
> +						      old_ck_constraint;
> +		on_rollback->data = on_commit->data;
> diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
> new file mode 100644
> index 000000000..c74687b31
> --- /dev/null
> +++ b/src/box/ck_constraint.c
> +
> +void
> +ck_constraint_delete(struct ck_constraint *ck_constraint)
> +{
> +	sql_expr_delete(sql_get(), ck_constraint->expr, false);
> +	free(ck_constraint->def);

3. The same. Use ck_constraint_def_delete.

> +	TRASH(ck_constraint);
> +	free(ck_constraint);
> +}
> diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
> new file mode 100644
> index 000000000..e20203bb5
> --- /dev/null
> +++ b/src/box/ck_constraint.h
> +
> +/**
> + * Calculate check constraint definition memory size and fields
> + * offsets for given arguments.
> + *
> + * Alongside with struct ck_constraint_def itself, we reserve
> + * memory for string containing its name and expression string.
> + *
> + * Memory layout:
> + * +-----------------------------+ <- Allocated memory starts here
> + * |   struct ck_constraint_def  |
> + * |-----------------------------|
> + * |          name + \0          |
> + * |-----------------------------|
> + * |        expr_str + \0        |
> + * +-----------------------------+
> + *
> + * @param name_len The length of the name.
> + * @param expr_str_len The length of the expr_str.
> + * @param[out] name_offset The offset of the name string.
> + * @param[out] expr_str_offset The offset of the expr_str string.
> + * @retval The size of the ck constraint definition object for
> + *         given parameters.

4. I said it probably 2-3 times, but you still ignore. @retval takes
one parameter, @return take 0 parameters. Here you wrote, that
returned value is 'The'.

> + */
> +static inline uint32_t
> +ck_constraint_def_sizeof(uint32_t name_len, uint32_t expr_str_len,
> +			 uint32_t *name_offset, uint32_t *expr_str_offset)
> +{
> +	*name_offset = sizeof(struct ck_constraint_def);
> +	*expr_str_offset = *name_offset + name_len + 1;
> +	return *expr_str_offset + expr_str_len + 1;
> +}
> diff --git a/src/box/schema_def.h b/src/box/schema_def.h
> index dea3fad19..317e6b51f 100644
> --- a/src/box/schema_def.h
> +++ b/src/box/schema_def.h
> @@ -108,6 +108,8 @@ enum {
>  	BOX_SPACE_SEQUENCE_ID = 340,
>  	/** Space id of _fk_constraint. */
>  	BOX_FK_CONSTRAINT_ID = 356,
> +	/** Space id of _ck_contraint. */
> +	BOX_CK_CONSTRAINT_ID = 357,

5. Please, leave a gap after BOX_FK_CONSTRAINT_ID. We keep
gaps to be able in future to add a view space, and to choose
recovery order. Gap is typically 7 unused IDs. The new ID
should be 364.

>  	/** End of the reserved range of system spaces. */
>  	BOX_SYSTEM_ID_MAX = 511,
>  	BOX_ID_NIL = 2147483647
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 91b977de2..a3b11531f 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -638,40 +640,116 @@ primary_key_exit:
>  	return;
>  }
>  
> +/**
> + * Prepare a 0-terminated string in the wptr memory buffer that
> + * does not contain a sequence of more than one whatespace
> + * character. Routine enforces ' ' (space) as whitespace
> + * delimiter. When character ' or " was met, the sting is copied
> + * without any changes until the next (corres) ' or " .

6. What is 'corres'?

> + * The wptr buffer is expected to have str_len + 1 bytes
> + * (this is the expected scenario where no extra whitespace
> + * characters preset in the source string).

7. What is 'characters preset'?

> + * @param wptr The destination memory buffer of size
> + *             @a str_len + 1.
> + * @param str The source string to be copied.
> + * @param str_len The source string @a str length.
> + */
> +static void
> +trim_space_snprintf(char *wptr, const char *str, uint32_t str_len)
> @@ -1416,6 +1529,37 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
>  	sqlReleaseTempRange(parse_context, key_reg, 3);
>  }
>  
> +/**
> + * Generate VDBE program to remove entry from _ck_constraint space.
> + *
> + * @param parser Parsing context.
> + * @param ck_name Name of CK constraint to be dropped.
> + * @param child_id Id of table which constraint belongs to.

8. There is no parameter 'child_id'.

> + */
> +static void
> +vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
> +			     uint32_t space_id)
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index f241b8d52..e93dfe751 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -297,7 +297,7 @@ ccons ::= check_constraint_def .
>  
>  check_constraint_def ::= cconsname(N) CHECK LP expr(X) RP. {
>    create_ck_def_init(&pParse->create_ck_def, &N, &X);
> -  sql_add_check_constraint(pParse);
> +  sql_create_check_contraint(pParse);

9. What was a motivation of this rename? As I understand,
we can add many CHECK constraints in one CREATE TABLE. We
do not 'create' one single CHECK, we add multiple ones. Please,
keep the old name.

>  }
>  
>  ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R). {
> diff --git a/test/sql/checks.result b/test/sql/checks.result
> index f7cddec43..5f0098e23 100644
> --- a/test/sql/checks.result
> +++ b/test/sql/checks.result
> @@ -30,89 +32,216 @@ t = {513, 1, 'test', 'memtx', 0, opts, format}
> -s = box.space._space:insert(t)
> +-- Pass integer instead of expression.
> +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666})
> +---
> +- error: 'Tuple field 5 type does not match one required by operation: expected string'
> +...
> +-- Defered CK constraints are not supported.

10. 'Defered' word does not exist, use 'deferred'.
'Unexistent' word does not exist as well. Non-existent is better.

> +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5'})
> +---
> +- error: Tarantool does not support deferred ck constraints
> +...
> +-- The only supperted language is SQL.

11. 'Supperted' word does not exist.

> +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5'})
> +---
> +- error: Unsupported language 'LUA' specified for function 'CK_CONSTRAINT_01'
> +...

> ...

> +---
> +...
> +box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), CONSTRAINT CK1 CHECK(ID < 0))")
> +---
> +- error: Duplicate key exists in unique index 'primary' in space '_ck_constraint'

12. The error is not ok. It should not expose any system details like
_ck_constraint space. Please, check duplicate names before an insertion.

> +...
> +box.space._ck_constraint:select()
> +---
> +- []
>  ...