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 tarantool#4188

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 @@ sequence_next(struct sequence *seq, int64_t *result)
 				   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,15 @@ sequence_data_iterator_create(void)
 	light_sequence_iterator_freeze(&sequence_data_index, &iter->iter);
 	return &iter->base;
 }
+
+int64_t
+sequence_get_value(struct sequence *seq)
+{
+	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);
+	return data.value;
+}

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,15 @@ 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.
+ * @retval last element of sequence.
+ */
+int64_t
+sequence_get_value(struct sequence *seq);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 

src/box/sql/insert.c

@@ -728,6 +728,10 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			}
 		}
 
+		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 +741,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 +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);
 }

src/box/sql/sqlInt.h

@@ -3498,11 +3498,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);

src/box/sql/update.c

@@ -423,7 +423,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;

src/box/sql/vdbe.c

@@ -566,7 +566,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);
@@ -4227,12 +4227,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
@@ -4274,6 +4279,12 @@ 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 = sequence_get_value(space->sequence);
+		if (vdbe_add_new_autoinc_id(p, value) != 0)
+			goto abort_due_to_error;
+	}
 
 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
 		/* Ignore any kind of failes and do not raise error message */

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)')

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

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;')