[tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes! See 12 comments below. I have not reviewed
the whole patch since you did not fix some of the previous
comments, and I have found new bugs.

> Introduced sql_triggers field in space structure.
> Changed parser logic to do not insert built triggers, just only
> to do parsing. All triggers insertions and deletions are operated
> via on_replace_dd_trigger now.
> 
> Resolves #3273.
> ---
>   src/box/alter.cc                      | 140 ++++++++++++++++++
>   src/box/errcode.h                     |   2 +-
>   src/box/space.h                       |   2 +
>   src/box/sql.c                         |  48 ++-----
>   src/box/sql.h                         |  42 ++++++
>   src/box/sql/build.c                   |   8 +-
>   src/box/sql/fkey.c                    |   2 -
>   src/box/sql/insert.c                  |   6 +-
>   src/box/sql/sqliteInt.h               |   7 +-
>   src/box/sql/tokenize.c                |  28 +++-
>   src/box/sql/trigger.c                 | 259 ++++++++++++++++------------------
>   src/box/sql/vdbe.c                    |  76 ++--------
>   src/box/sql/vdbe.h                    |   1 -
>   src/box/sql/vdbeaux.c                 |   9 --
>   test/box/misc.result                  |   1 +
>   test/sql-tap/identifier_case.test.lua |   4 +-
>   test/sql-tap/trigger1.test.lua        |  14 +-
>   test/sql/triggers.result              | 187 ++++++++++++++++++++++++
>   test/sql/triggers.test.lua            |  69 +++++++++
>   19 files changed, 629 insertions(+), 276 deletions(-)
>   create mode 100644 test/sql/triggers.result
>   create mode 100644 test/sql/triggers.test.lua
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index f2bf85d..23cd4a3 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -551,6 +551,8 @@ space_swap_triggers(struct space *new_space, struct space *old_space)
>   	rlist_swap(&new_space->before_replace, &old_space->before_replace);
>   	rlist_swap(&new_space->on_replace, &old_space->on_replace);
>   	rlist_swap(&new_space->on_stmt_begin, &old_space->on_stmt_begin);
> +	/** Copy SQL Triggers pointer. */
> +	new_space->sql_triggers = old_space->sql_triggers;

1. It is not swap still. Swap of A and B means that before swap
A == value1, B = value2. After swap A == value2, B == value1.
Here you have done this:
A == B.

Do you understand why exactly swap is needed here? Please, answer.
Do not skip the question.

2. Space->sql_triggers leaks when I drop a space via Lua.

3. After you fix the leak, try this test to simulate WAL error and you
will see a crash:

	box.cfg{}
	box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER);");
	box.sql.execute("CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER);");
	box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
	box.error.injection.set("ERRINJ_WAL_IO", true)
	box.sql.execute("CREATE INDEX t1a ON t1(a);")
	box.error.injection.set("ERRINJ_WAL_IO", false)
	box.sql.execute("INSERT INTO t1 VALUES (3, 3);")

>   }
>   
>   /**
> @@ -3091,6 +3093,55 @@ lock_before_dd(struct trigger *trigger, void *event)
>   	latch_lock(&schema_lock);
>   }
>   
> +static void
> +triggers_task_rollback(struct trigger *trigger, void *event)
> +{
> +	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
> +	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
> +	struct Trigger *new_trigger;
> +
> +	if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
> +		/* DELETE trigger. */
> +		if (sql_trigger_replace(sql_get(),
> +					sql_trigger_name(old_trigger),
> +					old_trigger, &new_trigger) != 0)
> +			panic("Out of memory on insertion into trigger hash");
> +		assert(new_trigger == NULL);
> +	}  else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) {
> +		/* INSERT trigger. */
> +		int rc =
> +			sql_trigger_replace(sql_get(),
> +					    sql_trigger_name(old_trigger),
> +					    NULL, &new_trigger);

4. Fits in 3 lines.

> +		(void)rc;
> +		assert(rc == 0);
> +		assert(new_trigger == old_trigger);
> +		sql_trigger_delete(sql_get(), new_trigger);
> +	} else {
> +		/* REPLACE trigger. */
> +		if (sql_trigger_replace(sql_get(),
> +					sql_trigger_name(old_trigger),
> +					old_trigger, &new_trigger) != 0)
> +			panic("Out of memory on insertion into trigger hash");
> +		assert(old_trigger != new_trigger);
> +
> +		sql_trigger_delete(sql_get(), new_trigger);
> +	}
> +}
> +
> +

5. Extra new line.

