[tarantool-patches] [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed

[imeevma@tarantool.org]

When the INSERT was executed as an INSERT OR IGNORE, it was
possible to get the generated autoincrement identifiers, even if
the insert 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);

should return only two identifiers.

After this patch, only in case of successful insertion, the
generated identifiers will be returned. Also, row-count shows now
number of successfull insertions in case of INSERT OR IGNORE.

Follow-up #4188
---
 src/box/sql/insert.c        |  9 +++++++--
 src/box/sql/sqlInt.h        | 10 +++++++++-
 src/box/sql/update.c        |  2 +-
 src/box/sql/vdbe.c          | 14 +++++++++++---
 test/sql/row-count.result   | 26 ++++++++++++++++++++++++++
 test/sql/row-count.test.lua |  7 +++++++
 6 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index db95a02..d162242 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -737,7 +737,9 @@ 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_fieldno < UINT32_MAX &&
+					       pParse->triggered_space == NULL);
 		/*
 		 * Save the newly generated autoincrement value to
 		 * VDBE context. We don't need to do so for
@@ -984,10 +986,13 @@ 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,
+			       bool is_autoinc_present)
 {
 	assert(v != NULL);
 	u16 pik_flags = OPFLAG_NCHANGE;
+	if (is_autoinc_present)
+		pik_flags |= OPFLAG_AUTOINC;
 	SET_CONFLICT_FLAG(pik_flags, on_conflict);
 	sqlVdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len,
 			  raw_data_reg + tuple_len);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 73dc6e4..f668236 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2365,6 +2365,11 @@ struct Parse {
 #define OPFLAG_NCHANGE       0x01	/* OP_Insert: Set to update db->nChange */
 				     /* Also used in P2 (not P5) of OP_Delete */
 #define OPFLAG_EPHEM         0x01	/* OP_Column: Ephemeral output is ok */
+/*
+ * OP_IdxInsert, OP_IdxReplace: Inserted tuple can contain NULL in
+ * PK field, that will be replaced by integer using sequence.
+ */
+#define OPFLAG_AUTOINC      0x100
 #define OPFLAG_OE_IGNORE    0x200	/* OP_IdxInsert: Ignore flag */
 #define OPFLAG_OE_FAIL      0x400	/* OP_IdxInsert: Fail flag */
 #define OPFLAG_OE_ROLLBACK  0x800	/* OP_IdxInsert: Rollback flag. */
@@ -3502,11 +3507,14 @@ 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 is_autoinc_present True if space has a 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,
+			       bool is_autoinc_present);
 
 void
 sql_set_multi_write(Parse *, bool);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d77bee0..d249a55 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, false);
 
 		} else {
 			int key_reg;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 47ef0ab..884c660 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4204,8 +4204,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;
@@ -4228,8 +4226,18 @@ case OP_IdxInsert: {
 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
 						     pIn2->z + pIn2->n);
 	}
+	if (rc == 0 && (pOp->p5 & OPFLAG_NCHANGE) != 0)
+		p->nChange++;
 
-	if (pOp->p5 & OPFLAG_OE_IGNORE) {
+	if ((pOp->p5 & OPFLAG_OE_IGNORE) != 0) {
+		/*
+		 * If we ignore errors, we must skip the
+		 * OP_SaveAutoincValue opcode, if present. This
+		 * should be the following opcode if the space has
+		 * a field with AUTOINCREMENT.
+		 */
+		if (rc != 0 && (pOp->p5 & OPFLAG_AUTOINC) != 0)
+			++pOp;
 		/* Ignore any kind of failes and do not raise error message */
 		rc = 0;
 		/* If we are in trigger, increment ignore raised counter */
diff --git a/test/sql/row-count.result b/test/sql/row-count.result
index e7841ca..522d2f1 100644
--- a/test/sql/row-count.result
+++ b/test/sql/row-count.result
@@ -335,3 +335,29 @@ 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]
+...
diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
index 9f5215c..30de8b4 100644
--- a/test/sql/row-count.test.lua
+++ b/test/sql/row-count.test.lua
@@ -73,3 +73,10 @@ 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;")
-- 
2.7.4