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 1008A2665F for ; Fri, 18 Jan 2019 13:11:24 -0500 (EST) 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 1guk1TioEgdj for ; Fri, 18 Jan 2019 13:11:23 -0500 (EST) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 C0C9F2660B for ; Fri, 18 Jan 2019 13:11:23 -0500 (EST) Date: Fri, 18 Jan 2019 21:11:21 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side Message-ID: <20190118181121.GC24439@chai> References: <2d7c73e83bcee024ffcc095854a31bcd46f6ccab.1547128310.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2d7c73e83bcee024ffcc095854a31bcd46f6ccab.1547128310.git.kshcherbatov@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: korablev@tarantool.org, Kirill Shcherbatov * Kirill Shcherbatov [19/01/10 23:12]: > diff --git a/src/box/space.c b/src/box/space.c > index 4d174f7ec..b139a0905 100644 > --- a/src/box/space.c > +++ b/src/box/space.c > @@ -29,6 +29,7 @@ > * SUCH DAMAGE. > */ > #include "space.h" > +#include "sql.h" Just STOP. Please read the SOP. You can't add an arbitrary header to another file just because you happen to need it. What the hell is going on here, why do you need to add entire sql layer to space.c? > +/** > + * SQL-specific actions for space. > + */ > +static void > +on_space_before_replace(struct trigger *trigger, void *event) > +{ > + uint32_t space_id = (uint32_t)(uintptr_t)trigger->data; > + struct space *space = space_cache_find(space_id); > + assert(space != NULL); > + struct txn *txn = (struct txn *) event; > + struct txn_stmt *stmt = txn_current_stmt(txn); > + if (stmt == NULL) > + return; > + struct tuple *new_tuple = stmt->new_tuple; > + struct tuple *old_tuple = stmt->old_tuple; > + if (new_tuple != NULL && space->sql_checks != NULL) { > + const char *new_tuple_raw = tuple_data(new_tuple); > + const char *old_tuple_raw = old_tuple != NULL ? > + tuple_data(old_tuple) : NULL; > + if (sql_checks_run(space->sql_checks, > + space->def->opts.checks_ast, space->def, > + old_tuple_raw, new_tuple_raw) != 0) { > + diag_raise(); > + } > + } > +} Why couldn't you define this function wherever you like? Please read the sop carefully about introducing new header file dependencies. > + if (def->opts.checks_ast != NULL) { > + struct sql_checks *checks = > + sql_checks_create(sql_get(), def->opts.checks_ast, def); > + if (unlikely(checks == NULL)) > + goto fail_free_indexes; > + space->sql_checks = checks; > + > + struct trigger *sql_actions_trigger = > + (struct trigger *)malloc(sizeof(struct trigger)); Please don't use plural. Why do you call it a different name? Why not ck_constraint_trigger? Even if it does an action, well, action is part of a check, so why invent a separate name for it? > + * UPDATE operations. > + */ > + struct sql_checks *sql_checks; struct ck_constraint > index 26b84c5db..8961c4fd9 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -35,9 +35,10 @@ > #include "sql/tarantoolInt.h" > #include "sql/vdbeInt.h" > #include "mpstream.h" > +#include "execute.h" You're not writing PHP! Please watch your dependencies carefully. > +/** SQL Checks object. */ > +struct sql_checks { > + /** > + * The first in an array variables representing the > + * new tuple to be inserted. > + */ > + int new_tuple_var; > + /** > + * The first in array of variables representing the state > + * (1 for ON and 0 for OFF) of corresponding Check. This > + * is the UPDATE optimization - disabled checks would be > + * skipped. > + */ > + int checks_state_var; I don't understand what these variables are for even though there are comments :/ > + /** > + * Precompiled reusable VDBE program for SQL Checks > + * raising error on Check constraint failure. VDBE > + * program assumes new_tuple and checks_state actual > + * parameters to be placed in VDBE memory before run. > + */ > + struct sqlite3_stmt *stmt; > +}; -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov