[tarantool-patches] Re: [PATCH v5 4/6] schema: add new system space for CHECK constraints
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun May 26 15:06:46 MSK 2019
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()
> +---
> +- []
> ...
More information about the Tarantool-patches
mailing list