[tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 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.