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 9CA1E2206E for ; Tue, 26 Jun 2018 10:05:24 -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 OcdOnMWzmIyx for ; Tue, 26 Jun 2018 10:05:24 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 52EAE214E9 for ; Tue, 26 Jun 2018 10:05:24 -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: <5704ba48-ea6b-4995-c405-4ea7c1ade87c@tarantool.org> Date: Tue, 26 Jun 2018 17:05:22 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <8d28de228babb13b962c8ffed9399caf45586447.1529490955.git.kshcherbatov@tarantool.org> <4FD641D5-2133-4135-ADC6-72900826F458@tarantool.org> <5704ba48-ea6b-4995-c405-4ea7c1ade87c@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 25 Jun 2018, at 18:27, Kirill Shcherbatov = wrote: >=20 >> 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.=20 > 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 > Global DB hash has been kept as we still ned to get Trigger AST=20 > by name on trigger deletion. =20 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. >=20 >> 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 , 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. > 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=E2=80=99t forget next time. >>=20 >>> @@ -566,34 +559,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger = * pTrigger) >>=20 >> 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. >=20 >> 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. >=20 >> 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.=20 > Like this: Add cautionary (or FIXME) comment which would explain that in case of RENAME=E2=80=99s fail we are totally fucked up: part of = triggers remain with old space=E2=80=99s name. Thus, after creation of new space = with the name of old one, it appears with triggers which haven=E2=80=99t been = created. >=20 > if (rc !=3D SQLITE_OK) { > sqlite3CommitInternalChanges(); > goto abort_due_to_error; > } >=20 >=20 >> 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.