From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: do not show additional generated ids Date: Wed, 3 Jul 2019 03:25:39 +0300 [thread overview] Message-ID: <2E310AE3-BE79-4F97-9443-EBD34D187B96@tarantool.org> (raw) In-Reply-To: <7af7c576-0555-558b-f905-bcb7d98afb78@tarantool.org> > On 25 Jun 2019, at 17:09, Imeev Mergen <imeevma@tarantool.org> 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: >>> >>> 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’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. 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. > > 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.
prev parent reply other threads:[~2019-07-03 0:25 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-29 17:09 [tarantool-patches] " imeevma 2019-06-07 0:38 ` [tarantool-patches] " n.pettik 2019-06-25 14:09 ` Imeev Mergen 2019-07-03 0:25 ` n.pettik [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2E310AE3-BE79-4F97-9443-EBD34D187B96@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: do not show additional generated ids' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox