Tarantool development patches archive
 help / color / mirror / Atom feed
From: Konstantin Osipov <kostja@tarantool.org>
To: tarantool-patches@freelists.org
Cc: korablev@tarantool.org, Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side
Date: Fri, 18 Jan 2019 21:11:21 +0300	[thread overview]
Message-ID: <20190118181121.GC24439@chai> (raw)
In-Reply-To: <2d7c73e83bcee024ffcc095854a31bcd46f6ccab.1547128310.git.kshcherbatov@tarantool.org>

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [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

  parent reply	other threads:[~2019-01-18 18:11 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-10 13:54 [tarantool-patches] [PATCH v1 0/4] sql: Checks " Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 1/4] box: rename space->opts checks to checks_ast Kirill Shcherbatov
2019-01-11 14:05   ` [tarantool-patches] " Konstantin Osipov
2019-01-18 18:00   ` Konstantin Osipov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 2/4] sql: disallow use of TYPEOF in Check Kirill Shcherbatov
2019-01-11 14:06   ` [tarantool-patches] " Konstantin Osipov
2019-01-11 14:07   ` Konstantin Osipov
2019-01-18 18:04   ` Konstantin Osipov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 3/4] box: exported sql_bind structure and API Kirill Shcherbatov
2019-01-10 13:54 ` [tarantool-patches] [PATCH v1 4/4] sql: make sql checks on server side Kirill Shcherbatov
2019-01-11 14:12   ` [tarantool-patches] " Konstantin Osipov
2019-01-11 14:14   ` Konstantin Osipov
2019-01-18 18:11   ` Konstantin Osipov [this message]
2019-01-21 14:47   ` n.pettik

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=20190118181121.GC24439@chai \
    --to=kostja@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side' \
    /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