[tarantool-patches] [PATCH v8 0/3] sql: return all generated ids via IPROTO

[tarantool-patches] [PATCH v8 0/3] sql: return all generated ids via IPROTO

[imeevma]

To make implementation of function getGeneratedKeys() of JDBC
driver we should be able to return information of generated in
last INSERT statement keys through IPROTO.

Patch 1 separates fideb_gc() from txn_commit().

Patch 2 implements mentioned functionality.

Patch 3 do some refactoring in VDBE.


Issue: https://github.com/tarantool/tarantool/issues/2618
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids

Mergen Imeev (1):
  sql: return all generated ids via IPROTO

Vladislav Shpilevoy (2):
  box: factor fiber_gc out of txn_commit
  sql: remove psql_txn from Vdbe

 src/box/execute.c        | 28 ++++++++++++++-
 src/box/execute.h        |  1 +
 src/box/lua/net_box.c    | 29 ++++++++++++---
 src/box/sequence.c       |  7 ++++
 src/box/sequence.h       | 13 +++++++
 src/box/sql/vdbe.c       | 42 +++++++++++++++++-----
 src/box/sql/vdbe.h       | 12 ++++++-
 src/box/sql/vdbeInt.h    |  7 ++--
 src/box/sql/vdbeaux.c    | 25 ++++++-------
 src/box/txn.c            | 13 ++++---
 src/box/txn.h            | 28 +++++++++++++++
 src/box/vy_scheduler.c   |  2 +-
 test/sql/iproto.result   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 24 +++++++++++++
 14 files changed, 287 insertions(+), 36 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[imeevma]

Now txn_commit is judge, jury and executioner. It both
commits or rollbacks data, and collects it calling fiber_gc,
which destroys the region.

But SQL wants to use some transactional data after commit. It is
autogenerated identifiers - a list of sequence values generated
for autoincrement columns and explicit sequence:next() calls.

It is possible to store the list on malloced mem inside Vdbe, but
it complicates deallocation. Much more convenient to store all
transactional data on the transaction memory region, so it would
be freed together with fiber_gc.

After this patch applied, Vdbe takes care of txn memory
deallocation in a finalizer routine. Between commit and
finalization transactional data can be serialized wherever.

Needed for #2618
---
Issue: https://github.com/tarantool/tarantool/issues/2618
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids

 src/box/txn.c          | 13 ++++++++-----
 src/box/vy_scheduler.c |  2 +-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 617ceb8..9bcc672 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -250,8 +250,11 @@ txn_commit_stmt(struct txn *txn, struct request *request)
 			goto fail;
 	}
 	--txn->in_sub_stmt;
-	if (txn->is_autocommit && txn->in_sub_stmt == 0)
-		return txn_commit(txn);
+	if (txn->is_autocommit && txn->in_sub_stmt == 0) {
+		int rc = txn_commit(txn);
+		fiber_gc();
+		return rc;
+	}
 	return 0;
 fail:
 	txn_rollback_stmt();
@@ -354,8 +357,6 @@ txn_commit(struct txn *txn)
 		txn_stmt_unref_tuples(stmt);
 
 	TRASH(txn);
-	/** Free volatile txn memory. */
-	fiber_gc();
 	fiber_set_txn(fiber(), NULL);
 	return 0;
 fail:
@@ -463,7 +464,9 @@ box_txn_commit()
 		diag_set(ClientError, ER_COMMIT_IN_SUB_STMT);
 		return -1;
 	}
-	return txn_commit(txn);
+	int rc = txn_commit(txn);
+	fiber_gc();
+	return rc;
 }
 
 int
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index eab3f6c..4a46243 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -886,7 +886,7 @@ vy_deferred_delete_batch_process_f(struct cmsg *cmsg)
 
 	if (txn_commit(txn) != 0)
 		goto fail;
-
+	fiber_gc();
 	return;
 fail:
 	batch->is_failed = true;
-- 
2.7.4

