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

[Mergen Imeev <imeevma@tarantool.org>]

Sorry, forgot to drop table in test. Fixed here and on branch.
Diff and new patch below.

Diff:

diff --git a/test/sql/row-count.result b/test/sql/row-count.result
index 522d2f1..69bfb78 100644
--- a/test/sql/row-count.result
+++ b/test/sql/row-count.result
@@ -361,3 +361,7 @@ box.execute("SELECT * FROM t;")
   - [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 30de8b4..965408e 100644
--- a/test/sql/row-count.test.lua
+++ b/test/sql/row-count.test.lua
@@ -80,3 +80,4 @@ box.execute("DROP TABLE t1;")
 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;")

New patch:

From 988b155477f12ed27cf20edf14a6a88d6f995a2b Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Wed, 10 Jul 2019 12:56:16 +0300
Subject: [PATCH] sql: do not show generated IDs if INSERT failed

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

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..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;")