From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B600124C5F for ; Sun, 24 Jun 2018 11:25:06 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id yXOp0hB-P8t5 for ; Sun, 24 Jun 2018 11:25:06 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id C7F2024C1C for ; Sun, 24 Jun 2018 11:25:05 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server From: "n.pettik" In-Reply-To: <8d28de228babb13b962c8ffed9399caf45586447.1529490955.git.kshcherbatov@tarantool.org> Date: Sun, 24 Jun 2018 18:24:58 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <4FD641D5-2133-4135-ADC6-72900826F458@tarantool.org> References: <8d28de228babb13b962c8ffed9399caf45586447.1529490955.git.kshcherbatov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov > On 20 Jun 2018, at 13:46, Kirill Shcherbatov = wrote: >=20 > 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. >=20 This patch in fact introduces a lot of changes. I would devote much more informative commit message to describe them. I had to investigate a lot of things which could be explained in comments/commit message. For instance, why do we still have to hold triggers in a separate hash? To invoke triggers on a particular space you can refer to = space->triggers. Deletion of trigger occurs at VDBE runtime, so you just need to make = sure that _trigger contains record with given name. > /** > @@ -3244,6 +3248,53 @@ lock_before_dd(struct trigger *trigger, void = *event) > } >=20 > /** > + * Trigger invoked on rollback in the _trigger space. > + */ Could you write more informative comment which would include information concerning each action, i.e. why we delete tuple on insertion etc. E.g.: /* INSERT trigger. */ int rc =3D sql_trigger_replace(sql_get(), sql_trigger_name(old_trigger), NULL, &new_trigger); It seems to be misleading since comment says =E2=80=98INSERT trigger=E2=80= =99,=20 but instead you delete trigger from hash. I understand that you mean =E2=80=98on rollback of insertion we must = delete already inserted in hash trigger=E2=80=99, but lets make it more clear. > +static void > +on_replace_trigger_rollback(struct trigger *trigger, void *event) > +{ > + struct txn_stmt *stmt =3D txn_last_stmt((struct txn*) event); > + struct Trigger *old_trigger =3D (struct Trigger *)trigger->data; It is quite terrible: now we have struct Trigger and struct trigger, which quite similar in their names. What do you think about renaming struct Trigger to struct sql_trigger?=20 > + struct Trigger *new_trigger; > + > + if (stmt->old_tuple !=3D NULL && stmt->new_tuple =3D=3D NULL) { > + /* DELETE trigger. */ > + if (sql_trigger_replace(sql_get(), > + sql_trigger_name(old_trigger), > + old_trigger, &new_trigger) !=3D = 0) > + panic("Out of memory on insertion into trigger = hash"); > + assert(new_trigger =3D=3D NULL); > + } else if (stmt->new_tuple !=3D NULL && stmt->old_tuple =3D=3D = NULL) { > + /* INSERT trigger. */ > + int rc =3D sql_trigger_replace(sql_get(), > + = sql_trigger_name(old_trigger), > + NULL, &new_trigger); > + (void)rc; > + assert(rc =3D=3D 0); > + assert(new_trigger =3D=3D 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) !=3D = 0) > + panic("Out of memory on insertion into trigger = hash"); > + assert(old_trigger !=3D new_trigger); > + sql_trigger_delete(sql_get(), new_trigger); > + } > +} > + >=20 > @@ -3252,6 +3303,88 @@ on_replace_dd_trigger(struct trigger * /* = trigger */, void *event) > { > + const char *space_opts =3D > + tuple_field_with_type_xc(new_tuple, > + BOX_TRIGGER_FIELD_OPTS, > + MP_MAP); Why do we still hold string of =E2=80=98CREATE TRIGGER=E2=80=99 = statement as a map? AFAIU trigger can=E2=80=99t feature any functional elements except for = mentioned one. > + struct space_opts opts; > + struct region *region =3D &fiber()->gc; > + space_opts_decode(&opts, space_opts, region); > + struct Trigger *new_trigger =3D > + sql_trigger_compile(sql_get(), opts.sql); > + if (new_trigger =3D=3D NULL) > + diag_raise(); >=20 > diff --git a/src/box/sql.h b/src/box/sql.h > index 90bba1a..7dc3f80 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -95,6 +95,17 @@ struct Select * > sql_view_compile(struct sqlite3 *db, const char *view_stmt); >=20 > /** > + * Perform parsing of provided SQL request and construct trigger AST. > + * @param db SQL context handle. > + * @param sql request to parse. > + * > + * @retval NULL on error > + * @retval not NULL Trigger AST pointer on success. > + */ > +struct Trigger * > +sql_trigger_compile(struct sqlite3 *db, const char *sql); > + > +/** > * Free AST pointed by trigger. > * @param db SQL handle. > * @param trigger AST object. > @@ -103,6 +114,45 @@ void > sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger); >=20 > /** > + * Get server triggers list by space_id. I would mention that this function must take valid space id. > + * @param space_id Space ID. > + * > + * @retval trigger AST list. > + */ > +struct Trigger * > +space_trigger_list(uint32_t space_id); > + >=20 > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index 122c283..e4c936d 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -33,6 +33,7 @@ > * This file contains the implementation for TRIGGERs > */ >=20 > +#include "box/schema.h" > #include "sqliteInt.h" > #include "tarantoolInt.h" > #include "vdbeInt.h" > @@ -81,115 +82,141 @@ sqlite3BeginTrigger(Parse * pParse, /* The = parse context of the CREATE TRIGGER s > ) > { I see that you have slightly refactored this function. I propose to make an effort to completely refactor this function. Moreover, comment to = this function is no longer relevant: >A Trigger >* structure is generated based on the information available and stored >* in pParse->pNewTrigger. > + if (space_id =3D=3D BOX_ID_NIL) { > + diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); > + pParse->rc =3D SQL_TARANTOOL_ERROR; > + pParse->nErr++; You are repeating these two lines 6 times across the code: > + pParse->rc =3D SQL_TARANTOOL_ERROR; > + pParse->nErr++; I suggest to add another one exit label, which would perform exactly these actions.=20 > + goto trigger_cleanup; > + } > + > + /* Do not create a trigger on a system table. */ > + if (sqlite3StrNICmp(table_name, "sqlite_", 7) =3D=3D 0) { I suggest to substitute this obsolete check with Tarantool=E2=80=99s = one: you can use space_is_system() function. > + diag_set(ClientError, ER_SQL, > + "cannot create trigger on system table"); > + pParse->rc =3D SQL_TARANTOOL_ERROR; > + pParse->nErr++; > goto trigger_cleanup; > } >=20 > - /* INSTEAD of triggers are only for views and views only support = INSTEAD > - * of triggers. > + /* > + * INSTEAD of triggers are only for views and > + * views only support INSTEAD of triggers. > */ > - if (pTab->def->opts.is_view && tr_tm !=3D TK_INSTEAD) { > - sqlite3ErrorMsg(pParse, "cannot create %s trigger on = view: %S", > - (tr_tm =3D=3D TK_BEFORE) ? "BEFORE" : = "AFTER", > - pTableName, 0); Well, personally I would move these checks (i.e. testing that trigger = created on VIEW must be , and trigger created on table can=E2=80=99t be = =E2=80=99) to on_replace_dd_trigger. Otherwise, you may catch severe errors after = creation of such =E2=80=98Inconsistent=E2=80=99 triggers from Lua. > @@ -566,34 +559,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * = pTrigger) Lets rename this function and fix comment for it. I suggest to call it = vdbe_code_drop_trigger(), vdbe_emit_drop_trigger() or whatever meaningful name you prefer. > sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); >=20 > sqlite3ChangeCookie(pParse); > - sqlite3VdbeAddOp4(v, OP_DropTrigger, 0, 0, 0, = pTrigger->zName, > - 0); > } > } >=20 > -/* > - * 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) I would give it a name like sql_trigger_hash_replace(). > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 5b5cf83..c1df880 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4710,46 +4710,6 @@ case OP_ParseSchema2: { > break; > } >=20 > /* Opcode: RenameTable P1 * * P4 * > * Synopsis: P1 =3D root, P4 =3D name > * > @@ -4823,18 +4783,22 @@ case OP_RenameTable: { > goto abort_due_to_error; > } >=20 > - pTab =3D sqlite3HashFind(&db->pSchema->tblHash, zNewTableName); > - pTab->pTrigger =3D pTrig; > - > - /* Rename all trigger created on this table.*/ > - for (; pTrig; pTrig =3D pTrig->pNext) { > - sqlite3DbFree(db, pTrig->table); > - pTrig->table =3D sqlite3DbStrNDup(db, zNewTableName, > - = sqlite3Strlen30(zNewTableName)); > - pTrig->pTabSchema =3D pTab->pSchema; > - rc =3D tarantoolSqlite3RenameTrigger(pTrig->zName, > + /* > + * Rebuild 'CREATE TRIGGER' expressions of all triggers > + * created on this table. Sure, this action is not atomic > + * due to lack of transactional DDL, but just do the best > + * effort. > + */ > + for (struct Trigger *trigger =3D triggers; trigger !=3D NULL; ) = { > + /* Store pointer as trigger will be destructed. */ > + struct Trigger *next_trigger =3D trigger->pNext; > + rc =3D tarantoolSqlite3RenameTrigger(trigger->zName, > zOldTableName, = zNewTableName); > - if (rc) goto abort_due_to_error; > + if (rc !=3D SQLITE_OK) { > + sqlite3ResetAllSchemasOfConnection(db); Are you sure that in a case of error we must delete all triggers from = hash? > + goto abort_due_to_error; > + } > + trigger =3D next_trigger; > } > sqlite3DbFree(db, (void*)zOldTableName); > sqlite3DbFree(db, (void*)zSqlStmt); > @@ -4881,18 +4845,6 @@ case OP_DropIndex: { > break; > } >=20 > -/* Opcode: DropTrigger P1 * * P4 * > - * > - * Remove the internal (in-memory) data structures that describe > - * the trigger named P4 in database P1. This is called after a = trigger > - * is dropped from disk (using the Destroy opcode) in order to keep > - * the internal representation of the > - * schema consistent with what is on disk. > - */ > -case OP_DropTrigger: { > - sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z); > - break; > -} > #ifndef SQLITE_OMIT_TRIGGER I would also delete all mentions of SQLITE_OMIT_TRIGGER (within a = separate patch), since triggers are always enabled in our SQL. > +... > diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua > index 63d3063..f559099 100644 > --- a/test/sql/errinj.test.lua > +++ b/test/sql/errinj.test.lua > @@ -44,3 +44,19 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false) >=20 > box.sql.execute('DROP TABLE test') > box.schema.user.revoke('guest', 'read,write,execute', 'universe') > + > +---- > +---- gh-3273: Move SQL TRIGGERs into server. > +---- > +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.error.injection.set("ERRINJ_WAL_IO", true) > +box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 = VALUES (1, 1); END;") > +box.sql.execute("CREATE INDEX t1a ON t1(a);") > +box.error.injection.set("ERRINJ_WAL_IO", false) I would also add the same tests with failed drop of trigger. > diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua > new file mode 100644 > index 0000000..8e3f0c5 > --- /dev/null > +++ b/test/sql/triggers.test.lua > @@ -0,0 +1,74 @@ > +env =3D require('test_run') > +test_run =3D env.new() > + > +-- get invariant part of the tuple Lets start comments with capital letter and end with dots. Also, it doesn=E2=80=99t seem to be obvious what you mean by = =E2=80=98invariant=E2=80=99. > + function immutable_part(data) local r =3D {} for i, l in pairs(data) = do table.insert(r, {l.name, l.opts}) end return r 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; ]]) > +immutable_part(box.space._trigger:select()) > + > +space_id =3D box.space._space.index["name"]:get('T1').id > +assert(box.space.T1.id =3D=3D space_id) How could they mismatch? I guess there are a lot of tests which check such things.