[tarantool-patches] [PATCH v4 0/4] sql: do not show IDs generated by trigger

[tarantool-patches] [PATCH v4 0/4] sql: do not show IDs generated by trigger

[imeevma]

Currently, if an INSERT was executed by a trigger during the
execution of a statement, if there were any generated identifiers,
they can be displayed as a result of the statement. This is
incorrect, since we are not able to divide the IDs obtained into
those that belong to the table mentioned in the statement and
those that do not belong to this table.

https://github.com/tarantool/tarantool/issues/4188
https://github.com/tarantool/tarantool/tree/imeevma/gh-4188-remove-additonal-generated-ids

Changes in v2:
1) Patch was divided into 3 new patches.

Changes in v3:
1) Removed tests in refactoring patch "sql: remove unnecessary
AUTOINCREMENT ID generation". They shows nothing.
2) Changed the way to check that INSERT executed by trigger.
3) Fixed a bug with the display of generated AUTOINCREMENT
identifiers, even if INSERT failed in the case of INSERT OR
IGNORE.

Changes in v4:
1) Changed the solution for the issue. Now it works without new
opcode. However, new argument was added to OP_IdxInsert.
2) Now the patch in which VDBE does not take into account failed
inserts becomes practically useless. It has been removed. However,
a new patch was added that corrects row_count in the specified
case.

Mergen Imeev (4):
  sql: remove unnecessary AUTOINCREMENT ID generation
  sql: skip autoinc IDs generated inside SQL trigger
  sql: remove VDBE from TXN
  sql: do not increase row-count if INSERT or REPLACE failed

 src/box/sequence.c          |  19 +++++---
 src/box/sequence.h          |  13 +++--
 src/box/sql/insert.c        |  18 ++++---
 src/box/sql/sqlInt.h        |   3 +-
 src/box/sql/update.c        |   2 +-
 src/box/sql/vdbe.c          |  54 ++++++++-------------
 src/box/sql/vdbe.h          |   3 +-
 src/box/sql/vdbeInt.h       |   2 +-
 src/box/sql/vdbeaux.c       |  10 ++--
 src/box/txn.h               |  17 -------
 test/sql/iproto.result      |   2 -
 test/sql/row-count.result   |  30 ++++++++++++
 test/sql/row-count.test.lua |   8 ++++
 test/sql/triggers.result    | 114 ++++++++++++++++++++++++++++++++++++++++++++
 test/sql/triggers.test.lua  |  24 ++++++++++
 15 files changed, 239 insertions(+), 80 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v4 1/4] sql: remove unnecessary AUTOINCREMENT ID generation

[imeevma]

Hi! Thank you for review. My answer and new version below.


