From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0C8532E5D2 for ; Tue, 7 May 2019 12:39:51 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id CIH1NR-Q3wTg for ; Tue, 7 May 2019 12:39:50 -0400 (EDT) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B1B322E4E3 for ; Tue, 7 May 2019 12:39:50 -0400 (EDT) Date: Tue, 7 May 2019 19:39:48 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter Message-ID: <20190507163948.GC10365@atlas> References: <5ecf24bcc8c4676d1bcfe0a36c9bb27ebc8281cd.1555420166.git.kshcherbatov@tarantool.org> <4f44a278-a591-4fbf-b2fa-8cb50200d4b0@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4f44a278-a591-4fbf-b2fa-8cb50200d4b0@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: "n.pettik" * Kirill Shcherbatov [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