[tarantool-patches] Re: [PATCH v1 4/4] sql: make sql checks on server side

[Konstantin Osipov]

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