On 7/15/19 8:50 PM, n.pettik wrote:
>
>>>> src/box/sql/insert.c                    |  5 +----
>>>> test/sql/gh-2981-check-autoinc.result   | 32 ++++++++++++++++++++++++++++++++
>>>> test/sql/gh-2981-check-autoinc.test.lua | 11 ++++++++++-
>>>> 3 files changed, 43 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>>>> index b353148..d2b4e17 100644
>>>> --- a/src/box/sql/insert.c
>>>> +++ b/src/box/sql/insert.c
>>>> @@ -628,10 +628,7 @@ sqlInsert(Parse * pParse,	/* Parser context */
>>>> 			if (j < 0 || nColumn == 0
>>>> 			    || (pColumn && j >= pColumn->nId)) {
>>>> 				if (i == (int) autoinc_fieldno) {
>>>> -					sqlVdbeAddOp2(v,
>>>> -							  OP_NextAutoincValue,
>>>> -							  space->def->id,
>>>> -							  iRegStore);
>>>
>>> Why didn’t you delete OP_NextAutioncValue?
>>>
>> I changed it functionality in the next patch.
>
> You changes affect more than 50% of opcode's content.
> Please, to make patches well structured, remove opcode
> in current patch and resurrect in the next one.
>
Removed the opcode.


New patch:

From a82a75782cc536af3e1001674759a1476af7dee7 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Wed, 3 Jul 2019 14:05:11 +0300
Subject: [PATCH] sql: remove unnecessary AUTOINCREMENT ID generation

Currently, if we perform something like
CREATE TABLE t1 (
        s1 INTEGER PRIMARY KEY AUTOINCREMENT,
        s2 INTEGER,
        CHECK (s1 <> 19)
);
INSERT INTO t1 VALUES (18, NULL);
INSERT INTO t1 (s2) VALUES (NULL);

we generate a new identifier in VDBE, but in any other case we
generate it in BOX. That was needed since the CHECK did not work
properly. This is not necessary now, because CHECK was moved to
BOX due to issue #3691. After this patch all new identifiers will
be generated in BOX.

Part of #4188

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b353148..d2b4e17 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -628,10 +628,7 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			if (j < 0 || nColumn == 0
 			    || (pColumn && j >= pColumn->nId)) {
 				if (i == (int) autoinc_fieldno) {
-					sqlVdbeAddOp2(v,
-							  OP_NextAutoincValue,
-							  space->def->id,
-							  iRegStore);
+					sqlVdbeAddOp2(v, OP_Null, 0, iRegStore);
 					continue;
 				}
 				struct Expr *dflt = NULL;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 9f4ee7a..6bb7bc3 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1149,34 +1149,6 @@ case OP_String: {          /* out2 */
 	break;
 }
 
-/* Opcode: NextAutoincValue P1 P2 * * *
- * Synopsis: r[P2] = next value from space sequence, which pageno is r[P1]
- *
- * Get next value from space sequence, which pageno is written into register
- * P1, write this value into register P2. If space doesn't exists (invalid
- * space_id or something else), raise an error. If space with
- * specified space_id doesn't have attached sequence, also raise an error.
- */
-case OP_NextAutoincValue: {
-	assert(pOp->p1 > 0);
-	assert(pOp->p2 > 0);
-
-	struct space *space = space_by_id(pOp->p1);
-	if (space == NULL)
-		goto abort_due_to_error;
-
-	int64_t value;
-	struct sequence *sequence = space->sequence;
-	if (sequence == NULL || sequence_next(sequence, &value) != 0)
-		goto abort_due_to_error;
-
-	pOut = out2Prerelease(p, pOp);
-	pOut->flags = MEM_Int;
-	pOut->u.i = value;
-
-	break;
-}
-
 /* Opcode: Null P1 P2 P3 * *
  * Synopsis: r[P2..P3]=NULL
  *

[tarantool-patches] [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger

[imeevma]

I changed the approach of the patch. My answers and new patch
below. This letter contains my answers to the earlier second and
fourth letters.

On 7/15/19 8:50 PM, n.pettik wrote:
>
> I’ve edited commit message:
>
>     sql: skip autoinc IDs generated inside SQL trigger
>     
>     Currently, if an INSERT is executed inside SQL trigger and it results
>     in generated autoincrement identifiers, ones will be displayed as a
>     result of the statement. This is wrong, since we are not able to divide
>     IDs obtained into those that belong to the table mentioned in the
>     statement and those that do not belong to this table. This has been
>     fixed by adding a new opcode OP_SaveAutoincValue, which follows each
>     OP_IdxInsert when there's autoincrement field. On the other hand, we can
>     avoid adding this opcode while producing VDBE instructions for triggers.
>     OP_SaveAutoincValue retrieves and saves recently generated identifiers
>     into the list, which is held in VDBE itself. Note that from now we don't
>     save autoincremented value to VDBE right in sequence_next() - this
>     operation is moved to OP_SaveAutoincValue. So that, VDBE can be removed
>     from struct txn.
>     
>     For example:
>     box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
>     box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
>     box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW '..
>                 'BEGIN INSERT INTO t2 VALUES (null); END')
>     box.execute('INSERT INTO t2 VALUES (100);')
>     box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
>     
>     Result should be:
>     ---
>     - autoincrement_ids:
>       - 1
>       - 2
>       - 3
>       row_count: 3
>     ...
>     
>     Closes #4188
>     
>     Closes #4188

Thanks. I changed it a bit according to new approach.


On 7/15/19 8:59 PM, n.pettik wrote:
> I’ve edited commit message:
>
>     sql: omit auto-inc ID if INSERT OR IGNORE failed
>     
>     If INSERT statement is executed with IGNORE error action
>     (i.e. INSERT OR IGNORE ...), it will return generated autoincrement
>     identifiers. Even in case the insert has failed. For example:
>     
>     CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT,
>                     a INT check (a > 0));
>     INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);
>     
>     In this case only two identifiers should be returned, but before this
>     patch all three are appeared. So, let's account only identifiers which
>     are really inserted to the space.  Also, row-count shows now
>     number of successful insertions in case of INSERT OR IGNORE.
>     
>     Follow-up #4188
>
Thank you. I used it as an template for newly fourth patch.

>
> TBO I dislike your approach for several reasons.
> Firstly, it introduces another one auxiliary flag, which
> enlarges branching, complicates code reading etc.
> Secondly, it changes program counter in OP_Insert:
> generally speaking, I’d not consider implicit incrementation
> of program counter to be a good practice. You even don’t
> check that the next instruction is OP_SaveAutoinc…
>
> Could you spend some more time trying come up with
> another solution?
>
Thanks for your notices. Now I see that I was wrong. I fixed it by
changing the approach of the main issue.

> Also, I wonder why didn’t you split OP_SaveAutoinc into
> two opcode: OP_IsNull + OP_SaveAutoinc which would
> unconditionally save autroincremented value to the vdbe?
>
I think that is not needed now.


New patch:

From c6ce7d0b81b5e93b87b2505ffbfda9e9c338b309 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Tue, 28 May 2019 17:41:15 +0300
Subject: [PATCH] sql: skip autoinc IDs generated inside SQL trigger

Currently, if an INSERT is executed inside SQL trigger and it
results in generated autoincrement identifiers, ones will be
displayed as a result of the statement. This is wrong, since we
are not able to divide IDs obtained into those that belong to the
table mentioned in the statement and those that do not belong to
this table. This has been fixed by adding a new argument to
OP_IdxInsert. In case the argument is not 0, recently generated
identifier is retrieved and saved into the list, which is held in
VDBE itself. Note that from now we don't save autoincremented
value to VDBE right in sequence_next() - this operation is moved
to OP_IdxInsert. So that, VDBE can be removed from struct txn.

For example:
box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW '..
            'BEGIN INSERT INTO t2 VALUES (null); END')
box.execute('INSERT INTO t2 VALUES (100);')
box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')

Result should be:
---
- autoincrement_ids:
- 1
- 2
- 3
row_count: 3
...

Closes #4188

diff --git a/src/box/sequence.c b/src/box/sequence.c
index 2bf38f9..5996519 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -216,9 +216,6 @@ 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);
@@ -253,9 +250,6 @@ 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) {
@@ -368,3 +362,16 @@ sequence_data_iterator_create(void)
 	light_sequence_iterator_freeze(&sequence_data_index, &iter->iter);
 	return &iter->base;
 }
+
+int
+sequence_get_value(struct sequence *seq, int64_t *out_value)
+{
+	uint32_t key = seq->def->id;
+	uint32_t hash = sequence_hash(key);
+	uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key);
+	assert(pos != light_sequence_end);
+	struct sequence_data data = light_sequence_get(&sequence_data_index,
+						       pos);
+	*out_value = data.value;
+	return 0;
+}
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 9a745ad..b56236e 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -157,9 +157,6 @@ 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.
  *
@@ -170,6 +167,16 @@ vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
 struct snapshot_iterator *
 sequence_data_iterator_create(void);
 
+/**
+ * Get last element of given sequence.
+ *
+ * @param seq sequence to get value from.
+ * @param[out] out_value last element of sequence.
+ * @retval 0 on success, -1 on error.
+ */
+int
+sequence_get_value(struct sequence *seq, int64_t *out_value);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index d2b4e17..6a2a742 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -728,6 +728,11 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			}
 		}
 
+		assert(regData > 0);
+		int autoinc_reg = 0;
+		if (autoinc_fieldno < UINT32_MAX &&
+		    pParse->triggered_space == NULL)
+			autoinc_reg = regData + autoinc_fieldno;
 		/*
 		 * Generate code to check constraints and process
 		 * final insertion.
@@ -737,7 +742,7 @@ sqlInsert(Parse * pParse,	/* Parser context */
 		fk_constraint_emit_check(pParse, space, 0, regIns, 0);
 		vdbe_emit_insertion_completion(v, space, regIns + 1,
 					       space->def->field_count,
-					       on_error);
+					       on_error, autoinc_reg);
 	}
 
 	/* Update the count of rows that are inserted
@@ -971,14 +976,16 @@ process_index:  ;
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 			       int raw_data_reg, uint32_t tuple_len,
-			       enum on_conflict_action on_conflict)
+			       enum on_conflict_action on_conflict,
+			       int autoinc_reg)
 {
 	assert(v != NULL);
 	u16 pik_flags = OPFLAG_NCHANGE;
 	SET_CONFLICT_FLAG(pik_flags, on_conflict);
 	sqlVdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len,
 			  raw_data_reg + tuple_len);
-	sqlVdbeAddOp1(v, OP_IdxInsert, raw_data_reg + tuple_len);
+	sqlVdbeAddOp3(v, OP_IdxInsert, raw_data_reg + tuple_len, 0,
+		      autoinc_reg);
 	sqlVdbeChangeP4(v, -1, (char *)space, P4_SPACEPTR);
 	sqlVdbeChangeP5(v, pik_flags);
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 3b8c82d..654eebd 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3506,7 +3506,8 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 			       int raw_data_reg, uint32_t tuple_len,
-			       enum on_conflict_action on_conflict);
+			       enum on_conflict_action on_conflict,
+			       int autoinc_reg);
 
 void
 sql_set_multi_write(Parse *, bool);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d77bee0..311e60e 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -450,7 +450,7 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 		if (on_error == ON_CONFLICT_ACTION_REPLACE) {
 			 vdbe_emit_insertion_completion(v, space, regNew,
 							space->def->field_count,
-							on_error);
+							on_error, 0);
 
 		} else {
 			int key_reg;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 6bb7bc3..951303c 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -559,7 +559,7 @@ vdbe_autoinc_id_list(struct Vdbe *vdbe)
 	return &vdbe->autoinc_id_list;
 }
 
-int
+static int
 vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
 {
 	assert(vdbe != NULL);
@@ -4161,12 +4161,17 @@ case OP_SorterInsert: {      /* in2 */
 	break;
 }
 
-/* Opcode: IdxInsert P1 P2 * P4 P5
+/* Opcode: IdxInsert P1 P2 P3 P4 P5
  * Synopsis: key=r[P1]
  *
  * @param P1 Index of a register with MessagePack data to insert.
  * @param P2 If P4 is not set, then P2 is register containing pointer
  *           to space to insert into.
+ * @param P3 If not 0, than it is an index of a register that
+ *           contains value that will be inserted into field with
+ *           AUTOINCREMENT. If the value is NULL, than the newly
+ *           generated autoincrement value will be saved to VDBE
+ *           context.
  * @param P4 Pointer to the struct space to insert to.
  * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
  *        accounts the change in a case of successful insertion in
@@ -4208,6 +4213,13 @@ case OP_IdxInsert: {
 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
 						     pIn2->z + pIn2->n);
 	}
+	if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
+		assert(space->sequence != NULL);
+		int64_t value;
+		if (sequence_get_value(space->sequence, &value) != 0)
+			goto abort_due_to_error;
+		vdbe_add_new_autoinc_id(p, value);
+	}
 
 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
 		/* Ignore any kind of failes and do not raise error message */
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 9639ba7..3580d6b 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -552,8 +552,6 @@ cn:execute('insert into test values (null, 1)')
 ---
 - autoincrement_ids:
   - 127
