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

[Imeev Mergen]

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.
>> 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.