[tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov at tarantool.org> [19/05/07 12:56]:
> +/**
> + * Create VDBE machine for ck constraint by given definition and
> + * expression AST. The generated instructions consist of
> + * prologue code that maps tuple fields via bindings and ck
> + * constraint code which implements given expression.
> + * In case of ck constraint error during VDBE execution, it is
> + * aborted and error is handled as diag message.
> + * @param ck_constraint_def Check constraint definition to prepare
> + *                          error description.
> + * @param expr Check constraint expression AST is built for
> + *             given @ck_constraint_def, see for
> + *             (sql_expr_compile +
> + *              ck_constraint_resolve_space_def) implementation.
> + * @param space_def The space definition of the space this check
> + *                  constraint is constructed for.
> + * @retval not NULL sql_stmt program pointer on success.
> + * @retval NULL otherwise.
> + */
> +static struct sql_stmt *
> +ck_constraint_program_compile(struct ck_constraint_def *ck_constraint_def,
> +			      struct Expr *expr, struct space_def *space_def)
> +{
> +	struct sql *db = sql_get();
> +	struct Parse parser;
> +	sql_parser_create(&parser, db);
> +	struct Vdbe *v = sqlGetVdbe(&parser);
> +	if (v == NULL) {
> +		diag_set(OutOfMemory, sizeof(struct Vdbe), "sqlGetVdbe",
> +			 "vdbe");
> +		return NULL;
> +	}
> +	/* Compile VDBE with default sql parameters. */
> +	struct session *user_session = current_session();
> +	uint32_t sql_flags = user_session->sql_flags;
> +	user_session->sql_flags = default_flags;

This is a time bomb from technical debt point of view. Please pass
the flags  to sql_parser_create instead, which will then pass them
to vdbe.

> +	uint32_t tuple_field_count = mp_decode_array(&new_tuple);
> +	uint32_t field_count =
> +		MIN(tuple_field_count, space->def->field_count);
> +	for (uint32_t i = 0; i < field_count; i++) {
> +		struct sql_bind bind;
> +		if (sql_bind_decode(&bind, i + 1, &new_tuple) != 0 ||
> +		    sql_bind_column(ck_constraint->stmt, &bind, i + 1) != 0) {
> +			diag_set(ClientError, ER_CK_CONSTRAINT_FAILED,
> +				 ck_constraint->def->name,
> +				 ck_constraint->def->expr_str);
> +			return -1;

This looks like a pessimization to me. Depending on the code flow,
some of the tuple fields may not be accessed at all. Is it really
necessary to decode them so agressibvely here? Especially since
you encode *all* space fields.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32