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 5AEF42311B for ; Tue, 2 Jul 2019 20:25:43 -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 4mqXJRARbrSm for ; Tue, 2 Jul 2019 20:25:43 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 1084A20A79 for ; Tue, 2 Jul 2019 20:25:42 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: do not show additional generated ids From: "n.pettik" In-Reply-To: <7af7c576-0555-558b-f905-bcb7d98afb78@tarantool.org> Date: Wed, 3 Jul 2019 03:25:39 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <2E310AE3-BE79-4F97-9443-EBD34D187B96@tarantool.org> References: <14D3EEAF-2501-4BDC-82B6-C0BD3729C57A@tarantool.org> <7af7c576-0555-558b-f905-bcb7d98afb78@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: Imeev Mergen > On 25 Jun 2019, at 17:09, Imeev Mergen wrote: > 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: >>>=20 >>> 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. Ok, now it is clear. Add explanation to the commit message. >>> 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=E2=80=99t 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. Please, split this patch into patches according to your points, and re-send new patch-set. > 4) I added an extra argument to the sqlInsert() function, which > makes it clear that the insertion was done in a trigger. >=20 > 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. Please, support such statements with arguments. > 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.