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 65B7B2E7CF for ; Tue, 25 Jun 2019 10:09:33 -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 tvxTBOxrG9vo for ; Tue, 25 Jun 2019 10:09:33 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 24D0020097 for ; Tue, 25 Jun 2019 10:09:33 -0400 (EDT) From: Imeev Mergen Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: do not show additional generated ids References: <14D3EEAF-2501-4BDC-82B6-C0BD3729C57A@tarantool.org> Message-ID: <7af7c576-0555-558b-f905-bcb7d98afb78@tarantool.org> Date: Tue, 25 Jun 2019 17:09:31 +0300 MIME-Version: 1.0 In-Reply-To: <14D3EEAF-2501-4BDC-82B6-C0BD3729C57A@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: "n.pettik" , tarantool-patches@freelists.org Hi! Thank you for review! My answers below. On 6/7/19 3:38 AM, n.pettik wrote: >> On 29 May 2019, at 20:09, imeevma@tarantool.org wrote: >> >> Currently, if INSERT is executed by a trigger, the new generated >> identifiers will be stored in the VDBE. This is wrong, and this >> patch fixes it. > Why is it so? Currently, we are not able to divide the received IDS into those that belong to the mentioned table and those that do not belong. >> Only identifiers generated during INSERT run by >> the user will be saved. > Firstly, I truly do not understand why this behaviour > is considered to be buggy. I don’t see discussion or > bug description. Now this discussion can be viewed on the dev mailing list. > Secondly, I do not understand what happens in this patch. > Could you please explain these changes and argue why > they are needed? Changes in this patch: 1) At first I removed changes made in patch #2981 since it isn't necessary now. Due to this, all the generation of new identifiers is now performed in BOX. 2) I removed VDBE from TXN. So now sequence_next() does not save the generated IDs in VDBE. 3) I changed the way OP_NextAutoincValue works. Now it is executed when there is an insert in the field with AUTOINCREMENT. If NULL was inserted it receives the last generated ID using the new function sequence_get_value(), and stores it in VDBE. Otherwise it is a no-op. 4) I added an extra argument to the sqlInsert() function, which makes it clear that the insertion was done in a trigger. My point about these changes: 1 - I think it should be done anyway. 2 - It doesn't look like VDBE should be stored in TXN. 3 - I think we can avoid creating the function sequence_get_value(), but then we should think about how to store the last generated ID. 4 - I think this change has a pretty clear purpose.