[tarantool-patches] Re: [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[n.pettik]

> On 29 Oct 2018, at 20:33, imeevma@tarantool.org wrote:
> 
> Now txn_commit is judge, jury and executioner. It both
> commits or rollbacks data, and collects it calling fiber_gc,
> which destroys the region.

Nit: both commits and rollbacks.

> 
> But SQL wants to use some transactional data after commit. It is
> autogenerated identifiers - a list of sequence values generated
> for autoincrement columns and explicit sequence:next() calls.
> 
> It is possible to store the list on malloced mem inside Vdbe, but
> it complicates deallocation.

What is the problem with deallocation? AFAIU it is enough to
simply iterate over the list and release each element - not big deal.

If you want to use region, mb it is worth to store separate region
specially for VDBE? We already have it in parser, so what prevents
us for adding the same thing to VDBE? I guess we can store many
things there, not only list of ids. I understand that parser in its turn
has nothing in common (at least it should, except for analyze machinery)
with transaction routines, so separate region is likely to be more
reasonable for parser, but anyway...

[tarantool-patches] Re: [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[Vladislav Shpilevoy]

On 30/10/2018 17:30, n.pettik wrote:
> 
> 
>> On 29 Oct 2018, at 20:33, imeevma@tarantool.org wrote:
>>
>> Now txn_commit is judge, jury and executioner. It both
>> commits or rollbacks data, and collects it calling fiber_gc,
>> which destroys the region.
> 
> Nit: both commits and rollbacks.
> 
>>
>> But SQL wants to use some transactional data after commit. It is
>> autogenerated identifiers - a list of sequence values generated
>> for autoincrement columns and explicit sequence:next() calls.
>>
>> It is possible to store the list on malloced mem inside Vdbe, but
>> it complicates deallocation.
> 
> What is the problem with deallocation? AFAIU it is enough to
> simply iterate over the list and release each element - not big deal.
> 
> If you want to use region, mb it is worth to store separate region
> specially for VDBE? We already have it in parser, so what prevents
> us for adding the same thing to VDBE? I guess we can store many
> things there, not only list of ids. I understand that parser in its turn
> has nothing in common (at least it should, except for analyze machinery)
> with transaction routines, so separate region is likely to be more
> reasonable for parser, but anyway...

Vdbe can yield. In parser we use region only because it
takes and frees slabs without yields.

> 

[tarantool-patches] Re: [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[Vladislav Shpilevoy]

Thanks for the review!

On 30/10/2018 17:30, n.pettik wrote:
> 
> 
>> On 29 Oct 2018, at 20:33, imeevma@tarantool.org wrote:
>>
>> Now txn_commit is judge, jury and executioner. It both
>> commits or rollbacks data, and collects it calling fiber_gc,
>> which destroys the region.
> 
> Nit: both commits and rollbacks.

Fixed.

> 
>>
>> But SQL wants to use some transactional data after commit. It is
>> autogenerated identifiers - a list of sequence values generated
>> for autoincrement columns and explicit sequence:next() calls.
>>
>> It is possible to store the list on malloced mem inside Vdbe, but
>> it complicates deallocation.
> 
> What is the problem with deallocation? AFAIU it is enough to
> simply iterate over the list and release each element - not big deal.
> 
> If you want to use region, mb it is worth to store separate region
> specially for VDBE? We already have it in parser, so what prevents
> us for adding the same thing to VDBE? I guess we can store many
> things there, not only list of ids. I understand that parser in its turn
> has nothing in common (at least it should, except for analyze machinery)
> with transaction routines, so separate region is likely to be more
> reasonable for parser, but anyway...
> 

I've decided to say more details. Parser never yields. This is why we can
waste here any resources, rack and ruin everything, but at the end of
parsing it should be returned back.

Vdbe, on the contrary, yields. So it holds some system resources while
other fibers can not use them. If we added a special region to Vdbe, it
would steal slabs from the thread's slab cache, while other fibers may
want to use it. Hence, when we use one region for all transactional data,
including language specific, allocations are much less fragmented over
different slabs.

Is this explanation decent?

Also, I do not agree, that 'deallocation is just iteration and it is
ok'. It is O(n) iteration and freeing of heap objects. If a one inserted
10k rows with autogenerated ids, it would waste 10k heap fragments,
10k calls of malloc/free - in my opinion it is an abysmal overhead, but
what is more, it can be avoided for free. Instead of 10k free() it boils
down to deallocation of N slabs, where N = slab_size / (10k * 8); 8 - size
of autogenerated it; slab size is at least 64Kb, so N = 64*1024/80000 < 1.
It takes 1 deallocation vs 10k deallocations. So I think this refactoring
is worth.

[tarantool-patches] Re: [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[Vladislav Shpilevoy]

On 30/10/2018 23:03, Vladislav Shpilevoy wrote:
> Thanks for the review!
> 
> On 30/10/2018 17:30, n.pettik wrote:
>>
>>
>>> On 29 Oct 2018, at 20:33, imeevma@tarantool.org wrote:
>>>
>>> Now txn_commit is judge, jury and executioner. It both
>>> commits or rollbacks data, and collects it calling fiber_gc,
>>> which destroys the region.
>>
>> Nit: both commits and rollbacks.
> 
> Fixed.
> 
>>
>>>
>>> But SQL wants to use some transactional data after commit. It is
>>> autogenerated identifiers - a list of sequence values generated
>>> for autoincrement columns and explicit sequence:next() calls.
>>>
>>> It is possible to store the list on malloced mem inside Vdbe, but
>>> it complicates deallocation.
>>
>> What is the problem with deallocation? AFAIU it is enough to
>> simply iterate over the list and release each element - not big deal.
>>
>> If you want to use region, mb it is worth to store separate region
>> specially for VDBE? We already have it in parser, so what prevents
>> us for adding the same thing to VDBE? I guess we can store many
>> things there, not only list of ids. I understand that parser in its turn
>> has nothing in common (at least it should, except for analyze machinery)
>> with transaction routines, so separate region is likely to be more
>> reasonable for parser, but anyway...
>>
> 
> I've decided to say more details. Parser never yields. This is why we can
> waste here any resources, rack and ruin everything, but at the end of
> parsing it should be returned back.
> 
> Vdbe, on the contrary, yields. So it holds some system resources while
> other fibers can not use them. If we added a special region to Vdbe, it
> would steal slabs from the thread's slab cache, while other fibers may
> want to use it. Hence, when we use one region for all transactional data,
> including language specific, allocations are much less fragmented over
> different slabs.
> 
> Is this explanation decent?
> 
> Also, I do not agree, that 'deallocation is just iteration and it is
> ok'. It is O(n) iteration and freeing of heap objects. If a one inserted
> 10k rows with autogenerated ids, it would waste 10k heap fragments,
> 10k calls of malloc/free - in my opinion it is an abysmal overhead, but
> what is more, it can be avoided for free. Instead of 10k free() it boils
> down to deallocation of N slabs, where N = slab_size / (10k * 8); 8 - size
> of autogenerated it; slab size is at least 64Kb, so N = 64*1024/80000 < 1.
> It takes 1 deallocation vs 10k deallocations. So I think this refactoring
> is worth.

Sorry, an error. N = 10Kb * 8 / slab_size ~= 2. Versus 10k still is
significant.

> 
> 

[tarantool-patches] Re: [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[Vladislav Shpilevoy]

On 30/10/2018 23:06, Vladislav Shpilevoy wrote:
> 
> 
> On 30/10/2018 23:03, Vladislav Shpilevoy wrote:
>> Thanks for the review!
>>
>> On 30/10/2018 17:30, n.pettik wrote:
>>>
>>>
>>>> On 29 Oct 2018, at 20:33, imeevma@tarantool.org wrote:
>>>>
>>>> Now txn_commit is judge, jury and executioner. It both
>>>> commits or rollbacks data, and collects it calling fiber_gc,
>>>> which destroys the region.
>>>
>>> Nit: both commits and rollbacks.
>>
>> Fixed.
>>
>>>
>>>>
>>>> But SQL wants to use some transactional data after commit. It is
>>>> autogenerated identifiers - a list of sequence values generated
>>>> for autoincrement columns and explicit sequence:next() calls.
>>>>
>>>> It is possible to store the list on malloced mem inside Vdbe, but
>>>> it complicates deallocation.
>>>
>>> What is the problem with deallocation? AFAIU it is enough to
>>> simply iterate over the list and release each element - not big deal.
>>>
>>> If you want to use region, mb it is worth to store separate region
>>> specially for VDBE? We already have it in parser, so what prevents
>>> us for adding the same thing to VDBE? I guess we can store many
>>> things there, not only list of ids. I understand that parser in its turn
>>> has nothing in common (at least it should, except for analyze machinery)
>>> with transaction routines, so separate region is likely to be more
>>> reasonable for parser, but anyway...
>>>
>>
>> I've decided to say more details. Parser never yields. This is why we can
>> waste here any resources, rack and ruin everything, but at the end of
>> parsing it should be returned back.
>>
>> Vdbe, on the contrary, yields. So it holds some system resources while
>> other fibers can not use them. If we added a special region to Vdbe, it
>> would steal slabs from the thread's slab cache, while other fibers may
>> want to use it. Hence, when we use one region for all transactional data,
>> including language specific, allocations are much less fragmented over
>> different slabs.
>>
>> Is this explanation decent?
>>
>> Also, I do not agree, that 'deallocation is just iteration and it is
>> ok'. It is O(n) iteration and freeing of heap objects. If a one inserted
>> 10k rows with autogenerated ids, it would waste 10k heap fragments,
>> 10k calls of malloc/free - in my opinion it is an abysmal overhead, but
>> what is more, it can be avoided for free. Instead of 10k free() it boils
>> down to deallocation of N slabs, where N = slab_size / (10k * 8); 8 - size
>> of autogenerated it; slab size is at least 64Kb, so N = 64*1024/80000 < 1.
>> It takes 1 deallocation vs 10k deallocations. So I think this refactoring
>> is worth.
> 
> Sorry, an error. N = 10Kb * 8 / slab_size ~= 2. Versus 10k still is
> significant.
> 
>>
>>

10Kb -> 10k, sorry again. I should go sleep ...

> 

[tarantool-patches] Re: [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 3058 bytes --]


>>> But SQL wants to use some transactional data after commit. It is
>>> autogenerated identifiers - a list of sequence values generated
>>> for autoincrement columns and explicit sequence:next() calls.
>>> 
>>> It is possible to store the list on malloced mem inside Vdbe, but
>>> it complicates deallocation.
>> What is the problem with deallocation? AFAIU it is enough to
>> simply iterate over the list and release each element - not big deal.
>> If you want to use region, mb it is worth to store separate region
>> specially for VDBE? We already have it in parser, so what prevents
>> us for adding the same thing to VDBE? I guess we can store many
>> things there, not only list of ids. I understand that parser in its turn
>> has nothing in common (at least it should, except for analyze machinery)
>> with transaction routines, so separate region is likely to be more
>> reasonable for parser, but anyway...
> 
> I've decided to say more details. Parser never yields. This is why we can
> waste here any resources, rack and ruin everything, but at the end of
> parsing it should be returned back.
> 
> Vdbe, on the contrary, yields. So it holds some system resources while
> other fibers can not use them. If we added a special region to Vdbe, it
> would steal slabs from the thread's slab cache, while other fibers may
> want to use it. Hence, when we use one region for all transactional data,
> including language specific, allocations are much less fragmented over
> different slabs.
> 
> Is this explanation decent?

Quite. I thought that used slabs are marked somehow so that different
fibers’ regions can’t rely on the same chunk. Probably, I misunderstood
how internals of our allocation system work. I would better ask you f2f
someday (or read again Konstantin’s article). Anyway, thanks.

> 
> Also, I do not agree, that 'deallocation is just iteration and it is
> ok'. It is O(n) iteration and freeing of heap objects. If a one inserted
> 10k rows with autogenerated ids, it would waste 10k heap fragments,
> 10k calls of malloc/free - in my opinion it is an abysmal overhead, but
> what is more, it can be avoided for free. Instead of 10k free() it boils
> down to deallocation of N slabs, where N = slab_size / (10k * 8); 8 - size
> of autogenerated it; slab size is at least 64Kb, so N = 64*1024/80000 <tel:1024/80000> < 1.
> It takes 1 deallocation vs 10k deallocations. So I think this refactoring
> is worth.

Very impressive calculations, however:

a. I doubt that smb extensively uses queries like
INSERT INTO t VALUES (NULL, ..), *10k repeats*, (NULL, ..)’
*Ok, neither I nor you know which queries users execute (or will execute),
 but anyway your example looks too synthetic.*

b. Nothing prevents us from counting number of NULLs right in parser
and allocate memory as single array (one malloc). In this case it would
be more efficient, I guess, since you don’t need that machinery connected
with linked list. Btw, why didn’t you consider this variant? 


[-- Attachment #2: Type: text/html, Size: 19608 bytes --]

[tarantool-patches] Re: [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[Vladislav Shpilevoy]

On 31/10/2018 02:08, n.pettik wrote:
> 
>>>> But SQL wants to use some transactional data after commit. It is
>>>> autogenerated identifiers - a list of sequence values generated
>>>> for autoincrement columns and explicit sequence:next() calls.
>>>>
>>>> It is possible to store the list on malloced mem inside Vdbe, but
>>>> it complicates deallocation.
>>> What is the problem with deallocation? AFAIU it is enough to
>>> simply iterate over the list and release each element - not big deal.
>>> If you want to use region, mb it is worth to store separate region
>>> specially for VDBE? We already have it in parser, so what prevents
>>> us for adding the same thing to VDBE? I guess we can store many
>>> things there, not only list of ids. I understand that parser in its turn
>>> has nothing in common (at least it should, except for analyze machinery)
>>> with transaction routines, so separate region is likely to be more
>>> reasonable for parser, but anyway...
>>
>> I've decided to say more details. Parser never yields. This is why we can
>> waste here any resources, rack and ruin everything, but at the end of
>> parsing it should be returned back.
>>
>> Vdbe, on the contrary, yields. So it holds some system resources while
>> other fibers can not use them. If we added a special region to Vdbe, it
>> would steal slabs from the thread's slab cache, while other fibers may
>> want to use it. Hence, when we use one region for all transactional data,
>> including language specific, allocations are much less fragmented over
>> different slabs.
>>
>> Is this explanation decent?
> 
> Quite. I thought that used slabs are marked somehow so that different
> fibers’ regions can’t rely on the same chunk. Probably, I misunderstood
> how internals of our allocation system work. I would better ask you f2f
> someday (or read again Konstantin’s article). Anyway, thanks.
> 
>>
>> Also, I do not agree, that 'deallocation is just iteration and it is
>> ok'. It is O(n) iteration and freeing of heap objects. If a one inserted
>> 10k rows with autogenerated ids, it would waste 10k heap fragments,
>> 10k calls of malloc/free - in my opinion it is an abysmal overhead, but
>> what is more, it can be avoided for free. Instead of 10k free() it boils
>> down to deallocation of N slabs, where N = slab_size / (10k * 8); 8 - size
>> of autogenerated it; slab size is at least 64Kb, so N = 64*1024/80000 <tel:1024/80000>< 1.
>> It takes 1 deallocation vs 10k deallocations. So I think this refactoring
>> is worth.
> 
> Very impressive calculations, however:
> 
> a. I doubt that smb extensively uses queries like
> INSERT INTO t VALUES (NULL, ..), *10k repeats*, (NULL, ..)’
> *Ok, neither I nor you know which queries users execute (or will execute),
>   but anyway your example looks too synthetic.*
> 
> b. Nothing prevents us from counting number of NULLs right in parser
> and allocate memory as single array (one malloc). In this case it would
> be more efficient, I guess, since you don’t need that machinery connected
> with linked list. Btw, why didn’t you consider this variant?

INSERT INTO ... SELECT FROM ...;

Here you can not calculate. Also, it is not possible to calculate
autoids from triggers, box Lua functions. So a list is the only
variant.

> 

[tarantool-patches] Re: [PATCH v8 1/3] box: factor fiber_gc out of txn_commit

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 3548 bytes --]



> On 31 Oct 2018, at 12:18, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> On 31/10/2018 02:08, n.pettik wrote:
>>>>> But SQL wants to use some transactional data after commit. It is
>>>>> autogenerated identifiers - a list of sequence values generated
>>>>> for autoincrement columns and explicit sequence:next() calls.
>>>>> 
>>>>> It is possible to store the list on malloced mem inside Vdbe, but
>>>>> it complicates deallocation.
>>>> What is the problem with deallocation? AFAIU it is enough to
>>>> simply iterate over the list and release each element - not big deal.
>>>> If you want to use region, mb it is worth to store separate region
>>>> specially for VDBE? We already have it in parser, so what prevents
>>>> us for adding the same thing to VDBE? I guess we can store many
>>>> things there, not only list of ids. I understand that parser in its turn
>>>> has nothing in common (at least it should, except for analyze machinery)
>>>> with transaction routines, so separate region is likely to be more
>>>> reasonable for parser, but anyway...
>>> 
>>> I've decided to say more details. Parser never yields. This is why we can
>>> waste here any resources, rack and ruin everything, but at the end of
>>> parsing it should be returned back.
>>> 
>>> Vdbe, on the contrary, yields. So it holds some system resources while
>>> other fibers can not use them. If we added a special region to Vdbe, it
>>> would steal slabs from the thread's slab cache, while other fibers may
>>> want to use it. Hence, when we use one region for all transactional data,
>>> including language specific, allocations are much less fragmented over
>>> different slabs.
>>> 
>>> Is this explanation decent?
>> Quite. I thought that used slabs are marked somehow so that different
>> fibers’ regions can’t rely on the same chunk. Probably, I misunderstood
>> how internals of our allocation system work. I would better ask you f2f
>> someday (or read again Konstantin’s article). Anyway, thanks.
>>> 
>>> Also, I do not agree, that 'deallocation is just iteration and it is
>>> ok'. It is O(n) iteration and freeing of heap objects. If a one inserted
>>> 10k rows with autogenerated ids, it would waste 10k heap fragments,
>>> 10k calls of malloc/free - in my opinion it is an abysmal overhead, but
>>> what is more, it can be avoided for free. Instead of 10k free() it boils
>>> down to deallocation of N slabs, where N = slab_size / (10k * 8); 8 - size
>>> of autogenerated it; slab size is at least 64Kb, so N = 64*1024/80000 <tel:1024/80000> <tel:1024/80000 <tel:1024/80000>>< 1.
>>> It takes 1 deallocation vs 10k deallocations. So I think this refactoring
>>> is worth.
>> Very impressive calculations, however:
>> a. I doubt that smb extensively uses queries like
>> INSERT INTO t VALUES (NULL, ..), *10k repeats*, (NULL, ..)’
>> *Ok, neither I nor you know which queries users execute (or will execute),
>>  but anyway your example looks too synthetic.*
>> b. Nothing prevents us from counting number of NULLs right in parser
>> and allocate memory as single array (one malloc). In this case it would
>> be more efficient, I guess, since you don’t need that machinery connected
>> with linked list. Btw, why didn’t you consider this variant?
> 
> INSERT INTO ... SELECT FROM ...;
> 
> Here you can not calculate. Also, it is not possible to calculate
> autoids from triggers, box Lua functions. So a list is the only
> variant.

Ok, now I see. Then patch LGTM.


[-- Attachment #2: Type: text/html, Size: 8905 bytes --]

[tarantool-patches] [PATCH v8 2/3] sql: return all generated ids via IPROTO

[imeevma]

According to documentation some JDBC functions have an ability to
return all ids that were generated in executed INSERT statement.
This patch gives a way to implement such functionality.

Closes #2618
---
Issue: https://github.com/tarantool/tarantool/issues/2618
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids

 src/box/execute.c        | 28 ++++++++++++++-
 src/box/execute.h        |  1 +
 src/box/lua/net_box.c    | 29 ++++++++++++---
 src/box/sequence.c       |  7 ++++
 src/box/sequence.h       | 13 +++++++
 src/box/sql/vdbe.c       | 27 ++++++++++++--
 src/box/sql/vdbe.h       | 12 ++++++-
 src/box/sql/vdbeInt.h    |  5 +++
 src/box/sql/vdbeaux.c    | 21 ++++++-----
 src/box/txn.h            | 28 +++++++++++++++
 test/sql/iproto.result   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 24 +++++++++++++
 12 files changed, 269 insertions(+), 18 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 24459b4..ebf3962 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -42,6 +42,7 @@
 #include "schema.h"
 #include "port.h"
 #include "tuple.h"
+#include "sql/vdbe.h"
 
 const char *sql_type_strs[] = {
 	NULL,
@@ -637,11 +638,26 @@ err:
 	} else {
 		keys = 1;
 		assert(port_tuple->size == 0);
-		if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
+		struct stailq *generated_ids =
+			vdbe_generated_ids((struct Vdbe *)stmt);
+		uint32_t map_size = stailq_empty(generated_ids) ? 1 : 2;
+		if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
 			goto err;
+		uint64_t id_count = 0;
 		int changes = sqlite3_changes(db);
 		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
 			   mp_sizeof_uint(changes);
+		if (!stailq_empty(generated_ids)) {
+			struct id_entry *id_entry;
+			stailq_foreach_entry(id_entry, generated_ids, link) {
+				size += id_entry->id >= 0 ?
+					mp_sizeof_uint(id_entry->id) :
+					mp_sizeof_int(id_entry->id);
+				id_count++;
+			}
+			size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) +
+				mp_sizeof_array(id_count);
+		}
 		char *buf = obuf_alloc(out, size);
 		if (buf == NULL) {
 			diag_set(OutOfMemory, size, "obuf_alloc", "buf");
@@ -649,6 +665,16 @@ err:
 		}
 		buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
 		buf = mp_encode_uint(buf, changes);
+		if (!stailq_empty(generated_ids)) {
+			buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+			buf = mp_encode_array(buf, id_count);
+			struct id_entry *id_entry;
+			stailq_foreach_entry(id_entry, generated_ids, link) {
+				buf = id_entry->id >= 0 ?
+				      mp_encode_uint(buf, id_entry->id) :
+				      mp_encode_int(buf, id_entry->id);
+			}
+		}
 	}
 	iproto_reply_sql(out, &header_svp, response->sync, schema_version,
 			 keys);
diff --git a/src/box/execute.h b/src/box/execute.h
index f21393b..614d3d0 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -42,6 +42,7 @@ extern "C" {
 /** Keys of IPROTO_SQL_INFO map. */
 enum sql_info_key {
 	SQL_INFO_ROW_COUNT = 0,
+	SQL_INFO_GENERATED_IDS = 1,
 	sql_info_key_MAX,
 };
 
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index a928a4c..5a137e0 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -668,16 +668,35 @@ static void
 netbox_decode_sql_info(struct lua_State *L, const char **data)
 {
 	uint32_t map_size = mp_decode_map(data);
-	/* Only SQL_INFO_ROW_COUNT is available. */
-	assert(map_size == 1);
-	(void) map_size;
+	assert(map_size == 1 || map_size == 2);
+	lua_newtable(L);
+	/*
+	 * First element in data is SQL_INFO_ROW_COUNT.
+	 */
 	uint32_t key = mp_decode_uint(data);
 	assert(key == SQL_INFO_ROW_COUNT);
-	(void) key;
 	uint32_t row_count = mp_decode_uint(data);
-	lua_createtable(L, 0, 1);
 	lua_pushinteger(L, row_count);
 	lua_setfield(L, -2, "rowcount");
+	/*
+	 * If data have two elements then second is
+	 * SQL_INFO_GENERATED_IDS.
+	 */
+	if (map_size == 2) {
+		key = mp_decode_uint(data);
+		assert(key == SQL_INFO_GENERATED_IDS);
+		(void)key;
+		uint64_t count = mp_decode_array(data);
+		assert(count > 0);
+		lua_createtable(L, 0, count);
+		for (uint32_t j = 0; j < count; ++j) {
+			int64_t id;
+			mp_read_int64(data, &id);
+			luaL_pushint64(L, id);
+			lua_rawseti(L, -2, j + 1);
+		}
+		lua_setfield(L, -2, "generated_ids");
+	}
 }
 
 static int
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 35b7605..9a87a56 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -38,6 +38,7 @@
 #include <small/mempool.h>
 #include <msgpuck/msgpuck.h>
 
+#include "txn.h"
 #include "diag.h"
 #include "error.h"
 #include "errcode.h"
@@ -194,6 +195,9 @@ sequence_next(struct sequence *seq, int64_t *result)
 					  new_data) == light_sequence_end)
 			return -1;
 		*result = def->start;
+		if (txn_vdbe() != NULL &&
+		    vdbe_add_new_generated_id(txn_vdbe(), *result) != 0)
+			return -1;
 		return 0;
 	}
 	old_data = light_sequence_get(&sequence_data_index, pos);
@@ -228,6 +232,9 @@ done:
 				   new_data, &old_data) == light_sequence_end)
 		unreachable();
 	*result = value;
+	if (txn_vdbe() != NULL &&
+	    vdbe_add_new_generated_id(txn_vdbe(), value) != 0)
+		return -1;
 	return 0;
 overflow:
 	if (!def->cycle) {
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 0f6c8da..7180ead 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -43,6 +43,7 @@ extern "C" {
 #endif /* defined(__cplusplus) */
 
 struct iterator;
+struct Vdbe;
 
 /** Sequence metadata. */
 struct sequence_def {
@@ -140,6 +141,18 @@ int
 access_check_sequence(struct sequence *seq);
 
 /**
+ * Append a new sequence value to Vdbe. List of automatically
+ * generated identifiers is returned as metadata of SQL response.
+ *
+ * @param Vdbe VDBE to save id in.
+ * @param id ID to save in VDBE.
+ * @retval 0 Success.
+ * @retval -1 Error.
+ */
+int
+vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);
+
+/**
  * Create an iterator over sequence data.
  *
  * The iterator creates a snapshot of sequence data and walks
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..2571492 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -578,6 +578,26 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
 	}
 }
 
+struct stailq *
+vdbe_generated_ids(struct Vdbe *vdbe)
+{
+	return &vdbe->generated_ids;
+}
+
+int
+vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
+{
+	assert(vdbe != NULL);
+	struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
+	if (id_entry == NULL) {
+		diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry");
+		return -1;
+	}
+	id_entry->id = id;
+	stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
+	return 0;
+}
+
 /*
  * Execute as much of a VDBE program as we can.
  * This is the core of sqlite3_step().
@@ -2995,8 +3015,9 @@ case OP_TransactionBegin: {
  * If there is no active transaction, raise an error.
  */
 case OP_TransactionCommit: {
-	if (box_txn()) {
-		if (box_txn_commit() != 0) {
+	struct txn *txn = in_txn();
+	if (txn != NULL) {
+		if (txn_commit(txn) != 0) {
 			rc = SQL_TARANTOOL_ERROR;
 			goto abort_due_to_error;
 		}
@@ -3040,7 +3061,7 @@ case OP_TransactionRollback: {
  */
 case OP_TTransaction: {
 	if (!box_txn()) {
-		if (box_txn_begin() != 0) {
+		if (sql_txn_begin(p) != 0) {
 			rc = SQL_TARANTOOL_ERROR;
 			goto abort_due_to_error;
 		}
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index d5da5d5..ed7fb2f 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -179,10 +179,11 @@ Vdbe *sqlite3VdbeCreate(Parse *);
  * Allocate and initialize SQL-specific struct which completes
  * original Tarantool's txn struct using region allocator.
  *
+ * @param v Vdbe with which associate this transaction.
  * @retval NULL on OOM, new psql_txn struct on success.
  **/
 struct sql_txn *
-sql_alloc_txn();
+sql_alloc_txn(struct Vdbe *v);
 
 /**
  * Prepare given VDBE to execution: initialize structs connected
@@ -197,6 +198,15 @@ sql_alloc_txn();
 int
 sql_vdbe_prepare(struct Vdbe *vdbe);
 
+/**
+ * Return pointer to list of generated ids.
+ *
+ * @param vdbe VDBE to get list of generated ids from.
+ * @retval List of generated ids.
+ */
+struct stailq *
+vdbe_generated_ids(struct Vdbe *vdbe);
+
 int sqlite3VdbeAddOp0(Vdbe *, int);
 int sqlite3VdbeAddOp1(Vdbe *, int, int);
 int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index ce97f49..7a7d3de 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -367,6 +367,11 @@ struct Vdbe {
 	struct sql_txn *psql_txn;
 	/** The auto-commit flag. */
 	bool auto_commit;
+	/**
+	 * List of ids generated in current VDBE. It is returned
+	 * as metadata of SQL response.
+	 */
+	struct stailq generated_ids;
 
 	/* When allocating a new Vdbe object, all of the fields below should be
 	 * initialized to zero or NULL
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 6e42d05..ae73f25 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
 		return 0;
 	memset(p, 0, sizeof(Vdbe));
 	p->db = db;
+	stailq_create(&p->generated_ids);
 	if (db->pVdbe) {
 		db->pVdbe->pPrev = p;
 	}
@@ -75,7 +76,7 @@ sqlite3VdbeCreate(Parse * pParse)
 }
 
 struct sql_txn *
-sql_alloc_txn()
+sql_alloc_txn(struct Vdbe *v)
 {
 	struct sql_txn *txn = region_alloc_object(&fiber()->gc,
 						  struct sql_txn);
@@ -84,6 +85,8 @@ sql_alloc_txn()
 			 "struct sql_txn");
 		return NULL;
 	}
+	txn->vdbe = v;
+	v->psql_txn = txn;
 	txn->pSavepoint = NULL;
 	txn->fk_deferred_count = 0;
 	return txn;
@@ -103,11 +106,12 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
 		 * check FK violations, at least now.
 		 */
 		if (txn->psql_txn == NULL) {
-			txn->psql_txn = sql_alloc_txn();
+			txn->psql_txn = sql_alloc_txn(vdbe);
 			if (txn->psql_txn == NULL)
 				return -1;
+		} else {
+			vdbe->psql_txn = txn->psql_txn;
 		}
-		vdbe->psql_txn = txn->psql_txn;
 	} else {
 		vdbe->psql_txn = NULL;
 	}
@@ -2261,12 +2265,11 @@ sql_txn_begin(Vdbe *p)
 	struct txn *ptxn = txn_begin(false);
 	if (ptxn == NULL)
 		return -1;
-	ptxn->psql_txn = sql_alloc_txn();
+	ptxn->psql_txn = sql_alloc_txn(p);
 	if (ptxn->psql_txn == NULL) {
 		box_txn_rollback();
 		return -1;
 	}
-	p->psql_txn = ptxn->psql_txn;
 	return 0;
 }
 
@@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
 					 * key constraints to hold up the transaction. This means a commit
 					 * is required.
 					 */
-					rc = box_txn_commit() ==
-						    0 ? SQLITE_OK :
-						    SQL_TARANTOOL_ERROR;
+					rc = (in_txn() == NULL ||
+					      txn_commit(in_txn()) == 0) ?
+					     SQLITE_OK : SQL_TARANTOOL_ERROR;
 					closeCursorsAndFree(p);
 				}
 				if (rc == SQLITE_BUSY && !p->pDelFrame) {
@@ -2780,6 +2783,8 @@ sqlite3VdbeDelete(Vdbe * p)
 	}
 	p->magic = VDBE_MAGIC_DEAD;
 	p->db = 0;
+	if (in_txn() == NULL)
+		fiber_gc();
 	sqlite3DbFree(db, p);
 }
 
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..87b55da 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -49,6 +49,7 @@ struct engine;
 struct space;
 struct tuple;
 struct xrow_header;
+struct Vdbe;
 
 enum {
 	/**
@@ -117,6 +118,19 @@ struct sql_txn {
 	 * VDBE to the next in the same transaction.
 	 */
 	uint32_t fk_deferred_count;
+	/** Current VDBE. */
+	struct Vdbe *vdbe;
+};
+
+/**
+ * An element of list of autogenerated ids, being returned as SQL
+ * response metadata.
+ */
+struct id_entry {
+	/** A link in a generated id list. */
+	struct stailq_entry link;
+	/** Generated id. */
+	int64_t id;
 };
 
 struct txn {
@@ -337,6 +351,20 @@ txn_last_stmt(struct txn *txn)
 void
 txn_on_stop(struct trigger *trigger, void *event);
 
+/**
+ * Return VDBE that is being currently executed.
+ *
+ * @retval VDBE that is being currently executed.
+ * @retval NULL Either txn or ptxn_sql or vdbe is NULL;
+ */
+static inline struct Vdbe *
+txn_vdbe()
+{
+	struct txn *txn = in_txn();
+	if (txn == NULL || txn->psql_txn == NULL)
+		return NULL;
+	return txn->psql_txn->vdbe;
+}
 
 /**
  * FFI bindings: do not throw exceptions, do not accept extra
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index af474bc..af4fc12 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -580,6 +580,98 @@ cn:close()
 box.sql.execute('drop table test')
 ---
 ...
+-- gh-2618 Return generated columns after INSERT in IPROTO.
+-- Return all ids generated in current INSERT statement.
+box.sql.execute('create table test (id integer primary key autoincrement, a integer)')
+---
+...
+cn = remote.connect(box.cfg.listen)
+---
+...
+cn:execute('insert into test values (1, 1)')
+---
+- rowcount: 1
+...
+cn:execute('insert into test values (null, 2)')
+---
+- generated_ids:
+  - 2
+  rowcount: 1
+...
+cn:execute('update test set a = 11 where id == 1')
+---
+- rowcount: 1
+...
+cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)')
+---
+- generated_ids:
+  - 101
+  - 121
+  rowcount: 4
+...
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)')
+---
+- generated_ids:
+  - 122
+  - 123
+  - 124
+  - 125
+  - 126
+  rowcount: 5
+...
+cn:execute('select * from test')
+---
+- metadata:
+  - name: ID
+  - name: A
+  rows:
+  - [1, 11]
+  - [2, 2]
+  - [100, 1]
+  - [101, 1]
+  - [120, 1]
+  - [121, 1]
+  - [122, 1]
+  - [123, 1]
+  - [124, 1]
+  - [125, 1]
+  - [126, 1]
+...
+s = box.schema.create_space('test2', {engine = engine})
+---
+...
+sq = box.schema.sequence.create('test2')
+---
+...
+pk = s:create_index('pk', {sequence = 'test2'})
+---
+...
+function push_id() s:replace{box.NULL} s:replace{box.NULL} end
+---
+...
+_ = box.space.TEST:on_replace(push_id)
+---
+...
+cn:execute('insert into test values (null, 1)')
+---
+- generated_ids:
+  - 127
+  - 1
+  - 2
+  rowcount: 1
+...
+cn:close()
+---
+...
+box.sql.execute('drop table test')
+---
+...
+s:drop()
+---
+...
+sq:drop()
+---
+...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 220331b..a2caa57 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -201,6 +201,30 @@ future4:wait_result()
 cn:close()
 box.sql.execute('drop table test')
 
+-- gh-2618 Return generated columns after INSERT in IPROTO.
+-- Return all ids generated in current INSERT statement.
+box.sql.execute('create table test (id integer primary key autoincrement, a integer)')
+
+cn = remote.connect(box.cfg.listen)
+cn:execute('insert into test values (1, 1)')
+cn:execute('insert into test values (null, 2)')
+cn:execute('update test set a = 11 where id == 1')
+cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)')
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)')
+cn:execute('select * from test')
+
+s = box.schema.create_space('test2', {engine = engine})
+sq = box.schema.sequence.create('test2')
+pk = s:create_index('pk', {sequence = 'test2'})
+function push_id() s:replace{box.NULL} s:replace{box.NULL} end
+_ = box.space.TEST:on_replace(push_id)
+cn:execute('insert into test values (null, 1)')
+
+cn:close()
+box.sql.execute('drop table test')
+s:drop()
+sq:drop()
+
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 space = nil
 
-- 
2.7.4

[tarantool-patches] Re: [PATCH v8 2/3] sql: return all generated ids via IPROTO

[n.pettik]

> On 29 Oct 2018, at 20:33, imeevma@tarantool.org wrote:
> 
> According to documentation some JDBC functions have an ability to
> return all ids that were generated in executed INSERT statement.
> This patch gives a way to implement such functionality.

I guess you should be more specific in describing such things.
I don’t ask you to dive into details, but explaining main idea of
implementation would definitely help reviewer to do his/her job.

> 
> Closes #2618
> —
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 24459b4..ebf3962 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -42,6 +42,7 @@
> #include "schema.h"
> #include "port.h"
> #include "tuple.h"
> +#include "sql/vdbe.h"
> 
> 
> diff --git a/src/box/execute.h b/src/box/execute.h
> index f21393b..614d3d0 100644
> --- a/src/box/execute.h
> +++ b/src/box/execute.h
> @@ -42,6 +42,7 @@ extern "C" {
> /** Keys of IPROTO_SQL_INFO map. */
> enum sql_info_key {
> 	SQL_INFO_ROW_COUNT = 0,
> +	SQL_INFO_GENERATED_IDS = 1,

I vote for _AUTOINCREMENT_IDS, see comments below.
Anyway, in JDBC it is called GENERATED_KEYS. So, we either
follow JDBC convention or use our naming. In the latter case,
AUTOINCREMENT_IDS sounds more solid.

> 	sql_info_key_MAX,
> };
> 
> /**
> + * Append a new sequence value to Vdbe. List of automatically
> + * generated identifiers is returned as metadata of SQL response.
> + *
> + * @param Vdbe VDBE to save id in.
> + * @param id ID to save in VDBE.
> + * @retval 0 Success.
> + * @retval -1 Error.

It looks terrible. Does this function really need comment at all?

> + */
> +int
> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);
> 
> +
> +int
> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
> +{
> +	assert(vdbe != NULL);
> +	struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
> +	if (id_entry == NULL) {
> +		diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry”);

Out of 80.

> +		return -1;
> +	}
> +	id_entry->id = id;
> +	stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
> +	return 0;
> +}
> +
> /*
>  * Execute as much of a VDBE program as we can.
>  * This is the core of sqlite3_step().
> @@ -2995,8 +3015,9 @@ case OP_TransactionBegin: {
>  * If there is no active transaction, raise an error.
>  */
> case OP_TransactionCommit: {
> -	if (box_txn()) {
> -		if (box_txn_commit() != 0) {
> +	struct txn *txn = in_txn();
> +	if (txn != NULL) {
> +		if (txn_commit(txn) != 0) {

From first sigh it doesn’t look like diff related to patch.
Add comment pointing out that txn_commit() doesn’t
invoke fiber_gc() anymore and VDBE takes care of it.

> /**
>  * Prepare given VDBE to execution: initialize structs connected
> @@ -197,6 +198,15 @@ sql_alloc_txn();
> int
> sql_vdbe_prepare(struct Vdbe *vdbe);
> 
> +/**
> + * Return pointer to list of generated ids.
> + *
> + * @param vdbe VDBE to get list of generated ids from.
> + * @retval List of generated ids.
> + */
> +struct stailq *
> +vdbe_generated_ids(struct Vdbe *vdbe);

As for me, “generated_ids” seems to be too general name…
I would call it “autoincrement_ids" or sort of.

> +
> int sqlite3VdbeAddOp0(Vdbe *, int);
> int sqlite3VdbeAddOp1(Vdbe *, int, int);
> int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index ce97f49..7a7d3de 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -367,6 +367,11 @@ struct Vdbe {
> 	struct sql_txn *psql_txn;
> 	/** The auto-commit flag. */
> 	bool auto_commit;
> +	/**
> +	 * List of ids generated in current VDBE. It is returned
> +	 * as metadata of SQL response.
> +	 */
> +	struct stailq generated_ids;

Again: “autoincrement_ids”. Term “generated_ids” can be confused
with entity ids. For example, during execution of <CREATE TABLE>
statement VDBE operates on space id, index id etc (IMHO).

> 
> 	/* When allocating a new Vdbe object, all of the fields below should be
> 	 * initialized to zero or NULL
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 6e42d05..ae73f25 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
> 		return 0;
> 	memset(p, 0, sizeof(Vdbe));
> 	p->db = db;
> +	stailq_create(&p->generated_ids);
> 	if (db->pVdbe) {
> 		db->pVdbe->pPrev = p;
> 	}
> 
> @@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
> 					 * key constraints to hold up the transaction. This means a commit
> 					 * is required.
> 					 */
> -					rc = box_txn_commit() ==
> -						    0 ? SQLITE_OK :
> -						    SQL_TARANTOOL_ERROR;
> +					rc = (in_txn() == NULL ||
> +					      txn_commit(in_txn()) == 0) ?
> +					     SQLITE_OK : SQL_TARANTOOL_ERROR;
> 					closeCursorsAndFree(p);
> 				}
> 				if (rc == SQLITE_BUSY && !p->pDelFrame) {
> @@ -2780,6 +2783,8 @@ sqlite3VdbeDelete(Vdbe * p)
> 	}
> 	p->magic = VDBE_MAGIC_DEAD;
> 	p->db = 0;
> +	if (in_txn() == NULL)
> +		fiber_gc();

Describe this change (in code, obviously):
“VDBE is responsible for releasing region …"
and so on.

> 	sqlite3DbFree(db, p);
> }
> 
> diff --git a/src/box/txn.h b/src/box/txn.h
> index e74c14d..87b55da 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -49,6 +49,7 @@ struct engine;
> struct space;
> struct tuple;
> struct xrow_header;
> +struct Vdbe;
> 
> enum {
> 	/**
> @@ -117,6 +118,19 @@ struct sql_txn {
> 	 * VDBE to the next in the same transaction.
> 	 */
> 	uint32_t fk_deferred_count;
> +	/** Current VDBE. */
> +	struct Vdbe *vdbe;
> +};
> +
> +/**
> + * An element of list of autogenerated ids, being returned as SQL
> + * response metadata.
> + */
> +struct id_entry {

“id_entry” is too common name, be more specific pls. 

> +	/** A link in a generated id list. */
> +	struct stailq_entry link;
> +	/** Generated id. */
> +	int64_t id;

These comments are too obvious, they only contaminate
source code. 

> };
> 
> /**
>  * FFI bindings: do not throw exceptions, do not accept extra
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index af474bc..af4fc12 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> @@ -580,6 +580,98 @@ cn:close()
> box.sql.execute('drop table test')
> ---
> ...
> +-- gh-2618 Return generated columns after INSERT in IPROTO.
> +-- Return all ids generated in current INSERT statement.

Add pls case where generated ids are less than 0.

[tarantool-patches] Re: [PATCH v8 2/3] sql: return all generated ids via IPROTO

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 37350 bytes --]

Hi! Thank you for review! My answer, diff between patches and new
patch below. Changes were made only in commit named by "sql:
return all generated ids via IPROTO". After changes were made and
diff saved I rebased this branch to current 2.1. After this new
patch description were formed, so it is a bit different from
old patch with applied diff.

On 10/30/18 5:30 PM, n.pettik wrote:

>> On 29 Oct 2018, at 20:33,imeevma@tarantool.org  wrote:
>>
>> According to documentation some JDBC functions have an ability to
>> return all ids that were generated in executed INSERT statement.
>> This patch gives a way to implement such functionality.
> I guess you should be more specific in describing such things.
> I don’t ask you to dive into details, but explaining main idea of
> implementation would definitely help reviewer to do his/her job.
A bit extended this message. Not sure if it is enough.
>> Closes #2618
>> —
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 24459b4..ebf3962 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -42,6 +42,7 @@
>> #include "schema.h"
>> #include "port.h"
>> #include "tuple.h"
>> +#include "sql/vdbe.h"
>>
>>
>> diff --git a/src/box/execute.h b/src/box/execute.h
>> index f21393b..614d3d0 100644
>> --- a/src/box/execute.h
>> +++ b/src/box/execute.h
>> @@ -42,6 +42,7 @@ extern "C" {
>> /** Keys of IPROTO_SQL_INFO map. */
>> enum sql_info_key {
>> 	SQL_INFO_ROW_COUNT = 0,
>> +	SQL_INFO_GENERATED_IDS = 1,
> I vote for _AUTOINCREMENT_IDS, see comments below.
> Anyway, in JDBC it is called GENERATED_KEYS. So, we either
> follow JDBC convention or use our naming. In the latter case,
> AUTOINCREMENT_IDS sounds more solid.
Renamed to SQL_INFO_AUTOINCREMENT_IDS.
>> 	sql_info_key_MAX,
>> };
>>
>> /**
>> + * Append a new sequence value to Vdbe. List of automatically
>> + * generated identifiers is returned as metadata of SQL response.
>> + *
>> + * @param Vdbe VDBE to save id in.
>> + * @param id ID to save in VDBE.
>> + * @retval 0 Success.
>> + * @retval -1 Error.
> It looks terrible. Does this function really need comment at all?
Removed.
>> + */
>> +int
>> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);
>>
>> +
>> +int
>> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
>> +{
>> +	assert(vdbe != NULL);
>> +	struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
>> +	if (id_entry == NULL) {
>> +		diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry”);
> Out of 80.
>
>> +		return -1;
>> +	}
>> +	id_entry->id = id;
>> +	stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
>> +	return 0;
>> +}
>> +
>> /*
>>   * Execute as much of a VDBE program as we can.
>>   * This is the core of sqlite3_step().
>> @@ -2995,8 +3015,9 @@ case OP_TransactionBegin: {
>>   * If there is no active transaction, raise an error.
>>   */
>> case OP_TransactionCommit: {
>> -	if (box_txn()) {
>> -		if (box_txn_commit() != 0) {
>> +	struct txn *txn = in_txn();
>> +	if (txn != NULL) {
>> +		if (txn_commit(txn) != 0) {
>  From first sigh it doesn’t look like diff related to patch.
> Add comment pointing out that txn_commit() doesn’t
> invoke fiber_gc() anymore and VDBE takes care of it.
Extended comment of this opcode.
>> /**
>>   * Prepare given VDBE to execution: initialize structs connected
>> @@ -197,6 +198,15 @@ sql_alloc_txn();
>> int
>> sql_vdbe_prepare(struct Vdbe *vdbe);
>>
>> +/**
>> + * Return pointer to list of generated ids.
>> + *
>> + * @param vdbe VDBE to get list of generated ids from.
>> + * @retval List of generated ids.
>> + */
>> +struct stailq *
>> +vdbe_generated_ids(struct Vdbe *vdbe);
> As for me, “generated_ids” seems to be too general name…
> I would call it “autoincrement_ids" or sort of.
Renamed to autoinc_id_list here.
>> +
>> int sqlite3VdbeAddOp0(Vdbe *, int);
>> int sqlite3VdbeAddOp1(Vdbe *, int, int);
>> int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
>> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
>> index ce97f49..7a7d3de 100644
>> --- a/src/box/sql/vdbeInt.h
>> +++ b/src/box/sql/vdbeInt.h
>> @@ -367,6 +367,11 @@ struct Vdbe {
>> 	struct sql_txn *psql_txn;
>> 	/** The auto-commit flag. */
>> 	bool auto_commit;
>> +	/**
>> +	 * List of ids generated in current VDBE. It is returned
>> +	 * as metadata of SQL response.
>> +	 */
>> +	struct stailq generated_ids;
> Again: “autoincrement_ids”. Term “generated_ids” can be confused
> with entity ids. For example, during execution of <CREATE TABLE>
> statement VDBE operates on space id, index id etc (IMHO).
Renamed to autoinc_id_list here.
>> 	/* When allocating a new Vdbe object, all of the fields below should be
>> 	 * initialized to zero or NULL
>> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
>> index 6e42d05..ae73f25 100644
>> --- a/src/box/sql/vdbeaux.c
>> +++ b/src/box/sql/vdbeaux.c
>> @@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
>> 		return 0;
>> 	memset(p, 0, sizeof(Vdbe));
>> 	p->db = db;
>> +	stailq_create(&p->generated_ids);
>> 	if (db->pVdbe) {
>> 		db->pVdbe->pPrev = p;
>> 	}
>>
>> @@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
>> 					 * key constraints to hold up the transaction. This means a commit
>> 					 * is required.
>> 					 */
>> -					rc = box_txn_commit() ==
>> -						    0 ? SQLITE_OK :
>> -						    SQL_TARANTOOL_ERROR;
>> +					rc = (in_txn() == NULL ||
>> +					      txn_commit(in_txn()) == 0) ?
>> +					     SQLITE_OK : SQL_TARANTOOL_ERROR;
>> 					closeCursorsAndFree(p);
>> 				}
>> 				if (rc == SQLITE_BUSY && !p->pDelFrame) {
>> @@ -2780,6 +2783,8 @@ sqlite3VdbeDelete(Vdbe * p)
>> 	}
>> 	p->magic = VDBE_MAGIC_DEAD;
>> 	p->db = 0;
>> +	if (in_txn() == NULL)
>> +		fiber_gc();
> Describe this change (in code, obviously):
> “VDBE is responsible for releasing region …"
> and so on.
Added comment.
>> 	sqlite3DbFree(db, p);
>> }
>>
>> diff --git a/src/box/txn.h b/src/box/txn.h
>> index e74c14d..87b55da 100644
>> --- a/src/box/txn.h
>> +++ b/src/box/txn.h
>> @@ -49,6 +49,7 @@ struct engine;
>> struct space;
>> struct tuple;
>> struct xrow_header;
>> +struct Vdbe;
>>
>> enum {
>> 	/**
>> @@ -117,6 +118,19 @@ struct sql_txn {
>> 	 * VDBE to the next in the same transaction.
>> 	 */
>> 	uint32_t fk_deferred_count;
>> +	/** Current VDBE. */
>> +	struct Vdbe *vdbe;
>> +};
>> +
>> +/**
>> + * An element of list of autogenerated ids, being returned as SQL
>> + * response metadata.
>> + */
>> +struct id_entry {
> “id_entry” is too common name, be more specific pls.
Renamed to struct autoinc_id_entry. Still, name of variable is
'id_entry'.
>> +	/** A link in a generated id list. */
>> +	struct stailq_entry link;
>> +	/** Generated id. */
>> +	int64_t id;
> These comments are too obvious, they only contaminate
> source code.
Removed these comments.
>> };
>>
>> /**
>>   * FFI bindings: do not throw exceptions, do not accept extra
>> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
>> index af474bc..af4fc12 100644
>> --- a/test/sql/iproto.result
>> +++ b/test/sql/iproto.result
>> @@ -580,6 +580,98 @@ cn:close()
>> box.sql.execute('drop table test')
>> ---
>> ...
>> +-- gh-2618 Return generated columns after INSERT in IPROTO.
>> +-- Return all ids generated in current INSERT statement.
> Add pls case where generated ids are less than 0.

Added.


*Diff between versions:*

diff --git a/src/box/execute.c b/src/box/execute.c
index ebf3962..64c0be6 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -638,24 +638,24 @@ err:
   } else {
     keys = 1;
     assert(port_tuple->size == 0);
-   struct stailq *generated_ids =
-     vdbe_generated_ids((struct Vdbe *)stmt);
-   uint32_t map_size = stailq_empty(generated_ids) ? 1 : 2;
+   struct stailq *autoinc_id_list =
+     vdbe_autoinc_id_list((struct Vdbe *)stmt);
+   uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
     if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
       goto err;
     uint64_t id_count = 0;
     int changes = sqlite3_changes(db);
     int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
          mp_sizeof_uint(changes);
-   if (!stailq_empty(generated_ids)) {
-     struct id_entry *id_entry;
-     stailq_foreach_entry(id_entry, generated_ids, link) {
+   if (!stailq_empty(autoinc_id_list)) {
+     struct autoinc_id_entry *id_entry;
+     stailq_foreach_entry(id_entry, autoinc_id_list, link) {
         size += id_entry->id >= 0 ?
           mp_sizeof_uint(id_entry->id) :
           mp_sizeof_int(id_entry->id);
         id_count++;
       }
-     size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) +
+     size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) +
         mp_sizeof_array(id_count);
     }
     char *buf = obuf_alloc(out, size);
@@ -665,11 +665,11 @@ err:
     }
     buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
     buf = mp_encode_uint(buf, changes);
-   if (!stailq_empty(generated_ids)) {
-     buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+   if (!stailq_empty(autoinc_id_list)) {
+     buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS);
       buf = mp_encode_array(buf, id_count);
-     struct id_entry *id_entry;
-     stailq_foreach_entry(id_entry, generated_ids, link) {
+     struct autoinc_id_entry *id_entry;
+     stailq_foreach_entry(id_entry, autoinc_id_list, link) {
         buf = id_entry->id >= 0 ?
               mp_encode_uint(buf, id_entry->id) :
               mp_encode_int(buf, id_entry->id);
diff --git a/src/box/execute.h b/src/box/execute.h
index 614d3d0..77bfd79 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -42,7 +42,7 @@ extern "C" {
  /** Keys of IPROTO_SQL_INFO map. */
  enum sql_info_key {
   SQL_INFO_ROW_COUNT = 0,
- SQL_INFO_GENERATED_IDS = 1,
+ SQL_INFO_AUTOINCREMENT_IDS = 1,
   sql_info_key_MAX,
  };

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 5a137e0..836ed6e 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -680,11 +680,11 @@ netbox_decode_sql_info(struct lua_State *L, const 
char **data)
   lua_setfield(L, -2, "rowcount");
   /*
    * If data have two elements then second is
-  * SQL_INFO_GENERATED_IDS.
+  * SQL_INFO_AUTOINCREMENT_IDS.
    */
   if (map_size == 2) {
     key = mp_decode_uint(data);
-   assert(key == SQL_INFO_GENERATED_IDS);
+   assert(key == SQL_INFO_AUTOINCREMENT_IDS);
     (void)key;
     uint64_t count = mp_decode_array(data);
     assert(count > 0);
@@ -695,7 +695,7 @@ netbox_decode_sql_info(struct lua_State *L, const 
char **data)
       luaL_pushint64(L, id);
       lua_rawseti(L, -2, j + 1);
     }
-   lua_setfield(L, -2, "generated_ids");
+   lua_setfield(L, -2, "autoincrement_ids");
   }
  }

diff --git a/src/box/sequence.c b/src/box/sequence.c
index 9a87a56..c9828c0 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -196,7 +196,7 @@ sequence_next(struct sequence *seq, int64_t *result)
       return -1;
     *result = def->start;
     if (txn_vdbe() != NULL &&
-       vdbe_add_new_generated_id(txn_vdbe(), *result) != 0)
+       vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)
       return -1;
     return 0;
   }
@@ -233,7 +233,7 @@ done:
     unreachable();
   *result = value;
   if (txn_vdbe() != NULL &&
-     vdbe_add_new_generated_id(txn_vdbe(), value) != 0)
+     vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)
     return -1;
   return 0;
  overflow:
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 7180ead..4419427 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -140,17 +140,8 @@ sequence_next(struct sequence *seq, int64_t *result);
  int
  access_check_sequence(struct sequence *seq);

-/**
- * Append a new sequence value to Vdbe. List of automatically
- * generated identifiers is returned as metadata of SQL response.
- *
- * @param Vdbe VDBE to save id in.
- * @param id ID to save in VDBE.
- * @retval 0 Success.
- * @retval -1 Error.
- */
  int
-vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);

  /**
   * Create an iterator over sequence data.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 1e732a9..dd763e7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -579,22 +579,24 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
  }

  struct stailq *
-vdbe_generated_ids(struct Vdbe *vdbe)
+vdbe_autoinc_id_list(struct Vdbe *vdbe)
  {
- return &vdbe->generated_ids;
+ return &vdbe->autoinc_id_list;
  }

  int
-vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
  {
   assert(vdbe != NULL);
- struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
+ struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc,
+              sizeof(*id_entry));
   if (id_entry == NULL) {
-   diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry");
+   diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc",
+      "id_entry");
     return -1;
   }
   id_entry->id = id;
- stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
+ stailq_add_tail_entry(vdbe_autoinc_id_list(vdbe), id_entry, link);
   return 0;
  }

@@ -3014,6 +3016,9 @@ case OP_TransactionBegin: {
   *
   * Commit Tarantool's transaction.
   * If there is no active transaction, raise an error.
+ * After txn was commited VDBE should take care of region as
+ * txn_commit() doesn't invoke fiber_gc(). Region is needed to
+ * get information of autogenerated ids during sql response dump.
   */
  case OP_TransactionCommit: {
   struct txn *txn = in_txn();
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index ed7fb2f..9546d42 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -205,7 +205,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe);
   * @retval List of generated ids.
   */
  struct stailq *
-vdbe_generated_ids(struct Vdbe *vdbe);
+vdbe_autoinc_id_list(struct Vdbe *vdbe);

  int sqlite3VdbeAddOp0(Vdbe *, int);
  int sqlite3VdbeAddOp1(Vdbe *, int, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 19b35b7..d16a51e 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -369,7 +369,7 @@ struct Vdbe {
    * List of ids generated in current VDBE. It is returned
    * as metadata of SQL response.
    */
- struct stailq generated_ids;
+ struct stailq autoinc_id_list;

   /* When allocating a new Vdbe object, all of the fields below should be
    * initialized to zero or NULL
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 3efe252..78cf2c4 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,7 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
     return 0;
   memset(p, 0, sizeof(Vdbe));
   p->db = db;
- stailq_create(&p->generated_ids);
+ stailq_create(&p->autoinc_id_list);
   if (db->pVdbe) {
     db->pVdbe->pPrev = p;
   }
@@ -2779,6 +2779,10 @@ sqlite3VdbeDelete(Vdbe * p)
   }
   p->magic = VDBE_MAGIC_DEAD;
   p->db = 0;
+ /*
+  * VDBE is responsible for releasing region after txn
+  * was commited.
+  */
   if (in_txn() == NULL)
     fiber_gc();
   sqlite3DbFree(db, p);
diff --git a/src/box/txn.h b/src/box/txn.h
index 87b55da..de5cb0d 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -126,10 +126,8 @@ struct sql_txn {
   * An element of list of autogenerated ids, being returned as SQL
   * response metadata.
   */
-struct id_entry {
- /** A link in a generated id list. */
+struct autoinc_id_entry {
   struct stailq_entry link;
- /** Generated id. */
   int64_t id;
  };

diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index af4fc12..06cb4b9 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -37,6 +37,9 @@ box.sql.execute('select * from test')
  box.schema.user.grant('guest','read,write,execute', 'universe')
  ---
  ...
+box.schema.user.grant('guest', 'create', 'space')
+---
+...
  cn = remote.connect(box.cfg.listen)
  ---
  ...
@@ -594,7 +597,7 @@ cn:execute('insert into test values (1, 1)')
  ...
  cn:execute('insert into test values (null, 2)')
  ---
-- generated_ids:
+- autoincrement_ids:
    - 2
    rowcount: 1
  ...
@@ -604,14 +607,14 @@ cn:execute('update test set a = 11 where id == 1')
  ...
  cn:execute('insert into test values (100, 1), (null, 1), (120, 1), 
(null, 1)')
  ---
-- generated_ids:
+- autoincrement_ids:
    - 101
    - 121
    rowcount: 4
  ...
  cn:execute('insert into test values (null, 1), (null, 1), (null, 1), 
(null, 1), (null, 1)')
  ---
-- generated_ids:
+- autoincrement_ids:
    - 122
    - 123
    - 124
@@ -654,12 +657,27 @@ _ = box.space.TEST:on_replace(push_id)
  ...
  cn:execute('insert into test values (null, 1)')
  ---
-- generated_ids:
+- autoincrement_ids:
    - 127
    - 1
    - 2
    rowcount: 1
  ...
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+---
+...
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+---
+...
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+---
+- autoincrement_ids:
+  - 1
+  - -9
+  - -19
+  - -29
+  rowcount: 4
+...
  cn:close()
  ---
  ...
@@ -672,9 +690,15 @@ s:drop()
  sq:drop()
  ---
  ...
+box.sql.execute('drop table test3')
+---
+...
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  ---
  ...
+box.schema.user.revoke('guest', 'create', 'space')
+---
+...
  space = nil
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index a2caa57..51ac43b 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -10,6 +10,7 @@ space:replace{4, 5, '6'}
  space:replace{7, 8.5, '9'}
  box.sql.execute('select * from test')
  box.schema.user.grant('guest','read,write,execute', 'universe')
+box.schema.user.grant('guest', 'create', 'space')
  cn = remote.connect(box.cfg.listen)
  cn:ping()

@@ -220,12 +221,18 @@ function push_id() s:replace{box.NULL} 
s:replace{box.NULL} end
  _ = box.space.TEST:on_replace(push_id)
  cn:execute('insert into test values (null, 1)')

+box.sql.execute('create table test3 (id int primary key autoincrement)')
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+
  cn:close()
  box.sql.execute('drop table test')
  s:drop()
  sq:drop()
+box.sql.execute('drop table test3')

  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+box.schema.user.revoke('guest', 'create', 'space')
  space = nil

  -- Cleanup xlog

*New version:*

commit 58f70a2cc6bfbed679f1601b09ba1a5d9db05872
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Sep 26 22:07:54 2018 +0300

     sql: return all generated ids via IPROTO

     According to documentation some JDBC functions have an ability to
     return all ids that were generated in executed INSERT statement.
     This patch gives a way to implement such functionality. After
     this patch all ids autogenerated during VDBE execution will be
     saved and returned through IPROTO.

     Closes #2618

diff --git a/src/box/execute.c b/src/box/execute.c
index 24459b4..64c0be6 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -42,6 +42,7 @@
  #include "schema.h"
  #include "port.h"
  #include "tuple.h"
+#include "sql/vdbe.h"

  const char *sql_type_strs[] = {
      NULL,
@@ -637,11 +638,26 @@ err:
      } else {
          keys = 1;
          assert(port_tuple->size == 0);
-        if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
+        struct stailq *autoinc_id_list =
+            vdbe_autoinc_id_list((struct Vdbe *)stmt);
+        uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
+        if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
              goto err;
+        uint64_t id_count = 0;
          int changes = sqlite3_changes(db);
          int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
                 mp_sizeof_uint(changes);
+        if (!stailq_empty(autoinc_id_list)) {
+            struct autoinc_id_entry *id_entry;
+            stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+                size += id_entry->id >= 0 ?
+                    mp_sizeof_uint(id_entry->id) :
+                    mp_sizeof_int(id_entry->id);
+                id_count++;
+            }
+            size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) +
+                mp_sizeof_array(id_count);
+        }
          char *buf = obuf_alloc(out, size);
          if (buf == NULL) {
              diag_set(OutOfMemory, size, "obuf_alloc", "buf");
@@ -649,6 +665,16 @@ err:
          }
          buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
          buf = mp_encode_uint(buf, changes);
+        if (!stailq_empty(autoinc_id_list)) {
+            buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS);
+            buf = mp_encode_array(buf, id_count);
+            struct autoinc_id_entry *id_entry;
+            stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+                buf = id_entry->id >= 0 ?
+                      mp_encode_uint(buf, id_entry->id) :
+                      mp_encode_int(buf, id_entry->id);
+            }
+        }
      }
      iproto_reply_sql(out, &header_svp, response->sync, schema_version,
               keys);
diff --git a/src/box/execute.h b/src/box/execute.h
index f21393b..77bfd79 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -42,6 +42,7 @@ extern "C" {
  /** Keys of IPROTO_SQL_INFO map. */
  enum sql_info_key {
      SQL_INFO_ROW_COUNT = 0,
+    SQL_INFO_AUTOINCREMENT_IDS = 1,
      sql_info_key_MAX,
  };

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index a928a4c..836ed6e 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -668,16 +668,35 @@ static void
  netbox_decode_sql_info(struct lua_State *L, const char **data)
  {
      uint32_t map_size = mp_decode_map(data);
-    /* Only SQL_INFO_ROW_COUNT is available. */
-    assert(map_size == 1);
-    (void) map_size;
+    assert(map_size == 1 || map_size == 2);
+    lua_newtable(L);
+    /*
+     * First element in data is SQL_INFO_ROW_COUNT.
+     */
      uint32_t key = mp_decode_uint(data);
      assert(key == SQL_INFO_ROW_COUNT);
-    (void) key;
      uint32_t row_count = mp_decode_uint(data);
-    lua_createtable(L, 0, 1);
      lua_pushinteger(L, row_count);
      lua_setfield(L, -2, "rowcount");
+    /*
+     * If data have two elements then second is
+     * SQL_INFO_AUTOINCREMENT_IDS.
+     */
+    if (map_size == 2) {
+        key = mp_decode_uint(data);
+        assert(key == SQL_INFO_AUTOINCREMENT_IDS);
+        (void)key;
+        uint64_t count = mp_decode_array(data);
+        assert(count > 0);
+        lua_createtable(L, 0, count);
+        for (uint32_t j = 0; j < count; ++j) {
+            int64_t id;
+            mp_read_int64(data, &id);
+            luaL_pushint64(L, id);
+            lua_rawseti(L, -2, j + 1);
+        }
+        lua_setfield(L, -2, "autoincrement_ids");
+    }
  }

  static int
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 35b7605..c9828c0 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -38,6 +38,7 @@
  #include <small/mempool.h>
  #include <msgpuck/msgpuck.h>

+#include "txn.h"
  #include "diag.h"
  #include "error.h"
  #include "errcode.h"
@@ -194,6 +195,9 @@ sequence_next(struct sequence *seq, int64_t *result)
                        new_data) == light_sequence_end)
              return -1;
          *result = def->start;
+        if (txn_vdbe() != NULL &&
+            vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)
+            return -1;
          return 0;
      }
      old_data = light_sequence_get(&sequence_data_index, pos);
@@ -228,6 +232,9 @@ done:
                     new_data, &old_data) == light_sequence_end)
          unreachable();
      *result = value;
+    if (txn_vdbe() != NULL &&
+        vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)
+        return -1;
      return 0;
  overflow:
      if (!def->cycle) {
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 0f6c8da..4419427 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -43,6 +43,7 @@ extern "C" {
  #endif /* defined(__cplusplus) */

  struct iterator;
+struct Vdbe;

  /** Sequence metadata. */
  struct sequence_def {
@@ -139,6 +140,9 @@ sequence_next(struct sequence *seq, int64_t *result);
  int
  access_check_sequence(struct sequence *seq);

+int
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
+
  /**
   * Create an iterator over sequence data.
   *
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index d6d6b7e..78fd28a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -578,6 +578,28 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
      }
  }

+struct stailq *
+vdbe_autoinc_id_list(struct Vdbe *vdbe)
+{
+    return &vdbe->autoinc_id_list;
+}
+
+int
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
+{
+    assert(vdbe != NULL);
+    struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc,
+                             sizeof(*id_entry));
+    if (id_entry == NULL) {
+        diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc",
+             "id_entry");
+        return -1;
+    }
+    id_entry->id = id;
+    stailq_add_tail_entry(vdbe_autoinc_id_list(vdbe), id_entry, link);
+    return 0;
+}
+
  /*
   * Execute as much of a VDBE program as we can.
   * This is the core of sqlite3_step().
@@ -2997,10 +3019,14 @@ case OP_TransactionBegin: {
   *
   * Commit Tarantool's transaction.
   * If there is no active transaction, raise an error.
+ * After txn was commited VDBE should take care of region as
+ * txn_commit() doesn't invoke fiber_gc(). Region is needed to
+ * get information of autogenerated ids during sql response dump.
   */
  case OP_TransactionCommit: {
-    if (box_txn()) {
-        if (box_txn_commit() != 0) {
+    struct txn *txn = in_txn();
+    if (txn != NULL) {
+        if (txn_commit(txn) != 0) {
              rc = SQL_TARANTOOL_ERROR;
              goto abort_due_to_error;
          }
@@ -3044,7 +3070,7 @@ case OP_TransactionRollback: {
   */
  case OP_TTransaction: {
      if (!box_txn()) {
-        if (box_txn_begin() != 0) {
+        if (sql_txn_begin(p) != 0) {
              rc = SQL_TARANTOOL_ERROR;
              goto abort_due_to_error;
          }
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index d5da5d5..9546d42 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -179,10 +179,11 @@ Vdbe *sqlite3VdbeCreate(Parse *);
   * Allocate and initialize SQL-specific struct which completes
   * original Tarantool's txn struct using region allocator.
   *
+ * @param v Vdbe with which associate this transaction.
   * @retval NULL on OOM, new psql_txn struct on success.
   **/
  struct sql_txn *
-sql_alloc_txn();
+sql_alloc_txn(struct Vdbe *v);

  /**
   * Prepare given VDBE to execution: initialize structs connected
@@ -197,6 +198,15 @@ sql_alloc_txn();
  int
  sql_vdbe_prepare(struct Vdbe *vdbe);

+/**
+ * Return pointer to list of generated ids.
+ *
+ * @param vdbe VDBE to get list of generated ids from.
+ * @retval List of generated ids.
+ */
+struct stailq *
+vdbe_autoinc_id_list(struct Vdbe *vdbe);
+
  int sqlite3VdbeAddOp0(Vdbe *, int);
  int sqlite3VdbeAddOp1(Vdbe *, int, int);
  int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index ce97f49..cfb327a 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -367,6 +367,11 @@ struct Vdbe {
      struct sql_txn *psql_txn;
      /** The auto-commit flag. */
      bool auto_commit;
+    /**
+     * List of ids generated in current VDBE. It is returned
+     * as metadata of SQL response.
+     */
+    struct stailq autoinc_id_list;

      /* When allocating a new Vdbe object, all of the fields below 
should be
       * initialized to zero or NULL
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 6e42d05..1e72a4d 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
          return 0;
      memset(p, 0, sizeof(Vdbe));
      p->db = db;
+    stailq_create(&p->autoinc_id_list);
      if (db->pVdbe) {
          db->pVdbe->pPrev = p;
      }
@@ -75,7 +76,7 @@ sqlite3VdbeCreate(Parse * pParse)
  }

  struct sql_txn *
-sql_alloc_txn()
+sql_alloc_txn(struct Vdbe *v)
  {
      struct sql_txn *txn = region_alloc_object(&fiber()->gc,
                            struct sql_txn);
@@ -84,6 +85,8 @@ sql_alloc_txn()
               "struct sql_txn");
          return NULL;
      }
+    txn->vdbe = v;
+    v->psql_txn = txn;
      txn->pSavepoint = NULL;
      txn->fk_deferred_count = 0;
      return txn;
@@ -103,11 +106,12 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
           * check FK violations, at least now.
           */
          if (txn->psql_txn == NULL) {
-            txn->psql_txn = sql_alloc_txn();
+            txn->psql_txn = sql_alloc_txn(vdbe);
              if (txn->psql_txn == NULL)
                  return -1;
+        } else {
+            vdbe->psql_txn = txn->psql_txn;
          }
-        vdbe->psql_txn = txn->psql_txn;
      } else {
          vdbe->psql_txn = NULL;
      }
@@ -2261,12 +2265,11 @@ sql_txn_begin(Vdbe *p)
      struct txn *ptxn = txn_begin(false);
      if (ptxn == NULL)
          return -1;
-    ptxn->psql_txn = sql_alloc_txn();
+    ptxn->psql_txn = sql_alloc_txn(p);
      if (ptxn->psql_txn == NULL) {
          box_txn_rollback();
          return -1;
      }
-    p->psql_txn = ptxn->psql_txn;
      return 0;
  }

@@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
                       * key constraints to hold up the transaction. 
This means a commit
                       * is required.
                       */
-                    rc = box_txn_commit() ==
-                            0 ? SQLITE_OK :
-                            SQL_TARANTOOL_ERROR;
+                    rc = (in_txn() == NULL ||
+                          txn_commit(in_txn()) == 0) ?
+                         SQLITE_OK : SQL_TARANTOOL_ERROR;
                      closeCursorsAndFree(p);
                  }
                  if (rc == SQLITE_BUSY && !p->pDelFrame) {
@@ -2780,6 +2783,12 @@ sqlite3VdbeDelete(Vdbe * p)
      }
      p->magic = VDBE_MAGIC_DEAD;
      p->db = 0;
+    /*
+     * VDBE is responsible for releasing region after txn
+     * was commited.
+     */
+    if (in_txn() == NULL)
+        fiber_gc();
      sqlite3DbFree(db, p);
  }

diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..de5cb0d 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -49,6 +49,7 @@ struct engine;
  struct space;
  struct tuple;
  struct xrow_header;
+struct Vdbe;

  enum {
      /**
@@ -117,6 +118,17 @@ struct sql_txn {
       * VDBE to the next in the same transaction.
       */
      uint32_t fk_deferred_count;
+    /** Current VDBE. */
+    struct Vdbe *vdbe;
+};
+
+/**
+ * An element of list of autogenerated ids, being returned as SQL
+ * response metadata.
+ */
+struct autoinc_id_entry {
+    struct stailq_entry link;
+    int64_t id;
  };

  struct txn {
@@ -337,6 +349,20 @@ txn_last_stmt(struct txn *txn)
  void
  txn_on_stop(struct trigger *trigger, void *event);

+/**
+ * Return VDBE that is being currently executed.
+ *
+ * @retval VDBE that is being currently executed.
+ * @retval NULL Either txn or ptxn_sql or vdbe is NULL;
+ */
+static inline struct Vdbe *
+txn_vdbe()
+{
+    struct txn *txn = in_txn();
+    if (txn == NULL || txn->psql_txn == NULL)
+        return NULL;
+    return txn->psql_txn->vdbe;
+}

  /**
   * FFI bindings: do not throw exceptions, do not accept extra
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index ee34f6f..06cb4b9 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -583,6 +583,116 @@ cn:close()
  box.sql.execute('drop table test')
  ---
  ...
+-- gh-2618 Return generated columns after INSERT in IPROTO.
+-- Return all ids generated in current INSERT statement.
+box.sql.execute('create table test (id integer primary key 
autoincrement, a integer)')
+---
+...
+cn = remote.connect(box.cfg.listen)
+---
+...
+cn:execute('insert into test values (1, 1)')
+---
+- rowcount: 1
+...
+cn:execute('insert into test values (null, 2)')
+---
+- autoincrement_ids:
+  - 2
+  rowcount: 1
+...
+cn:execute('update test set a = 11 where id == 1')
+---
+- rowcount: 1
+...
+cn:execute('insert into test values (100, 1), (null, 1), (120, 1), 
(null, 1)')
+---
+- autoincrement_ids:
+  - 101
+  - 121
+  rowcount: 4
+...
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1), 
(null, 1), (null, 1)')
+---
+- autoincrement_ids:
+  - 122
+  - 123
+  - 124
+  - 125
+  - 126
+  rowcount: 5
+...
+cn:execute('select * from test')
+---
+- metadata:
+  - name: ID
+  - name: A
+  rows:
+  - [1, 11]
+  - [2, 2]
+  - [100, 1]
+  - [101, 1]
+  - [120, 1]
+  - [121, 1]
+  - [122, 1]
+  - [123, 1]
+  - [124, 1]
+  - [125, 1]
+  - [126, 1]
+...
+s = box.schema.create_space('test2', {engine = engine})
+---
+...
+sq = box.schema.sequence.create('test2')
+---
+...
+pk = s:create_index('pk', {sequence = 'test2'})
+---
+...
+function push_id() s:replace{box.NULL} s:replace{box.NULL} end
+---
+...
+_ = box.space.TEST:on_replace(push_id)
+---
+...
+cn:execute('insert into test values (null, 1)')
+---
+- autoincrement_ids:
+  - 127
+  - 1
+  - 2
+  rowcount: 1
+...
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+---
+...
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+---
+...
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+---
+- autoincrement_ids:
+  - 1
+  - -9
+  - -19
+  - -29
+  rowcount: 4
+...
+cn:close()
+---
+...
+box.sql.execute('drop table test')
+---
+...
+s:drop()
+---
+...
+sq:drop()
+---
+...
+box.sql.execute('drop table test3')
+---
+...
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index b065590..51ac43b 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -202,6 +202,35 @@ future4:wait_result()
  cn:close()
  box.sql.execute('drop table test')

+-- gh-2618 Return generated columns after INSERT in IPROTO.
+-- Return all ids generated in current INSERT statement.
+box.sql.execute('create table test (id integer primary key 
autoincrement, a integer)')
+
+cn = remote.connect(box.cfg.listen)
+cn:execute('insert into test values (1, 1)')
+cn:execute('insert into test values (null, 2)')
+cn:execute('update test set a = 11 where id == 1')
+cn:execute('insert into test values (100, 1), (null, 1), (120, 1), 
(null, 1)')
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1), 
(null, 1), (null, 1)')
+cn:execute('select * from test')
+
+s = box.schema.create_space('test2', {engine = engine})
+sq = box.schema.sequence.create('test2')
+pk = s:create_index('pk', {sequence = 'test2'})
+function push_id() s:replace{box.NULL} s:replace{box.NULL} end
+_ = box.space.TEST:on_replace(push_id)
+cn:execute('insert into test values (null, 1)')
+
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+
+cn:close()
+box.sql.execute('drop table test')
+s:drop()
+sq:drop()
+box.sql.execute('drop table test3')
+
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  box.schema.user.revoke('guest', 'create', 'space')
  space = nil


[-- Attachment #2: Type: text/html, Size: 50444 bytes --]

[tarantool-patches] Re: [PATCH v8 2/3] sql: return all generated ids via IPROTO

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]


> Hi! Thank you for review! My answer, diff between patches and new
> patch below. Changes were made only in commit named by "sql:
> return all generated ids via IPROTO". After changes were made and
> diff saved I rebased this branch to current 2.1. After this new
> patch description were formed, so it is a bit different from
> old patch with applied diff.

Now LGTM (as well as whole patch-set).

[-- Attachment #2: Type: text/html, Size: 5484 bytes --]

[tarantool-patches] [PATCH v8 3/3] sql: remove psql_txn from Vdbe

[imeevma]

It makes no sense to store it here, and it has never
did. SQL transaction specific things shall be taken
from global txn object, as any transaction specific
things.
---
Issue: https://github.com/tarantool/tarantool/issues/2618
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids

 src/box/sql/vdbe.c    | 15 +++++++++------
 src/box/sql/vdbeInt.h |  2 --
 src/box/sql/vdbeaux.c | 10 +++-------
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2571492..1e732a9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2882,7 +2882,8 @@ case OP_Savepoint: {
 	Savepoint *pNew;
 	Savepoint *pSavepoint;
 	Savepoint *pTmp;
-	struct sql_txn *psql_txn = p->psql_txn;
+	struct txn *txn = in_txn();
+	struct sql_txn *psql_txn = txn != NULL ? txn->psql_txn : NULL;
 
 	if (psql_txn == NULL) {
 		assert(!box_txn());
@@ -2960,7 +2961,7 @@ case OP_Savepoint: {
 				assert(pSavepoint == psql_txn->pSavepoint);
 				psql_txn->pSavepoint = pSavepoint->pNext;
 			} else {
-				p->psql_txn->fk_deferred_count =
+				psql_txn->fk_deferred_count =
 					pSavepoint->tnt_savepoint->fk_deferred_count;
 			}
 		}
@@ -4802,8 +4803,9 @@ case OP_Param: {           /* out2 */
 case OP_FkCounter: {
 	if ((user_session->sql_flags & SQLITE_DeferFKs || pOp->p1 != 0) &&
 	    !p->auto_commit) {
-		assert(p->psql_txn != NULL);
-		p->psql_txn->fk_deferred_count += pOp->p2;
+		struct txn *txn = in_txn();
+		assert(txn != NULL && txn->psql_txn != NULL);
+		txn->psql_txn->fk_deferred_count += pOp->p2;
 	} else {
 		p->nFkConstraint += pOp->p2;
 	}
@@ -4825,8 +4827,9 @@ case OP_FkCounter: {
 case OP_FkIfZero: {         /* jump */
 	if ((user_session->sql_flags & SQLITE_DeferFKs || pOp->p1) &&
 	    !p->auto_commit) {
-		assert(p->psql_txn != NULL);
-		if (p->psql_txn->fk_deferred_count == 0)
+		struct txn *txn = in_txn();
+		assert(txn != NULL && txn->psql_txn != NULL);
+		if (txn->psql_txn->fk_deferred_count == 0)
 			goto jump_to_p2;
 	} else {
 		if (p->nFkConstraint == 0)
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 7a7d3de..19b35b7 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -363,8 +363,6 @@ struct Vdbe {
 	 * ignoreRaised variable helps to track such situations
 	 */
 	u8 ignoreRaised;	/* Flag for ON CONFLICT IGNORE for triggers */
-	/** Data related to current transaction. */
-	struct sql_txn *psql_txn;
 	/** The auto-commit flag. */
 	bool auto_commit;
 	/**
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index ae73f25..3efe252 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -86,7 +86,6 @@ sql_alloc_txn(struct Vdbe *v)
 		return NULL;
 	}
 	txn->vdbe = v;
-	v->psql_txn = txn;
 	txn->pSavepoint = NULL;
 	txn->fk_deferred_count = 0;
 	return txn;
@@ -109,11 +108,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
 			txn->psql_txn = sql_alloc_txn(vdbe);
 			if (txn->psql_txn == NULL)
 				return -1;
-		} else {
-			vdbe->psql_txn = txn->psql_txn;
 		}
-	} else {
-		vdbe->psql_txn = NULL;
 	}
 	return 0;
 }
@@ -2244,8 +2239,9 @@ sqlite3VdbeCloseStatement(Vdbe * p, int eOp)
 int
 sqlite3VdbeCheckFk(Vdbe * p, int deferred)
 {
-	if ((deferred && p->psql_txn != NULL &&
-	     p->psql_txn->fk_deferred_count > 0) ||
+	struct txn *txn = in_txn();
+	if ((deferred && txn != NULL && txn->psql_txn != NULL &&
+	     txn->psql_txn->fk_deferred_count > 0) ||
 	    (!deferred && p->nFkConstraint > 0)) {
 		p->rc = SQLITE_CONSTRAINT_FOREIGNKEY;
 		p->errorAction = ON_CONFLICT_ACTION_ABORT;
-- 
2.7.4

[tarantool-patches] Re: [PATCH v8 3/3] sql: remove psql_txn from Vdbe

[n.pettik]

> It makes no sense to store it here, and it has never
> did.

Nit: this sentence is grammatically incorrect, I guess.

> SQL transaction specific things shall be taken
> from global txn object, as any transaction specific
> things.
> ---
> Issue: https://github.com/tarantool/tarantool/issues/2618
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids
> 

You don’t need to put links to branch/issue to each patch of patch-set -
you should do it only for covering letter.

Patch itself is OK, but I would either separate this patch from patch-set,
or add "Follow up #xxxx” sign.

[tarantool-patches] Re: [PATCH v8 3/3] sql: remove psql_txn from Vdbe

[Vladislav Shpilevoy]

Thanks for the review!

On 30/10/2018 17:30, n.pettik wrote:
> 
>> It makes no sense to store it here, and it has never
>> did.
> 
> Nit: this sentence is grammatically incorrect, I guess.

Sorry, just typo. I removed the second part of the
sentence. Look at the branch.

> 
>> SQL transaction specific things shall be taken
>> from global txn object, as any transaction specific
>> things.
> 
> Patch itself is OK, but I would either separate this patch from patch-set,
> or add "Follow up #xxxx” sign.

Done. I used 'follow up'.

[tarantool-patches] Re: [PATCH v8 0/3] sql: return all generated ids via IPROTO

[Kirill Yukhin]

Hello,
On 29 Oct 20:33, imeevma@tarantool.org wrote:
> To make implementation of function getGeneratedKeys() of JDBC
> driver we should be able to return information of generated in
> last INSERT statement keys through IPROTO.
> 
> Patch 1 separates fideb_gc() from txn_commit().
> 
> Patch 2 implements mentioned functionality.
> 
> Patch 3 do some refactoring in VDBE.
> 
> 
> Issue: https://github.com/tarantool/tarantool/issues/2618
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids
I've checked your patch set into 2.1 branch.

--
Regards, Kirill Yukhin