[tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Jun 5 16:31:47 MSK 2018
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.
More information about the Tarantool-patches
mailing list