-  - 1
-  - 2
   row_count: 1
 ...
 box.execute('create table test3 (id int primary key autoincrement)')
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index 307b390..8ef9648 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -551,3 +551,117 @@ box.execute("DROP TABLE t1;")
 ---
 - row_count: 1
 ...
+--
+-- gh-4188: Additional generated identifiers when INSERT is
+-- executed by triggers.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t1 VALUES (100);')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [100]
+  - [101]
+  - [102]
+  - [103]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+...
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [100]
+  - [101]
+  - [102]
+  - [103]
+  - [104]
+  - [105]
+  - [106]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+  - [4]
+  - [5]
+  - [6]
+...
+box.execute('SELECT * FROM t3;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t2;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t3;')
+---
+- row_count: 1
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index a056e79..ee77192 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -191,3 +191,27 @@ box.execute([[CREATE TRIGGER r1 AFTER INSERT ON t1 FOR EACH ROW BEGIN SELECT 1;
 box.session.su('admin')
 box.schema.user.drop('tester')
 box.execute("DROP TABLE t1;")
+
+--
+-- gh-4188: Additional generated identifiers when INSERT is
+-- executed by triggers.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
+box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
+box.execute('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+box.execute('INSERT INTO t1 VALUES (100);')
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+box.execute('SELECT * FROM t3;')
+
+box.execute('DROP TABLE t1;')
+box.execute('DROP TABLE t2;')
+box.execute('DROP TABLE t3;')

[tarantool-patches] Re: [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger

[n.pettik]

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index d2b4e17..6a2a742 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -728,6 +728,11 @@ sqlInsert(Parse * pParse,	/* Parser context */
> 			}
> 		}
> 
> +		assert(regData > 0);

Pretty much useless assertion here.

> +		int autoinc_reg = 0;
> +		if (autoinc_fieldno < UINT32_MAX &&
> +		    pParse->triggered_space == NULL)
> +			autoinc_reg = regData + autoinc_fieldno;
> 		/*
> 		 * Generate code to check constraints and process
> 		 * final insertion.
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 3b8c82d..654eebd 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3506,7 +3506,8 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
> void
> vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
> 			       int raw_data_reg, uint32_t tuple_len,
> -			       enum on_conflict_action on_conflict);
> +			       enum on_conflict_action on_conflict,
> +			       int autoinc_reg);

You forgot to update comment to this function.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 6bb7bc3..951303c 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -559,7 +559,7 @@ vdbe_autoinc_id_list(struct Vdbe *vdbe)
> 	return &vdbe->autoinc_id_list;
> }
> 
> -int
> +static int
> vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
> {
> 	assert(vdbe != NULL);
> @@ -4161,12 +4161,17 @@ case OP_SorterInsert: {      /* in2 */
> 	break;
> }
> 
> -/* Opcode: IdxInsert P1 P2 * P4 P5
> +/* Opcode: IdxInsert P1 P2 P3 P4 P5
>  * Synopsis: key=r[P1]
>  *
>  * @param P1 Index of a register with MessagePack data to insert.
>  * @param P2 If P4 is not set, then P2 is register containing pointer
>  *           to space to insert into.
> + * @param P3 If not 0, than it is an index of a register that
> + *           contains value that will be inserted into field with
> + *           AUTOINCREMENT. If the value is NULL, than the newly
> + *           generated autoincrement value will be saved to VDBE
> + *           context.
>  * @param P4 Pointer to the struct space to insert to.
>  * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
>  *        accounts the change in a case of successful insertion in
> @@ -4208,6 +4213,13 @@ case OP_IdxInsert: {
> 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
> 						     pIn2->z + pIn2->n);
> 	}
> +	if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
> +		assert(space->sequence != NULL);
> +		int64_t value;
> +		if (sequence_get_value(space->sequence, &value) != 0)
> +			goto abort_due_to_error;
> +		vdbe_add_new_autoinc_id(p, value);
> +	}

I’d refactor this way (placed it at the top of last commit):

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 02f35c406..f3d70a982 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4171,7 +4171,8 @@ case OP_SorterInsert: {      /* in2 */
  *           contains value that will be inserted into field with
  *           AUTOINCREMENT. If the value is NULL, than the newly
  *           generated autoincrement value will be saved to VDBE
- *           context.
+ *           context. At the end of execution, mentioned list of
+ *           ids is returned as a result of query execution.
  * @param P4 Pointer to the struct space to insert to.
  * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
  *        accounts the change in a case of successful insertion in
@@ -4211,31 +4212,36 @@ case OP_IdxInsert: {
                rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
                                                     pIn2->z + pIn2->n);
        }
-       if (rc == 0) {
-               if (pOp->p5 & OPFLAG_NCHANGE)
-                       p->nChange++;
-               if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
-                       assert(space->sequence != NULL);
-                       int64_t value;
-                       if (sequence_get_value(space->sequence, &value) != 0)
-                               goto abort_due_to_error;
-                       vdbe_add_new_autoinc_id(p, value);
+       if (rc != 0) {
+               if ((pOp->p5 & OPFLAG_OE_IGNORE) != 0) {
+                       /*
+                        * Ignore any kind of fails and do not
+                        * raise error message.
+                        */
+                       rc = 0;
+                       /*
+                        * If we are in trigger, increment ignore
+                        * raised counter.
+                        */
+                       if (p->pFrame)
+                               p->ignoreRaised++;
+                       break;
                }
+               if (pOp->p5 & OPFLAG_OE_FAIL)
+                       p->errorAction = ON_CONFLICT_ACTION_FAIL;
+               else if (pOp->p5 & OPFLAG_OE_ROLLBACK)
+                       p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
+               goto abort_due_to_error;
        }
-
-       if (pOp->p5 & OPFLAG_OE_IGNORE) {
-               /* Ignore any kind of failes and do not raise error message */
-               rc = 0;
-               /* If we are in trigger, increment ignore raised counter */
-               if (p->pFrame)
-                       p->ignoreRaised++;
-       } else if (pOp->p5 & OPFLAG_OE_FAIL) {
-               p->errorAction = ON_CONFLICT_ACTION_FAIL;
-       } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) {
-               p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
+       if (pOp->p5 & OPFLAG_NCHANGE)
+               p->nChange++;
+       if (pOp->p3 > 0 && (aMem[pOp->p3].flags & MEM_Null) != 0) {
+               assert(space->sequence != NULL);
+               int64_t value;
+               if (sequence_get_value(space->sequence, &value) != 0)
+                       goto abort_due_to_error;
+               vdbe_add_new_autoinc_id(p, value);
        }
-       if (rc != 0)
-               goto abort_due_to_error;
        break;
 }

> 
> 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
> 		/* Ignore any kind of failes and do not raise error message */
> 
> diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
> index a056e79..ee77192 100644
> --- a/test/sql/triggers.test.lua
> +++ b/test/sql/triggers.test.lua
> @@ -191,3 +191,27 @@ box.execute([[CREATE TRIGGER r1 AFTER INSERT ON t1 FOR EACH ROW BEGIN SELECT 1;
> box.session.su('admin')
> box.schema.user.drop('tester')
> box.execute("DROP TABLE t1;")
> +
> +--
> +-- gh-4188: Additional generated identifiers when INSERT is
> +-- executed by triggers.

Please, leave comments in form “Make sure that …
doesn’t appear / is fixed” or sort of. It allows to avoid
confusion while reading such comments.

[tarantool-patches] Re: [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger

[Mergen Imeev]

Hi! Thank you for review. Diff between patches, my answers
and new patch below.

On Wed, Jul 17, 2019 at 07:58:43PM +0300, n.pettik wrote:
> 
> > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> > index d2b4e17..6a2a742 100644
> > --- a/src/box/sql/insert.c
> > +++ b/src/box/sql/insert.c
> > @@ -728,6 +728,11 @@ sqlInsert(Parse * pParse,	/* Parser context */
> > 			}
> > 		}
> > 
> > +		assert(regData > 0);
> 
> Pretty much useless assertion here.
> 
Removed, but added another, more informative.

> > +		int autoinc_reg = 0;
> > +		if (autoinc_fieldno < UINT32_MAX &&
> > +		    pParse->triggered_space == NULL)
> > +			autoinc_reg = regData + autoinc_fieldno;
> > 		/*
> > 		 * Generate code to check constraints and process
> > 		 * final insertion.
> > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> > index 3b8c82d..654eebd 100644
> > --- a/src/box/sql/sqlInt.h
> > +++ b/src/box/sql/sqlInt.h
> > @@ -3506,7 +3506,8 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
> > void
> > vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
> > 			       int raw_data_reg, uint32_t tuple_len,
> > -			       enum on_conflict_action on_conflict);
> > +			       enum on_conflict_action on_conflict,
> > +			       int autoinc_reg);
> 
> You forgot to update comment to this function.
> 
Thanks, fixed.

> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 6bb7bc3..951303c 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -559,7 +559,7 @@ vdbe_autoinc_id_list(struct Vdbe *vdbe)
> > 	return &vdbe->autoinc_id_list;
> > }
> > 
> > -int
> > +static int
> > vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
> > {
> > 	assert(vdbe != NULL);
> > @@ -4161,12 +4161,17 @@ case OP_SorterInsert: {      /* in2 */
> > 	break;
> > }
> > 
> > -/* Opcode: IdxInsert P1 P2 * P4 P5
> > +/* Opcode: IdxInsert P1 P2 P3 P4 P5
> >  * Synopsis: key=r[P1]
> >  *
> >  * @param P1 Index of a register with MessagePack data to insert.
> >  * @param P2 If P4 is not set, then P2 is register containing pointer
> >  *           to space to insert into.
> > + * @param P3 If not 0, than it is an index of a register that
> > + *           contains value that will be inserted into field with
> > + *           AUTOINCREMENT. If the value is NULL, than the newly
> > + *           generated autoincrement value will be saved to VDBE
> > + *           context.
> >  * @param P4 Pointer to the struct space to insert to.
> >  * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
> >  *        accounts the change in a case of successful insertion in
> > @@ -4208,6 +4213,13 @@ case OP_IdxInsert: {
> > 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
> > 						     pIn2->z + pIn2->n);
> > 	}
> > +	if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
> > +		assert(space->sequence != NULL);
> > +		int64_t value;
> > +		if (sequence_get_value(space->sequence, &value) != 0)
> > +			goto abort_due_to_error;
> > +		vdbe_add_new_autoinc_id(p, value);
> > +	}
> 
> I’d refactor this way (placed it at the top of last commit):
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 02f35c406..f3d70a982 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4171,7 +4171,8 @@ case OP_SorterInsert: {      /* in2 */
>   *           contains value that will be inserted into field with
>   *           AUTOINCREMENT. If the value is NULL, than the newly
>   *           generated autoincrement value will be saved to VDBE
> - *           context.
> + *           context. At the end of execution, mentioned list of
> + *           ids is returned as a result of query execution.
>   * @param P4 Pointer to the struct space to insert to.
>   * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
>   *        accounts the change in a case of successful insertion in
> @@ -4211,31 +4212,36 @@ case OP_IdxInsert: {
>                 rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
>                                                      pIn2->z + pIn2->n);
>         }
> -       if (rc == 0) {
> -               if (pOp->p5 & OPFLAG_NCHANGE)
> -                       p->nChange++;
> -               if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
> -                       assert(space->sequence != NULL);
> -                       int64_t value;
> -                       if (sequence_get_value(space->sequence, &value) != 0)
> -                               goto abort_due_to_error;
> -                       vdbe_add_new_autoinc_id(p, value);
> +       if (rc != 0) {
> +               if ((pOp->p5 & OPFLAG_OE_IGNORE) != 0) {
> +                       /*
> +                        * Ignore any kind of fails and do not
> +                        * raise error message.
> +                        */
> +                       rc = 0;
> +                       /*
> +                        * If we are in trigger, increment ignore
> +                        * raised counter.
> +                        */
> +                       if (p->pFrame)
> +                               p->ignoreRaised++;
> +                       break;
>                 }
> +               if (pOp->p5 & OPFLAG_OE_FAIL)
> +                       p->errorAction = ON_CONFLICT_ACTION_FAIL;
> +               else if (pOp->p5 & OPFLAG_OE_ROLLBACK)
> +                       p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
> +               goto abort_due_to_error;
>         }
> -
> -       if (pOp->p5 & OPFLAG_OE_IGNORE) {
> -               /* Ignore any kind of failes and do not raise error message */
> -               rc = 0;
> -               /* If we are in trigger, increment ignore raised counter */
> -               if (p->pFrame)
> -                       p->ignoreRaised++;
> -       } else if (pOp->p5 & OPFLAG_OE_FAIL) {
> -               p->errorAction = ON_CONFLICT_ACTION_FAIL;
> -       } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) {
> -               p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
> +       if (pOp->p5 & OPFLAG_NCHANGE)
> +               p->nChange++;
> +       if (pOp->p3 > 0 && (aMem[pOp->p3].flags & MEM_Null) != 0) {
> +               assert(space->sequence != NULL);
> +               int64_t value;
> +               if (sequence_get_value(space->sequence, &value) != 0)
> +                       goto abort_due_to_error;
> +               vdbe_add_new_autoinc_id(p, value);
>         }
> -       if (rc != 0)
> -               goto abort_due_to_error;
>         break;
>  }
> 
Thanks, applied.

> > 
> > 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
> > 		/* Ignore any kind of failes and do not raise error message */
> > 
> > diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
> > index a056e79..ee77192 100644
> > --- a/test/sql/triggers.test.lua
> > +++ b/test/sql/triggers.test.lua
> > @@ -191,3 +191,27 @@ box.execute([[CREATE TRIGGER r1 AFTER INSERT ON t1 FOR EACH ROW BEGIN SELECT 1;
> > box.session.su('admin')
> > box.schema.user.drop('tester')
> > box.execute("DROP TABLE t1;")
> > +
> > +--
> > +-- gh-4188: Additional generated identifiers when INSERT is
> > +-- executed by triggers.
> 
> Please, leave comments in form “Make sure that …
> doesn’t appear / is fixed” or sort of. It allows to avoid
> confusion while reading such comments.
> 
Fixed.


Diff:

commit 943ec9696ef6d8435892a40dea784d82a1f8ee98
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Jul 18 17:46:00 2019 +0300

    Review fix

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index d6ddd77..9f94c0c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -728,11 +728,12 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			}
 		}
 
-		assert(regData > 0);
 		int autoinc_reg = 0;
 		if (autoinc_fieldno < UINT32_MAX &&
 		    pParse->triggered_space == NULL)
 			autoinc_reg = regData + autoinc_fieldno;
+		assert(autoinc_fieldno == UINT32_MAX ||
+		       pParse->triggered_space != NULL || autoinc_reg > 0);
 		/*
 		 * Generate code to check constraints and process
 		 * final insertion.
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 0257ec9..6532149 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3502,6 +3502,9 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
  * @param raw_data_reg Register with raw data to insert.
  * @param tuple_len Number of registers to hold the tuple.
  * @param on_conflict On conflict action.
+ * @param autoinc_reg if not 0, then this is the register that
+ *                    contains the value that will be inserted
+ *                    into the field with AUTOINCREMENT.
  */
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index 8ef9648..dcb66b0 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -552,8 +552,8 @@ box.execute("DROP TABLE t1;")
 - row_count: 1
 ...
 --
--- gh-4188: Additional generated identifiers when INSERT is
--- executed by triggers.
+-- gh-4188: make sure that the identifiers that were generated
+-- during the INSERT performed by the triggers are not returned.
 --
 box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
 ---
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index ee77192..f9d50b4 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -193,8 +193,8 @@ box.schema.user.drop('tester')
 box.execute("DROP TABLE t1;")
 
 --
--- gh-4188: Additional generated identifiers when INSERT is
--- executed by triggers.
+-- gh-4188: make sure that the identifiers that were generated
+-- during the INSERT performed by the triggers are not returned.
 --
 box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
 box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')


New patch:

From dfb07020cae8e7cf9f6e079fd8efe9e20fe144fe Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Tue, 28 May 2019 17:41:15 +0300
Subject: [PATCH] sql: skip autoinc IDs generated inside SQL trigger

Currently, if an INSERT is executed inside SQL trigger and it
results in generated autoincrement identifiers, ones will be
displayed as a result of the statement. This is wrong, since we
are not able to divide IDs obtained into those that belong to the
table mentioned in the statement and those that do not belong to
this table. This has been fixed by adding a new argument to
OP_IdxInsert. In case the argument is not 0, recently generated
identifier is retrieved and saved into the list, which is held in
VDBE itself. Note that from now we don't save autoincremented
value to VDBE right in sequence_next() - this operation is moved
to OP_IdxInsert. So that, VDBE can be removed from struct txn.

For example:
box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW '..
            'BEGIN INSERT INTO t2 VALUES (null); END')
box.execute('INSERT INTO t2 VALUES (100);')
box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')

Result should be:
---
- autoincrement_ids:
  - 1
  - 2
  - 3
  row_count: 3
...

Closes #4188

diff --git a/src/box/sequence.c b/src/box/sequence.c
index 2bf38f9..5996519 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -216,9 +216,6 @@ 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);
@@ -253,9 +250,6 @@ 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) {
@@ -368,3 +362,16 @@ sequence_data_iterator_create(void)
 	light_sequence_iterator_freeze(&sequence_data_index, &iter->iter);
 	return &iter->base;
 }
+
+int
+sequence_get_value(struct sequence *seq, int64_t *out_value)
+{
+	uint32_t key = seq->def->id;
+	uint32_t hash = sequence_hash(key);
+	uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key);
+	assert(pos != light_sequence_end);
+	struct sequence_data data = light_sequence_get(&sequence_data_index,
+						       pos);
+	*out_value = data.value;
+	return 0;
+}
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 9a745ad..b56236e 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -157,9 +157,6 @@ 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.
  *
@@ -170,6 +167,16 @@ vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
 struct snapshot_iterator *
 sequence_data_iterator_create(void);
 
+/**
+ * Get last element of given sequence.
+ *
+ * @param seq sequence to get value from.
+ * @param[out] out_value last element of sequence.
+ * @retval 0 on success, -1 on error.
+ */
+int
+sequence_get_value(struct sequence *seq, int64_t *out_value);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 9a77f3b..9f94c0c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -728,6 +728,12 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			}
 		}
 
+		int autoinc_reg = 0;
+		if (autoinc_fieldno < UINT32_MAX &&
+		    pParse->triggered_space == NULL)
+			autoinc_reg = regData + autoinc_fieldno;
+		assert(autoinc_fieldno == UINT32_MAX ||
+		       pParse->triggered_space != NULL || autoinc_reg > 0);
 		/*
 		 * Generate code to check constraints and process
 		 * final insertion.
@@ -737,7 +743,7 @@ sqlInsert(Parse * pParse,	/* Parser context */
 		fk_constraint_emit_check(pParse, space, 0, regIns, 0);
 		vdbe_emit_insertion_completion(v, space, regIns + 1,
 					       space->def->field_count,
-					       on_error);
+					       on_error, autoinc_reg);
 	}
 
 	/* Update the count of rows that are inserted
@@ -972,14 +978,16 @@ process_index:  ;
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 			       int raw_data_reg, uint32_t tuple_len,
-			       enum on_conflict_action on_conflict)
+			       enum on_conflict_action on_conflict,
+			       int autoinc_reg)
 {
 	assert(v != NULL);
 	u16 pik_flags = OPFLAG_NCHANGE;
 	SET_CONFLICT_FLAG(pik_flags, on_conflict);
 	sqlVdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len,
 			  raw_data_reg + tuple_len);
-	sqlVdbeAddOp1(v, OP_IdxInsert, raw_data_reg + tuple_len);
+	sqlVdbeAddOp3(v, OP_IdxInsert, raw_data_reg + tuple_len, 0,
+		      autoinc_reg);
 	sqlVdbeChangeP4(v, -1, (char *)space, P4_SPACEPTR);
 	sqlVdbeChangeP5(v, pik_flags);
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 4f5bad2..6532149 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3502,11 +3502,15 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
  * @param raw_data_reg Register with raw data to insert.
  * @param tuple_len Number of registers to hold the tuple.
  * @param on_conflict On conflict action.
+ * @param autoinc_reg if not 0, then this is the register that
+ *                    contains the value that will be inserted
+ *                    into the field with AUTOINCREMENT.
  */
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 			       int raw_data_reg, uint32_t tuple_len,
-			       enum on_conflict_action on_conflict);
+			       enum on_conflict_action on_conflict,
+			       int autoinc_reg);
 
 void
 sql_set_multi_write(Parse *, bool);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d77bee0..311e60e 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -450,7 +450,7 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 		if (on_error == ON_CONFLICT_ACTION_REPLACE) {
 			 vdbe_emit_insertion_completion(v, space, regNew,
 							space->def->field_count,
-							on_error);
+							on_error, 0);
 
 		} else {
 			int key_reg;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index aaf4e5d..af0c51e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -559,7 +559,7 @@ vdbe_autoinc_id_list(struct Vdbe *vdbe)
 	return &vdbe->autoinc_id_list;
 }
 
