[tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server

["n.pettik" <korablev@tarantool.org>]

> On 25 Jun 2018, at 18:27, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
>> 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.
> 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.                    
> Global DB hash has been kept as we still ned to get Trigger AST 
> by name on trigger deletion.    

Now it is possible to delete trigger using only its name, since
in on_replace_dd_trigger() we would have space_id from tuple.
So, we came to agreement to delete global hash of triggers.

> 
>> Could you write more informative comment which would include information
>> concerning each action, i.e. why we delete tuple on insertion etc.
> /**
>  * Trigger invoked on rollback in the _trigger space.
> + * Insert old sql_trigger AST stored in trigger->data,
> + * destruct new object in any.

*Since rollback trigger is invoked after insertion to hash and space,
 we have to delete it from those structures and release memory.
 Vice versa, after deletion of trigger we must return it back to hash and space*.

>> Well, personally I would move these checks (i.e. testing that trigger created on VIEW
>> must be <INSTEAD OF>, and trigger created on table can’t be <INSTEAD OF>’) to
>> on_replace_dd_trigger. Otherwise, you may catch severe errors after creation of such
>> ‘Inconsistent’ triggers from Lua.
> Good idea,
> moved to sql_trigger_replace.

It would be great if you provided diff: right here, at the end of letter or
as a next patch version. Don’t forget next time.

>> 
>>> @@ -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.
> Ok, as a part of next refactoring commit.

Please, sent that patch or attach diff.

> 
>> I would give it a name like sql_trigger_hash_replace().
> I can't agree. This function not only updates the cache, but also insert AST into space trigger list.

Ok, as you wish.

> 
>> Are you sure that in a case of error we must delete all triggers from hash?Hm, can't remember, how I've wrote this code.
> Yes, a non-trivial place. 
> Like this:

Add cautionary (or FIXME) comment which would explain that
in case of RENAME’s fail we are totally fucked up: part of triggers
remain with old space’s name. Thus, after creation of new space with
the name of old one, it appears with triggers which haven’t been created.

> 
> 		if (rc != SQLITE_OK) {
> 			sqlite3CommitInternalChanges();
> 			goto abort_due_to_error;
> 		}
> 
> 
>> I would also delete all mentions of SQLITE_OMIT_TRIGGER (within a separate patch),
>> since triggers are always enabled in our SQL.
> Ok, as a part of next refactoring commit.

Please, sent that patch or attach diff.