[tarantool-patches] Re: [PATCH v1 1/1] sql: do not show additional generated ids

[n.pettik]

> On 25 Jun 2019, at 17:09, Imeev Mergen <imeevma at 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 at 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.