Tarantool development patches archive
 help / color / mirror / Atom feed
From: Konstantin Osipov <kostja@tarantool.org>
To: tarantool-patches@freelists.org
Cc: "n.pettik" <korablev@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter
Date: Tue, 7 May 2019 19:39:48 +0300	[thread overview]
Message-ID: <20190507163948.GC10365@atlas> (raw)
In-Reply-To: <4f44a278-a591-4fbf-b2fa-8cb50200d4b0@tarantool.org>

* Kirill Shcherbatov <kshcherbatov@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

  reply	other threads:[~2019-05-07 16:39 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-16 13:51 [tarantool-patches] [PATCH v3 0/3] box: run checks on insertions in LUA spaces Kirill Shcherbatov
2019-04-16 13:51 ` [tarantool-patches] [PATCH v3 1/3] schema: add new system space for CHECK constraints Kirill Shcherbatov
2019-04-25 20:38   ` [tarantool-patches] " n.pettik
2019-05-07  9:53     ` Kirill Shcherbatov
2019-05-12 13:45       ` n.pettik
2019-05-12 15:52         ` Kirill Shcherbatov
2019-05-12 23:04           ` n.pettik
2019-05-13  7:11             ` Kirill Shcherbatov
2019-05-13 12:29               ` n.pettik
2019-05-13 13:13                 ` Vladislav Shpilevoy
2019-04-16 13:51 ` [tarantool-patches] [PATCH v3 2/3] box: run check constraint tests on space alter Kirill Shcherbatov
2019-04-25 20:38   ` [tarantool-patches] " n.pettik
2019-05-07  9:53     ` Kirill Shcherbatov
2019-05-07 16:39       ` Konstantin Osipov [this message]
2019-05-07 17:47         ` [tarantool-patches] " Kirill Shcherbatov
2019-05-07 20:28           ` Konstantin Osipov
2019-05-11 12:15           ` n.pettik
2019-05-12 21:12             ` Konstantin Osipov
2019-05-13  7:09               ` Kirill Shcherbatov
2019-05-13  7:49                 ` Konstantin Osipov
2019-05-14 16:49       ` n.pettik
2019-04-16 13:51 ` [tarantool-patches] [PATCH v3 3/3] box: user-friendly interface to manage ck constraints Kirill Shcherbatov
2019-04-25 20:38   ` [tarantool-patches] " n.pettik
2019-05-07  9:53     ` Kirill Shcherbatov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190507163948.GC10365@atlas \
    --to=kostja@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox