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
next prev parent 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