> +static void
> +triggers_task_commit(struct trigger *trigger, void *event)
> +{
> +	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
> +	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
> +
> +	if (stmt->old_tuple != NULL) {

6. Why do you need stmt and stmt->old_tuple here? As I see,
old_trigger != NULL is the same as stmt->old_tuple != NULL.

> +		/* DELETE, REPLACE trigger. */
> +		sql_trigger_delete(sql_get(), old_trigger);
> +	}
> +}
> +
> @@ -3100,6 +3151,95 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
>   {
>   	struct txn *txn = (struct txn *) event;
>   	txn_check_singlestatement_xc(txn, "Space _trigger");
> +	struct txn_stmt *stmt = txn_current_stmt(txn);
> +	struct tuple *old_tuple = stmt->old_tuple;
> +	struct tuple *new_tuple = stmt->new_tuple;
> +
> +	struct trigger *on_rollback =
> +		txn_alter_trigger_new(triggers_task_rollback, NULL);
> +	struct trigger *on_commit =
> +		txn_alter_trigger_new(triggers_task_commit, NULL);
> +
> +	char *trigger_name = NULL;
> +	struct Trigger *new_trigger = NULL;

7. Please, do not create two branches for each case. You can make this
function more compact.
> +	if (new_tuple != NULL) {
> +		/* INSERT, REPLACE trigger. */
> +		uint32_t trigger_name_len;
> +		const char *trigger_name_src =
> +			tuple_field_str_xc(new_tuple, 0, &trigger_name_len);
> +
> +		const char *space_opts =
> +			tuple_field_with_type_xc(new_tuple, 1, MP_MAP);
> +		struct space_opts opts;
> +		struct region *region = &fiber()->gc;
> +		space_opts_decode(&opts, space_opts, region);
> +		new_trigger = sql_trigger_compile(sql_get(), opts.sql);
> +		if (new_trigger == NULL)
> +			diag_raise();
> +
> +		if (strncmp(trigger_name_src, sql_trigger_name(new_trigger),
> +			    trigger_name_len) != 0) {
> +			sql_trigger_delete(sql_get(), new_trigger);
> +			tnt_raise(ClientError, ER_SQL,
> +				  "tuple trigger name does not match extracted "
> +				  "from SQL");
> +		}
> +		trigger_name = sql_trigger_name(new_trigger);
> +	}
> +
> +	if (old_tuple != NULL && new_tuple == NULL) {
> +		/* DELETE trigger. */
> +		struct Trigger *old_trigger;
> +		int rc =
> +			sql_trigger_replace(sql_get(), trigger_name, NULL,
> +					    &old_trigger);
> +		(void)rc;
> +		assert(rc == 0);
> +		assert(old_trigger != NULL);
> +
> +		on_commit->data = old_trigger;
> +		on_rollback->data = old_trigger;
> +	} else if (new_tuple != NULL && old_tuple == NULL) {
> +		/* INSERT trigger. */
> +		struct Trigger *old_trigger;
> +		if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
> +					&old_trigger) != 0) {
> +			sql_trigger_delete(sql_get(), new_trigger);
> +			diag_raise();
> +		}
> +		assert(old_trigger == NULL);
> +
> +		on_commit->data = NULL;
> +		on_rollback->data = new_trigger;
> +	} else {
> +		/* REPLACE trigger. */
> +		struct Trigger *old_trigger;
> +		if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
> +					&old_trigger) != 0) {
> +			sql_trigger_delete(sql_get(), new_trigger);
> +			diag_raise();
> +		}
> +		assert(old_trigger != NULL);
> +
> +		on_commit->data = old_trigger;
> +		on_rollback->data = old_trigger;
> +	}
> +
> +	txn_on_rollback(txn, on_rollback);
> +	txn_on_commit(txn, on_commit);
> +
> +

8. Two extra lines.

>   }
>   
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 35aacc3..885f0a7 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> +/**
> + * Get trigger name by trigger AST object.
> + * @param trigger AST object.
> + * @return trigger name string.
> + */
> +char *
> +sql_trigger_name(struct Trigger *trigger);

9. Const char *.


> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 817a282..d769d14 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -39,6 +39,7 @@
>   #include <stdlib.h>
>   #include <unicode/utf8.h>
>   #include <unicode/uchar.h>
> +#include "box/schema.h"

10. Why?

>   
>   #include "say.h"
>   #include "sqliteInt.h"
> @@ -123,6 +124,7 @@ static const char sql_ascii_class[] = {
>    * the #include below.
>    */
>   #include "keywordhash.h"
> +#include "tarantoolInt.h"

11. Same.

>   
>   #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0)
>   
> @@ -575,3 +582,22 @@ cleanup:
>   	sql_parser_destroy(&parser);
>   	return expression;
>   }
> +
> +struct Trigger *
> +sql_trigger_compile(struct sqlite3 *db, const char *sql)
> +{
> +	struct Parse parser;
> +	sql_parser_create(&parser, db);
> +	parser.parse_only = true;
> +	char *sql_error;
> +	struct Trigger *trigger = NULL;
> +	if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK) {
> +		char *error = tt_static_buf();
> +		snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error);
> +		diag_set(ClientError, ER_SQL, sql_error);

12. Still is not fixed from the previous review. 'Error' is unused.
And why do you need snprintf here?

Please, do another iteration of self-review before sending the patch
again. Remove all unused includes, extra lines, unused variables,
unnecessary calls, fix alignment etc.