[tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server
Kirill Shcherbatov
kshcherbatov at tarantool.org
Thu Jun 14 19:12:37 MSK 2018
> 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.
static void
-triggers_task_rollback(struct trigger *trigger, void *event)
+on_replace_trigger_rollback(struct trigger *trigger, void *event)
static void
-triggers_task_commit(struct trigger *trigger, void * /* event */)
+on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
struct trigger *on_rollback =
- txn_alter_trigger_new(triggers_task_rollback, NULL);
+ txn_alter_trigger_new(on_replace_trigger_rollback, NULL);
struct trigger *on_commit =
- txn_alter_trigger_new(triggers_task_commit, NULL);
+ txn_alter_trigger_new(on_replace_trigger_commit, NULL);
> 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.
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3160,7 +3160,8 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
/* DROP trigger. */
uint32_t trigger_name_len;
const char *trigger_name_src =
- tuple_field_str_xc(old_tuple, 0, &trigger_name_len);
+ tuple_field_str_xc(old_tuple, BOX_TRIGGER_FIELD_NAME,
+ &trigger_name_len);
char *trigger_name =
(char *)region_alloc_xc(&fiber()->gc,
trigger_name_len + 1);
@@ -3180,10 +3181,13 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
/* INSERT, REPLACE trigger. */
uint32_t trigger_name_len;
const char *trigger_name_src =
- tuple_field_str_xc(new_tuple, 0, &trigger_name_len);
+ tuple_field_str_xc(new_tuple, BOX_TRIGGER_FIELD_NAME,
+ &trigger_name_len);
const char *space_opts =
- tuple_field_with_type_xc(new_tuple, 2, MP_MAP);
+ tuple_field_with_type_xc(new_tuple,
+ BOX_TRIGGER_FIELD_OPTS,
+ MP_MAP);
struct space_opts opts;
struct region *region = &fiber()->gc;
space_opts_decode(&opts, space_opts, region);
@@ -3200,7 +3204,9 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
"tuple trigger name does not match extracted "
"from SQL");
}
- uint32_t space_id = tuple_field_u32_xc(new_tuple, 1);
+ uint32_t space_id =
+ tuple_field_u32_xc(new_tuple,
+ BOX_TRIGGER_FIELD_SPACE_ID);
> 3. Incorrect alignment.
int rc = sql_trigger_replace(sql_get(), trigger_name, NULL,
- &old_trigger);
+ &old_trigger);
> 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.
- if (strncmp(trigger_name_src, trigger_name,
- trigger_name_len) != 0) {
+ if (strlen(trigger_name) != trigger_name_len ||
+ memcmp(trigger_name_src, trigger_name,
+ trigger_name_len) != 0) {
> 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?
Yes, you are right.
on_commit->data = old_trigger; is enough.
> 6. Why not just
>
> for _, t in _trigger.index.space_id:pairs({space_id}) do
> _trigger:delete({t.name})
> end
Accepted.
- local triggers = _trigger.index["space_id"]:select({space_id})
- for i = #triggers, 1, -1 do
- local v = triggers[i]
- _trigger:delete{v[1]}
+ for _, t in _trigger.index.space_id:pairs({space_id}) do
+ _trigger:delete({t.name})
end
> 7. What is schema:delete?
- * initiated in LUA schema:delete.
+ * initiated in LUA part of deletion process.
> 8. This and the previous diff are unnecessary.
ok.
> 9. Same as in the reviews for previous patches. Why not just
if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK &&
parser.rc != SQL_TARANTOOL_ERROR)
diag_set(ClientError, ER_SQL, sql_error);
else
trigger = parser.pNewTrigger;
> 10. You can remove
> 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.
Not this way, but it is possible:
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e43fbcb..1fe6deb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser)
}
parser->disableLookaside = 0;
sqlite3DbFree(db, parser->zErrMsg);
+ sql_trigger_delete(db, parser->pNewTrigger);
region_destroy(&parser->region);
}
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 16e2111..e06cc0a 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -534,12 +534,6 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
if (pParse->pWithToFree)
sqlite3WithDelete(db, pParse->pWithToFree);
- /*
- * Trigger is exported with pNewTrigger field when
- * parse_only flag is set.
- */
- if (!pParse->parse_only)
- sql_trigger_delete(db, pParse->pNewTrigger);
sqlite3DbFree(db, pParse->pVList);
while (pParse->pZombieTab) {
Table *p = pParse->pZombieTab;
@@ -587,10 +581,12 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql)
char *sql_error;
struct Trigger *trigger = NULL;
if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK &&
- parser.rc != SQL_TARANTOOL_ERROR)
+ parser.rc != SQL_TARANTOOL_ERROR) {
diag_set(ClientError, ER_SQL, sql_error);
- else
+ } else {
trigger = parser.pNewTrigger;
+ parser.pNewTrigger = NULL;
+ }
sql_parser_destroy(&parser);
return trigger;
}
> 11. Forgot nErr++> 12. Same. Please, check in other places yourself.
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -127,6 +127,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
if (!noErr) {
diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
} else {
assert(!db->init.busy);
}
@@ -138,11 +139,13 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name),
&space_id) != 0) {
pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
if (space_id == BOX_ID_NIL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
@@ -168,6 +171,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
(tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER", table_name);
diag_set(ClientError, ER_SQL, error);
pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
if (!space->def->opts.is_view && tr_tm == TK_INSTEAD) {
@@ -177,6 +181,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
table_name);
diag_set(ClientError, ER_SQL, error);
pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
goto trigger_cleanup;
}
> 13. Fits in one line.
- struct space *space =
- space_cache_find(src_trigger->space_id);
+ struct space *space = space_cache_find(src_trigger->space_id);
> 14. Please, apply.
- 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;
> 15. != NULL. Please, check in other places yourself.
assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id);
- assert(pTrigger->zName == 0 || pTab->def->id == pTrigger->space_id);
+ assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id);
> 16. space is already got in this opcode above.
No! That pointer become invalid with sqlite3UnlinkAndDeleteTable, sqlite3InitCallback calls. Now there is a new instance of space.
> 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;
> }
>
I believe that to the moment of assignment next pointer source pointer could be already destructing and this value could be invalid.
+ /* Store pointer as trigger will be destructed. */
> 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
- function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} end return r end
+ function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
> 19. Your tests looks failing. Or is it ok? Then why do you need 'immutable_part' here,
> if :insert() fails before the call?
Looks like rudimentary part of some old test, dropped.
More information about the Tarantool-patches
mailing list