-int
+static int
 vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
 {
 	assert(vdbe != NULL);
@@ -4170,12 +4170,17 @@ case OP_SorterInsert: {      /* in2 */
 	break;
 }
 
-/* Opcode: IdxInsert P1 P2 * P4 P5
+/* Opcode: IdxInsert P1 P2 P3 P4 P5
  * Synopsis: key=r[P1]
  *
  * @param P1 Index of a register with MessagePack data to insert.
  * @param P2 If P4 is not set, then P2 is register containing pointer
  *           to space to insert into.
+ * @param P3 If not 0, than it is an index of a register that
+ *           contains value that will be inserted into field with
+ *           AUTOINCREMENT. If the value is NULL, than the newly
+ *           generated autoincrement value will be saved to VDBE
+ *           context.
  * @param P4 Pointer to the struct space to insert to.
  * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
  *        accounts the change in a case of successful insertion in
@@ -4217,6 +4222,13 @@ case OP_IdxInsert: {
 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
 						     pIn2->z + pIn2->n);
 	}
+	if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
+		assert(space->sequence != NULL);
+		int64_t value;
+		if (sequence_get_value(space->sequence, &value) != 0)
+			goto abort_due_to_error;
+		vdbe_add_new_autoinc_id(p, value);
+	}
 
 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
 		/* Ignore any kind of failes and do not raise error message */
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 9639ba7..3580d6b 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -552,8 +552,6 @@ cn:execute('insert into test values (null, 1)')
 ---
 - autoincrement_ids:
   - 127
-  - 1
-  - 2
   row_count: 1
 ...
 box.execute('create table test3 (id int primary key autoincrement)')
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index 307b390..dcb66b0 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -551,3 +551,117 @@ box.execute("DROP TABLE t1;")
 ---
 - row_count: 1
 ...
+--
+-- gh-4188: make sure that the identifiers that were generated
+-- during the INSERT performed by the triggers are not returned.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t1 VALUES (100);')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [100]
+  - [101]
+  - [102]
+  - [103]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+...
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [100]
+  - [101]
+  - [102]
+  - [103]
+  - [104]
+  - [105]
+  - [106]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+  - [4]
+  - [5]
+  - [6]
+...
+box.execute('SELECT * FROM t3;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t2;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t3;')
+---
+- row_count: 1
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index a056e79..f9d50b4 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -191,3 +191,27 @@ box.execute([[CREATE TRIGGER r1 AFTER INSERT ON t1 FOR EACH ROW BEGIN SELECT 1;
 box.session.su('admin')
 box.schema.user.drop('tester')
 box.execute("DROP TABLE t1;")
+
+--
+-- gh-4188: make sure that the identifiers that were generated
+-- during the INSERT performed by the triggers are not returned.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
+box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
+box.execute('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+box.execute('INSERT INTO t1 VALUES (100);')
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+box.execute('SELECT * FROM t3;')
+
+box.execute('DROP TABLE t1;')
+box.execute('DROP TABLE t2;')
+box.execute('DROP TABLE t3;')

[tarantool-patches] [PATCH v4 3/4] sql: remove VDBE from TXN

[imeevma]

VDBE was added to TXN because the generated identifiers were added
to VDBE in the sequence_next() function. Since they are now stored
in the VDBE itself, it is not necessary to have it in TXN.

Follow-up #4188
---
 src/box/sql/vdbe.c    |  4 ++--
 src/box/sql/vdbe.h    |  3 +--
 src/box/sql/vdbeInt.h |  2 +-
 src/box/sql/vdbeaux.c | 10 ++++------
 src/box/txn.h         | 17 -----------------
 5 files changed, 8 insertions(+), 28 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 951303c..3b38bc2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2898,7 +2898,7 @@ case OP_CheckViewReferences: {
  * Otherwise, raise an error with appropriate error message.
  */
 case OP_TransactionBegin: {
-	if (sql_txn_begin(p) != 0)
+	if (sql_txn_begin() != 0)
 		goto abort_due_to_error;
 	p->auto_commit = false	;
 	break;
@@ -2954,7 +2954,7 @@ case OP_TransactionRollback: {
  */
 case OP_TTransaction: {
 	if (!box_txn()) {
-		if (sql_txn_begin(p) != 0)
+		if (sql_txn_begin() != 0)
 			goto abort_due_to_error;
 	} else {
 		p->anonymous_savepoint = sql_savepoint(p, NULL);
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 4f94643..fde898d 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -175,11 +175,10 @@ Vdbe *sqlVdbeCreate(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(struct Vdbe *v);
+sql_alloc_txn();
 
 /**
  * Prepare given VDBE to execution: initialize structs connected
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 6bfeecc..30cac07 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -440,7 +440,7 @@ u32 sqlVdbeSerialGet(const unsigned char *, u32, Mem *);
 int sqlVdbeExec(Vdbe *);
 int sqlVdbeList(Vdbe *);
 int
-sql_txn_begin(Vdbe *p);
+sql_txn_begin();
 Savepoint *
 sql_savepoint(Vdbe *p,
 	      const char *zName);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index d1388f2..4e6d7d8 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -77,7 +77,7 @@ sqlVdbeCreate(Parse * pParse)
 }
 
 struct sql_txn *
-sql_alloc_txn(struct Vdbe *v)
+sql_alloc_txn()
 {
 	struct sql_txn *txn = region_alloc_object(&fiber()->gc,
 						  struct sql_txn);
@@ -86,7 +86,6 @@ sql_alloc_txn(struct Vdbe *v)
 			 "struct sql_txn");
 		return NULL;
 	}
-	txn->vdbe = v;
 	txn->pSavepoint = NULL;
 	txn->fk_deferred_count = 0;
 	return txn;
@@ -106,11 +105,10 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
 		 * check FK violations, at least now.
 		 */
 		if (txn->psql_txn == NULL) {
-			txn->psql_txn = sql_alloc_txn(vdbe);
+			txn->psql_txn = sql_alloc_txn();
 			if (txn->psql_txn == NULL)
 				return -1;
 		}
-		txn->psql_txn->vdbe = vdbe;
 	}
 	return 0;
 }
@@ -1997,7 +1995,7 @@ sqlVdbeCheckFk(Vdbe * p, int deferred)
 }
 
 int
-sql_txn_begin(Vdbe *p)
+sql_txn_begin()
 {
 	if (in_txn()) {
 		diag_set(ClientError, ER_ACTIVE_TRANSACTION);
@@ -2006,7 +2004,7 @@ sql_txn_begin(Vdbe *p)
 	struct txn *ptxn = txn_begin(false);
 	if (ptxn == NULL)
 		return -1;
-	ptxn->psql_txn = sql_alloc_txn(p);
+	ptxn->psql_txn = sql_alloc_txn();
 	if (ptxn->psql_txn == NULL) {
 		box_txn_rollback();
 		return -1;
diff --git a/src/box/txn.h b/src/box/txn.h
index baeaa0e..271d5f9 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -118,8 +118,6 @@ struct sql_txn {
 	 * VDBE to the next in the same transaction.
 	 */
 	uint32_t fk_deferred_count;
-	/** Current VDBE. */
-	struct Vdbe *vdbe;
 };
 
 /**
@@ -412,21 +410,6 @@ txn_last_stmt(struct txn *txn)
 }
 
 /**
- * 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
  * arguments
  */
-- 
2.7.4

[tarantool-patches] [PATCH v4 4/4] sql: do not increase row-count if INSERT or REPLACE failed

[imeevma]

If INSERT statement is executed with IGNORE error action (i.e.
INSERT OR IGNORE ...), it will return number of rows inserted.
For example:

CREATE TABLE t (i INT PRIMARY KEY, a INT check (a > 0));
INSERT OR IGNORE INTO t VALUES (1, 1), (2, -1), (3, 2);

Should return:
---
- row_count: 2
...

However it was three before this patch. So, let's account number
of successful insertions in case of INSERT or REPLACE.

Follow-up #4188
---
 src/box/sql/vdbe.c          | 18 ++++++++++--------
 test/sql/row-count.result   | 30 ++++++++++++++++++++++++++++++
 test/sql/row-count.test.lua |  8 ++++++++
 3 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 3b38bc2..02f35c4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4189,8 +4189,6 @@ case OP_IdxReplace:
 case OP_IdxInsert: {
 	pIn2 = &aMem[pOp->p1];
 	assert((pIn2->flags & MEM_Blob) != 0);
-	if (pOp->p5 & OPFLAG_NCHANGE)
-		p->nChange++;
 	if (ExpandBlob(pIn2) != 0)
 		goto abort_due_to_error;
 	struct space *space;
@@ -4213,12 +4211,16 @@ case OP_IdxInsert: {
 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
 						     pIn2->z + pIn2->n);
 	}
-	if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
-		assert(space->sequence != NULL);
-		int64_t value;
-		if (sequence_get_value(space->sequence, &value) != 0)
-			goto abort_due_to_error;
-		vdbe_add_new_autoinc_id(p, value);
+	if (rc == 0) {
+		if (pOp->p5 & OPFLAG_NCHANGE)
+			p->nChange++;
+		if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
+			assert(space->sequence != NULL);
+			int64_t value;
+			if (sequence_get_value(space->sequence, &value) != 0)
+				goto abort_due_to_error;
+			vdbe_add_new_autoinc_id(p, value);
+		}
 	}
 
 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
diff --git a/test/sql/row-count.result b/test/sql/row-count.result
index e7841ca..69bfb78 100644
--- a/test/sql/row-count.result
+++ b/test/sql/row-count.result
@@ -335,3 +335,33 @@ box.execute("DROP TABLE t1;")
 ---
 - row_count: 1
 ...
+--
+-- gh-4188: check that generated IDs is not showed for failed
+-- insertions in case of INSERT OR IGNORE.
+--
+box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
+---
+- row_count: 1
+...
+box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
+---
+- autoincrement_ids:
+  - 1
+  - 3
+  row_count: 2
+...
+box.execute("SELECT * FROM t;")
+---
+- metadata:
+  - name: I
+    type: integer
+  - name: A
+    type: integer
+  rows:
+  - [1, 1]
+  - [3, 2]
+...
+box.execute("DROP TABLE t;")
+---
+- row_count: 1
+...
diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
index 9f5215c..965408e 100644
--- a/test/sql/row-count.test.lua
+++ b/test/sql/row-count.test.lua
@@ -73,3 +73,11 @@ box.execute("DROP TABLE t2;")
 box.execute("DROP TABLE t3;")
 box.execute("DROP TABLE t1;")
 
+--
+-- gh-4188: check that generated IDs is not showed for failed
+-- insertions in case of INSERT OR IGNORE.
+--
+box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
+box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
+box.execute("SELECT * FROM t;")
+box.execute("DROP TABLE t;")
-- 
2.7.4

[tarantool-patches] Re: [PATCH v4 4/4] sql: do not increase row-count if INSERT or REPLACE failed

[n.pettik]

Subject says:
"sql: do not increase row-count if INSERT or REPLACE failed"

But in fact it is about INSERT OR IGNORE statement.

[tarantool-patches] Re: [tarantool-patches] Re: [PATCH v4 4/4] sql: do not increase row-count if INSERT or REPLACE failed

[Мерген Имеев]

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




>Среда, 17 июля 2019, 19:57 +03:00 от n.pettik <korablev@tarantool.org>:
>
>Subject says:
>"sql: do not increase row-count if INSERT or REPLACE failed"
>
>But in fact it is about INSERT OR IGNORE statement.
>

It fixes for INSERT and for REPLACE, but only in case
INSERT OR IGNORE it is shown to user.


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

[tarantool-patches] Re: [PATCH v4 4/4] sql: do not increase row-count if INSERT or REPLACE failed

[Mergen Imeev]

On Wed, Jul 17, 2019 at 07:57:16PM +0300, n.pettik wrote:
> Subject says:
> "sql: do not increase row-count if INSERT or REPLACE failed"
> 
> But in fact it is about INSERT OR IGNORE statement.
Fixed.

Diff beween patches:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a8ff8af..69ad3fc 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4220,31 +4220,37 @@ case OP_IdxInsert: {
 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
 						     pIn2->z + pIn2->n);
 	}
-	if (rc == 0) {
-		if (pOp->p5 & OPFLAG_NCHANGE)
-			p->nChange++;
-		if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
-			assert(space->sequence != NULL);
-			int64_t value;
-			if (sequence_get_value(space->sequence, &value) != 0)
-				goto abort_due_to_error;
-			vdbe_add_new_autoinc_id(p, value);
+	if (rc != 0) {
+		if (pOp->p5 & OPFLAG_OE_IGNORE) {
+			/*
+			 * Ignore any kind of failes and do not
+			 * raise error message
+			 */
+			rc = 0;
+			/*
+			 * If we are in trigger, increment ignore
+			 * raised counter
+			 */
+			if (p->pFrame)
+				p->ignoreRaised++;
+			break;
 		}
+		if (pOp->p5 & OPFLAG_OE_FAIL) {
+			p->errorAction = ON_CONFLICT_ACTION_FAIL;
+		} else if (pOp->p5 & OPFLAG_OE_ROLLBACK) {
+			p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
+		}
+		goto abort_due_to_error;
 	}
-
-	if (pOp->p5 & OPFLAG_OE_IGNORE) {
-		/* Ignore any kind of failes and do not raise error message */
-		rc = 0;
-		/* If we are in trigger, increment ignore raised counter */
-		if (p->pFrame)
-			p->ignoreRaised++;
-	} else if (pOp->p5 & OPFLAG_OE_FAIL) {
-		p->errorAction = ON_CONFLICT_ACTION_FAIL;
-	} else if (pOp->p5 & OPFLAG_OE_ROLLBACK) {
-		p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
+	if (pOp->p5 & OPFLAG_NCHANGE)
+		p->nChange++;
+	if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
+		assert(space->sequence != NULL);
+		int64_t value;
+		if (sequence_get_value(space->sequence, &value) != 0)
+			goto abort_due_to_error;
+		vdbe_add_new_autoinc_id(p, value);
 	}
-	if (rc != 0)
-		goto abort_due_to_error;
 	break;
 }
 
diff --git a/test/sql/row-count.result b/test/sql/row-count.result
index 69bfb78..94ebaf0 100644
--- a/test/sql/row-count.result
+++ b/test/sql/row-count.result
@@ -336,8 +336,8 @@ box.execute("DROP TABLE t1;")
 - row_count: 1
 ...
 --
--- gh-4188: check that generated IDs is not showed for failed
--- insertions in case of INSERT OR IGNORE.
+-- gh-4188: make sure that in case of INSERT OR IGNORE only
+-- successful inserts are counted.
 --
 box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
 ---
diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
index 965408e..af7f168 100644
--- a/test/sql/row-count.test.lua
+++ b/test/sql/row-count.test.lua
@@ -74,8 +74,8 @@ box.execute("DROP TABLE t3;")
 box.execute("DROP TABLE t1;")
 
 --
--- gh-4188: check that generated IDs is not showed for failed
--- insertions in case of INSERT OR IGNORE.
+-- gh-4188: make sure that in case of INSERT OR IGNORE only
+-- successful inserts are counted.
 --
 box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
 box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")


New patch:

From c987059d25f8a63931a1bb809d5ac71c53d64656 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Wed, 17 Jul 2019 12:26:04 +0300
Subject: [PATCH] sql: do not increase row-count if INSERT OR IGNORE fails

If INSERT statement is executed with IGNORE error action (i.e.
INSERT OR IGNORE ...), it will return number of rows inserted.
For example:

CREATE TABLE t (i INT PRIMARY KEY, a INT check (a > 0));
INSERT OR IGNORE INTO t VALUES (1, 1), (2, -1), (3, 2);

Should return:
---
- row_count: 2
...

However it was three before this patch. So, let's account number
of successful insertions in case of INSERT OR IGNORE.

Follow-up #4188

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 8ac3afb..69ad3fc 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4198,8 +4198,6 @@ case OP_IdxReplace:
 case OP_IdxInsert: {
 	pIn2 = &aMem[pOp->p1];
 	assert((pIn2->flags & MEM_Blob) != 0);
-	if (pOp->p5 & OPFLAG_NCHANGE)
-		p->nChange++;
 	if (ExpandBlob(pIn2) != 0)
 		goto abort_due_to_error;
 	struct space *space;
@@ -4222,27 +4220,37 @@ case OP_IdxInsert: {
 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
 						     pIn2->z + pIn2->n);
 	}
-	if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
+	if (rc != 0) {
+		if (pOp->p5 & OPFLAG_OE_IGNORE) {
+			/*
+			 * Ignore any kind of failes and do not
+			 * raise error message
+			 */
+			rc = 0;
+			/*
+			 * If we are in trigger, increment ignore
+			 * raised counter
+			 */
+			if (p->pFrame)
+				p->ignoreRaised++;
+			break;
+		}
+		if (pOp->p5 & OPFLAG_OE_FAIL) {
+			p->errorAction = ON_CONFLICT_ACTION_FAIL;
+		} else if (pOp->p5 & OPFLAG_OE_ROLLBACK) {
+			p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
+		}
+		goto abort_due_to_error;
+	}
+	if (pOp->p5 & OPFLAG_NCHANGE)
+		p->nChange++;
+	if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
 		assert(space->sequence != NULL);
 		int64_t value;
 		if (sequence_get_value(space->sequence, &value) != 0)
 			goto abort_due_to_error;
 		vdbe_add_new_autoinc_id(p, value);
 	}
-
-	if (pOp->p5 & OPFLAG_OE_IGNORE) {
-		/* Ignore any kind of failes and do not raise error message */
-		rc = 0;
-		/* If we are in trigger, increment ignore raised counter */
-		if (p->pFrame)
-			p->ignoreRaised++;
-	} else if (pOp->p5 & OPFLAG_OE_FAIL) {
-		p->errorAction = ON_CONFLICT_ACTION_FAIL;
-	} else if (pOp->p5 & OPFLAG_OE_ROLLBACK) {
-		p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
-	}
-	if (rc != 0)
-		goto abort_due_to_error;
 	break;
 }
 
diff --git a/test/sql/row-count.result b/test/sql/row-count.result
index e7841ca..94ebaf0 100644
--- a/test/sql/row-count.result
+++ b/test/sql/row-count.result
@@ -335,3 +335,33 @@ box.execute("DROP TABLE t1;")
 ---
 - row_count: 1
 ...
+--
+-- gh-4188: make sure that in case of INSERT OR IGNORE only
+-- successful inserts are counted.
+--
+box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
+---
+- row_count: 1
+...
+box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
+---
+- autoincrement_ids:
+  - 1
+  - 3
+  row_count: 2
+...
+box.execute("SELECT * FROM t;")
+---
+- metadata:
+  - name: I
+    type: integer
+  - name: A
+    type: integer
+  rows:
+  - [1, 1]
+  - [3, 2]
+...
+box.execute("DROP TABLE t;")
+---
+- row_count: 1
+...
diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
index 9f5215c..af7f168 100644
--- a/test/sql/row-count.test.lua
+++ b/test/sql/row-count.test.lua
@@ -73,3 +73,11 @@ box.execute("DROP TABLE t2;")
 box.execute("DROP TABLE t3;")
 box.execute("DROP TABLE t1;")
 
+--
+-- gh-4188: make sure that in case of INSERT OR IGNORE only
+-- successful inserts are counted.
+--
+box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
+box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
+box.execute("SELECT * FROM t;")
+box.execute("DROP TABLE t;")

[tarantool-patches] Re: [PATCH v4 4/4] sql: do not increase row-count if INSERT or REPLACE failed

[n.pettik]

I’ve force pushed a couple of cosmetic fixes.
Also added check of return value of
vdbe_add_new_autoinc_id() - it can fail due to OOM.

Now LGTM.

[tarantool-patches] Re: [PATCH v4 4/4] sql: do not increase row-count if INSERT or REPLACE failed

[Mergen Imeev]

Hi! Thank you for review! Just before sending patch to Kirill, I
discovered that the function sequence_get_value() returns only 0.
Because of this, it makes no sense to check the result of the
function. Since this function used a special argument to return a
value, I deleted this argument and made the function now return
this value.

Diff below.

On Mon, Jul 22, 2019 at 01:48:44PM +0300, n.pettik wrote:
> I’ve force pushed a couple of cosmetic fixes.
> Also added check of return value of
> vdbe_add_new_autoinc_id() - it can fail due to OOM.
> 
> Now LGTM.
i> 

diff --git a/src/box/sequence.c b/src/box/sequence.c
index 5996519..1aacc50 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -363,8 +363,8 @@ sequence_data_iterator_create(void)
 	return &iter->base;
 }
 
-int
-sequence_get_value(struct sequence *seq, int64_t *out_value)
+int64_t
+sequence_get_value(struct sequence *seq)
 {
 	uint32_t key = seq->def->id;
 	uint32_t hash = sequence_hash(key);
@@ -372,6 +372,5 @@ sequence_get_value(struct sequence *seq, int64_t *out_value)
 	assert(pos != light_sequence_end);
 	struct sequence_data data = light_sequence_get(&sequence_data_index,
 						       pos);
-	*out_value = data.value;
-	return 0;
+	return data.value;
 }
diff --git a/src/box/sequence.h b/src/box/sequence.h
index b56236e..976020a 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -171,11 +171,10 @@ sequence_data_iterator_create(void);
  * Get last element of given sequence.
  *
  * @param seq sequence to get value from.
- * @param[out] out_value last element of sequence.
- * @retval 0 on success, -1 on error.
+ * @retval last element of sequence.
  */
-int
-sequence_get_value(struct sequence *seq, int64_t *out_value);
+int64_t
+sequence_get_value(struct sequence *seq);
 
 #if defined(__cplusplus)
 } /* extern "C" */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index f771843..431b4d9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4224,9 +4224,7 @@ case OP_IdxInsert: {
 	}
 	if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
 		assert(space->sequence != NULL);
-		int64_t value;
-		if (sequence_get_value(space->sequence, &value) != 0)
-			goto abort_due_to_error;
+		int64_t value = sequence_get_value(space->sequence);
 		if (vdbe_add_new_autoinc_id(p, value) != 0)
 			goto abort_due_to_error;
 	}

[tarantool-patches] Re: [PATCH v4 0/4] sql: do not show IDs generated by trigger

[Kirill Yukhin]

Hello,

On 17 Jul 12:54, imeevma@tarantool.org wrote:
> Currently, if an INSERT was executed by a trigger during the
> execution of a statement, if there were any generated identifiers,
> they can be displayed as a result of the statement. This is
> incorrect, since we are not able to divide the IDs obtained into
> those that belong to the table mentioned in the statement and
> those that do not belong to this table.
> 
> https://github.com/tarantool/tarantool/issues/4188
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4188-remove-additonal-generated-ids
> 
> Changes in v2:
> 1) Patch was divided into 3 new patches.
> 
> Changes in v3:
> 1) Removed tests in refactoring patch "sql: remove unnecessary
> AUTOINCREMENT ID generation". They shows nothing.
> 2) Changed the way to check that INSERT executed by trigger.
> 3) Fixed a bug with the display of generated AUTOINCREMENT
> identifiers, even if INSERT failed in the case of INSERT OR
> IGNORE.
> 
> Changes in v4:
> 1) Changed the solution for the issue. Now it works without new
> opcode. However, new argument was added to OP_IdxInsert.
> 2) Now the patch in which VDBE does not take into account failed
> inserts becomes practically useless. It has been removed. However,
> a new patch was added that corrects row_count in the specified
> case.
> 
> Mergen Imeev (4):
>   sql: remove unnecessary AUTOINCREMENT ID generation
>   sql: skip autoinc IDs generated inside SQL trigger
>   sql: remove VDBE from TXN
>   sql: do not increase row-count if INSERT or REPLACE failed

I've checked the patch set into master.

--
Regards, Kirill Yukhin