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 1EB6C21773 for ; Mon, 25 Jun 2018 11:27:58 -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 N2VHIx7SUzaO for ; Mon, 25 Jun 2018 11:27:58 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 67B95216FC for ; Mon, 25 Jun 2018 11:27:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 6/6] sql: VDBE tests for trigger existence References: <8bb49455a1fa0aa86ca338deffb25d4d447acb7e.1529490955.git.kshcherbatov@tarantool.org> <0CD65924-D11C-401F-A9D5-4EC06F5310A8@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Mon, 25 Jun 2018 18:27:54 +0300 MIME-Version: 1.0 In-Reply-To: <0CD65924-D11C-401F-A9D5-4EC06F5310A8@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik@corp.mail.ru" > Why? If you didn’t check it, you simply would get ‘duplicate key error’. > So, you did it to display more informative error..? > >> tested on each VDBE >> execution attempt, not on Parser iteration. > > And? After this sentence you should explain changes which you patch provides. Trigger presence in system should be tested on each VDBE execution attempt, not on Parser iteration as system state could be changed between opcode run. With this patch, such checks are the part of VDBE program looking to the _trigger space and raise error message if tuple with specified key already exists. > I would call it vdbe_emit_halt_if_exists(). > Or vdbe_emit_halt_on_duplication()... +vdbe_emit_halt_if_exists(struct Parse *parser, int space_id, int index_id, > In previous patch, you used sqlite3DbMalloc() function without using diag_set(), > since in case of fail it would call sqlite3OomFault(db); > Lets handle all usages of sqlite3DbMalloc() in the same way. Ok. > The same is here. Ok. > Why do you set ON_CONFLICT_ACTION_FAIL? You can just skip this arg with 0. Ok. > Why this func is under SQLITE_OMIT_CTE guard? Moved. > Why have you duplicated this assert? It occurs 2 lines above. Fixed. > Rephrase last sentence - I can’t parse it.. + * The function allocates error and name resources for VDBE itself. > How can index id contain ’string primary key’? > Name of object to test on existence. > Error message to display on VDBE halt. Ok. > Looks like redundant diff. If you wanted to make comments fit into 66 chars, > you would better do it for the whole comment. Refactored while comment. > Why did you provide this diff? AFAIK in this particular case these two calls > are *almost* equivalent. No, you set SQL_TARANTOOL_ERROR here that require diag error been set with my patch.