[tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jun 13 21:53:38 MSK 2018
Thanks for the patch! See 19 minor comments below.
On 09/06/2018 12:32, Kirill Shcherbatov wrote:
> 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 | 124 +++++++++++++++
> src/box/errcode.h | 2 +-
> src/box/lua/schema.lua | 6 +
> src/box/space.c | 5 +
> src/box/space.h | 2 +
> src/box/sql.c | 39 -----
> src/box/sql.h | 50 ++++++
> src/box/sql/build.c | 8 +-
> src/box/sql/fkey.c | 2 -
> src/box/sql/insert.c | 6 +-
> src/box/sql/sqliteInt.h | 5 -
> src/box/sql/tokenize.c | 29 +++-
> src/box/sql/trigger.c | 281 +++++++++++++++++-----------------
> 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 | 260 +++++++++++++++++++++++++++++++
> test/sql/triggers.test.lua | 94 ++++++++++++
> 21 files changed, 737 insertions(+), 281 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..c683a51 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3091,6 +3095,49 @@ lock_before_dd(struct trigger *trigger, void *event)
> latch_lock(&schema_lock);
> }
>
> +static void
> +triggers_task_rollback(struct trigger *trigger, void *event)
1. On_replace_dd_trigger works when a single trigger is created, dropped
or updated. So 'triggers' here is incorrect. Lets name it
on_replace_trigger_rollback. And on_replace_trigger_commit the second
function.
> @@ -3100,6 +3147,83 @@ 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);
> +
> + if (old_tuple != NULL && new_tuple == NULL) {
> + /* DROP trigger. */
> + uint32_t trigger_name_len;
> + const char *trigger_name_src =
> + tuple_field_str_xc(old_tuple, 0, &trigger_name_len);
2. Please, define separate enums for _trigger field numbers in
schema_def.h like it is done for other system spaces. I think, the
previous patch is the perfect place for it.
> + char *trigger_name =
> + (char *)region_alloc_xc(&fiber()->gc,
> + trigger_name_len + 1);
> + memcpy(trigger_name, trigger_name_src, trigger_name_len);
> + trigger_name[trigger_name_len] = 0;
> +
> + struct Trigger *old_trigger;
> + int rc = sql_trigger_replace(sql_get(), trigger_name, NULL,
> + &old_trigger);
3. Incorrect alignment.
> + (void)rc;
> + assert(rc == 0);
> + assert(old_trigger != NULL);
> +
> + on_commit->data = old_trigger;
> + on_rollback->data = old_trigger;
> + } else {
> + /* 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, 2, MP_MAP);
> + struct space_opts opts;
> + struct region *region = &fiber()->gc;
> + space_opts_decode(&opts, space_opts, region);
> + struct Trigger *new_trigger =
> + sql_trigger_compile(sql_get(), opts.sql);
> + if (new_trigger == NULL)
> + diag_raise();
> +
> + const char *trigger_name = sql_trigger_name(new_trigger);
> + if (strncmp(trigger_name_src, trigger_name,
> + trigger_name_len) != 0) {
4. When you worked on CHECKs, I found a bug with strncmp() usage. Please,
fix it here too. Strncmp considers two strings be equal even if their
lengths are different.
> + bool is_insert = (new_tuple != NULL && old_tuple == NULL);
> + assert(!is_insert || old_trigger == NULL);
5. What is the point of is_insert? If it is true, then old_trigger == NULL,
so the line below is the same as just on_commit->data = old_trigger. It is
not?
> +
> + on_commit->data = is_insert ? NULL : old_trigger;
> + on_rollback->data = new_trigger;
> + }
> +
> + txn_on_rollback(txn, on_rollback);
> + txn_on_commit(txn, on_commit);
> }
>
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index dd5ce0a..4996565 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -471,6 +472,11 @@ box.schema.space.drop = function(space_id, space_name, opts)
> -- Delete automatically generated sequence.
> box.schema.sequence.drop(sequence_tuple[2])
> end
> + local triggers = _trigger.index["space_id"]:select({space_id})
> + for i = #triggers, 1, -1 do
> + local v = triggers[i]
> + _trigger:delete{v[1]}
> + end
6. Why not just
for _, t in _trigger.index.space_id:pairs({space_id}) do
_trigger:delete({t.name})
end
?
> diff --git a/src/box/space.c b/src/box/space.c
> index b370b7c..d2aeecf 100644
> --- a/src/box/space.c
> +++ b/src/box/space.c
> @@ -209,6 +209,11 @@ space_delete(struct space *space)
> trigger_destroy(&space->on_replace);
> trigger_destroy(&space->on_stmt_begin);
> space_def_delete(space->def);
> + /*
> + * SQL Triggers should be deleted with on_replace_dd_trigger
> + * initiated in LUA schema:delete.
7. What is schema:delete?
> + */
> + assert(space->sql_triggers == NULL);
> space->vtab->destroy(space);
> }
>
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 3f59fc8..71a74d6 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -42,7 +42,6 @@
>
> #include "say.h"
> #include "sqliteInt.h"
> -#include "tarantoolInt.h"
>
> /* Character classes for tokenizing
> *
> @@ -123,6 +122,7 @@ static const char sql_ascii_class[] = {
> * the #include below.
> */
> #include "keywordhash.h"
> +#include "tarantoolInt.h"
8. This and the previous diff are unnecessary.
> @@ -575,3 +580,23 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
> 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, error);
> + sqlite3DbFree(db, sql_error);
9. Same as in the reviews for previous patches. Why not just
diag_set(ClientError, EQ_SQL, sql_error);
?
> + } else {
> + trigger = parser.pNewTrigger;
10. You can remove
if (! parse_only)
delete(new_trigger)
from parser_destroy if here will nullify pNewTrigger like
sqlite3FinishTrigger. Parser_destroy is called much more
frequently, so lets optimize it to avoid branching when
possible.
> + }
> + sql_parser_destroy(&parser);
> + return trigger;
> +}
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index a9c686f..1b569bc 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -81,104 +82,128 @@ sqlite3BeginTrigger(Parse * pParse,
> - /* Do not create a trigger on a system table */
> - if (sqlite3StrNICmp(pTab->def->name, "sqlite_", 7) == 0) {
> - sqlite3ErrorMsg(pParse,
> - "cannot create trigger on system table");
> + const char *table_name = pTableName->a[0].zName;
> + uint32_t space_id;
> + if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name),
> + &space_id) != 0) {
> + pParse->rc = SQL_TARANTOOL_ERROR;
11. Forgot nErr++.
> + goto trigger_cleanup;
> + }
> + if (space_id == BOX_ID_NIL) {
> + diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> + pParse->rc = SQL_TARANTOOL_ERROR;
12. Same. Please, check in other places yourself.
> goto trigger_cleanup;
> }
>
> @@ -567,34 +555,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
> -/*
> - * Remove a trigger from the hash tables of the sqlite* pointer.
> - */
> -void
> -sqlite3UnlinkAndDeleteTrigger(sqlite3 * db, const char *zName)
> +int
> +sql_trigger_replace(struct sqlite3 *db, const char *name,
> + struct Trigger *trigger, struct Trigger **old_trigger)
> + struct Trigger *src_trigger = trigger != NULL ? trigger : *old_trigger;
> + assert(src_trigger != NULL);
> + struct space *space =
> + space_cache_find(src_trigger->space_id);
13. Fits in one line.
> @@ -633,22 +646,17 @@ sqlite3TriggersExist(Table * pTab, /* The table the contains the triggers */
> )
> {
> int mask = 0;
> - Trigger *pList = 0;
> - Trigger *p;
> + struct Trigger *trigger_list = NULL;
> struct session *user_session = current_session();
> -
> - if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) {
> - pList = pTab->pTrigger;
> - }
> - for (p = pList; p; p = p->pNext) {
> - if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) {
> + if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0)
> + trigger_list = space_trigger_list(pTab->def->id);
> + for (struct Trigger *p = trigger_list; p; p = p->pNext) {
> + if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
> mask |= p->tr_tm;
> - }
> }
> - if (pMask) {
> + if (pMask != 0)
14. Please, apply.
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 1b569bc7b..10294455a 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -650,13 +650,14 @@ sqlite3TriggersExist(Table * pTab, /* The table the contains the triggers */
struct session *user_session = current_session();
if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0)
trigger_list = space_trigger_list(pTab->def->id);
- for (struct Trigger *p = trigger_list; p; p = p->pNext) {
- if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
+ for (struct Trigger *p = trigger_list; p != NULL; p = p->pNext) {
+ if (p->op == op && checkColumnOverlap(p->pColumns,
+ pChanges) != 0)
mask |= p->tr_tm;
}
- if (pMask != 0)
+ if (pMask != NULL)
*pMask = mask;
- return (mask != 0) ? trigger_list : 0;
+ return mask != 0 ? trigger_list : NULL;
}
/*
> }
>
> /*
> @@ -837,7 +845,7 @@ codeRowTrigger(Parse * pParse, /* Current parse context */
> Parse *pSubParse; /* Parse context for sub-vdbe */
> int iEndTrigger = 0; /* Label to jump to if WHEN is false */
>
> - assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger));
> + assert(pTrigger->zName == 0 || pTab->def->id == pTrigger->space_id);
15. != NULL. Please, check in other places yourself.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3fe5875..2d1538e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4799,19 +4757,21 @@ case OP_RenameTable: {
> goto abort_due_to_error;
> }
>
> - pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
> - pTab->pTrigger = pTrig;
> + space = space_by_id(space_id);
16. space is already got in this opcode above.
> + assert(space != NULL);
>
> - /* Rename all trigger created on this table.*/
> - for (; pTrig; pTrig = pTrig->pNext) {
> - sqlite3DbFree(db, pTrig->table);
> - pTrig->table = sqlite3DbStrNDup(db, zNewTableName,
> - sqlite3Strlen30(zNewTableName));
> - pTrig->pTabSchema = pTab->pSchema;
> - rc = tarantoolSqlite3RenameTrigger(pTrig->zName,
> + /* Rename all triggers created on this table. */
> + for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
> + struct Trigger *next_trigger = trigger->pNext;
> + rc = tarantoolSqlite3RenameTrigger(trigger->zName,
> zOldTableName, zNewTableName);
> - if (rc) goto abort_due_to_error;
> + if (rc != SQLITE_OK) {
> + sqlite3ResetAllSchemasOfConnection(db);
> + goto abort_due_to_error;
> + }
> + trigger = next_trigger;
> }
17. Why not this?
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2d1538e24..98b7d95a0 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4761,15 +4761,14 @@ case OP_RenameTable: {
assert(space != NULL);
/* Rename all triggers created on this table. */
- for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
- struct Trigger *next_trigger = trigger->pNext;
+ for (struct Trigger *trigger = space->sql_triggers; trigger != NULL;
+ trigger = trigger->pNext) {
rc = tarantoolSqlite3RenameTrigger(trigger->zName,
zOldTableName, zNewTableName);
if (rc != SQLITE_OK) {
sqlite3ResetAllSchemasOfConnection(db);
goto abort_due_to_error;
}
- trigger = next_trigger;
}
> diff --git a/test/sql/triggers.result b/test/sql/triggers.result
> new file mode 100644
> index 0000000..d214962
> --- /dev/null
> +++ b/test/sql/triggers.result
> @@ -0,0 +1,260 @@
> +env = require('test_run')
> +---
> +...
> +test_run = env.new()
> +---
> +...
> +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
> +---
> +- true
> +...
> +-- get invariant part of the tuple
> + function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} end return r end
18. 'inmutable' does not exist. Maybe 'immutable'?
In Lua you have tuple field access by name and table.insert() to append a value. So
lets use them to make the code more understandable:
for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end
> +---
> +...
> +--
> +-- gh-3273: Move Triggers to server
> +--
> +box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
> +---
> +...
> +box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);")
> +---
> +...
> +box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
> +---
> +...
> +inmutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> + - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> + END; '}
> +...
> +space_id = box.space._space.index["name"]:get('T1').id
> +---
> +...
> +-- checks for LUA tuples
> +tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +inmutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: Duplicate key exists in unique index 'primary' in space '_trigger'
19. Your tests looks failing. Or is it ok? Then why do you need 'immutable_part' here,
if :insert() fails before the call?
More information about the Tarantool-patches
mailing list