[tarantool-patches] [PATCH] sql: fix non-working REPLACE optimization

[Bulat Niatshin <niatshin@tarantool.org>]

SQL PRIMARY KEY constraint with ON CONFLICT REPLACE/IGNORE
optimization (when error action is REPLACE or IGNORE, table
doesn't have related foreign keys, attached triggers and
secondary indexes and because of that uniqueness can be checked
by Tarantool only without bytecode) didn't work. Also ON CONFLICT
IGNORE uniqueness check can be optimized. This patch contrains a
small fix for that.

Closes #3293
---
 src/box/sql/insert.c          | 59 +++++++++++++++++++++++++++++++++----------
 src/box/sql/sqliteInt.h       |  4 +--
 src/box/sql/update.c          |  7 +++--
 test/sql/on-conflict.result   | 32 +++++++++++++++++++++++
 test/sql/on-conflict.test.lua | 13 ++++++++++
 5 files changed, 97 insertions(+), 18 deletions(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 42254ddf4..2c2fc41f2 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -829,10 +829,12 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		 */
 		int isReplace;	/* Set to true if constraints may cause a replace */
 		int bUseSeek;	/* True to use OPFLAG_SEEKRESULT */
+		int optimization_availability = ON_CONFLICT_ACTION_NONE;
 		sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
 						iIdxCur, regIns, 0,
 						ipkColumn >= 0, onError,
-						endOfLoop, &isReplace, 0);
+						endOfLoop, &isReplace, 0,
+						&optimization_availability);
 		sqlite3FkCheck(pParse, pTab, 0, regIns, 0);
 
 		/* Set the OPFLAG_USESEEKRESULT flag if either (a) there are no REPLACE
@@ -849,7 +851,8 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 						SQLITE_ForeignKeys) == 0 ||
 					       sqlite3FkReferences(pTab) == 0));
 		sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx,
-					 bUseSeek, onError);
+					 bUseSeek, onError,
+					 optimization_availability);
 	}
 
 	/* Update the count of rows that are inserted
@@ -1053,7 +1056,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 				u8 overrideError,	/* Override onError to this if not ON_CONFLICT_ACTION_DEFAULT */
 				int ignoreDest,		/* Jump to this label on an ON_CONFLICT_ACTION_IGNORE resolution */
 				int *pbMayReplace,	/* OUT: Set to true if constraint may cause a replace */
-				int *aiChng)		/* column i is unchanged if aiChng[i]<0 */
+				int *aiChng,		/* column i is unchanged if aiChng[i]<0 */
+				int *optimization_opt) 	/* optimization flag */
 {
 	Vdbe *v;		/* VDBE under constrution */
 	Index *pIdx;		/* Pointer to one of the indices */
@@ -1324,19 +1328,30 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			onError = ON_CONFLICT_ACTION_ABORT;
 		}
 
-		/* Collision detection may be omitted if all of the following are true:
-		 *   (1) The conflict resolution algorithm is REPLACE
-		 *   (2) There are no secondary indexes on the table
-		 *   (3) No delete triggers need to be fired if there is a conflict
-		 *   (4) No FK constraint counters need to be updated if a conflict occurs.
+		/**
+		 * Collision detection may be omitted if all of
+		 * the following are true:
+		 *   (1) The conflict resolution algorithm is
+		 *       REPLACE or IGNORE.
+		 *   (2) There are no secondary indexes on the
+		 *       table.
+		 *   (3) No delete triggers need to be fired if
+		 *       there is a conflict.
+		 *   (4) No FK constraint counters need to be
+		 *       updated if a conflict occurs.
 		 */
 		if ((ix == 0 && pIdx->pNext == 0)	/* Condition 2 */
-		    && onError == ON_CONFLICT_ACTION_REPLACE	/* Condition 1 */
-		    && (0 == (user_session->sql_flags & SQLITE_RecTriggers)	/* Condition 3 */
+		    /* Condition 1 */
+		    && (onError == ON_CONFLICT_ACTION_REPLACE ||
+			onError == ON_CONFLICT_ACTION_IGNORE)
+		    /* Condition 3 */
+		    && (0 == (user_session->sql_flags & SQLITE_RecTriggers)
 			||0 == sqlite3TriggersExist(pTab, TK_DELETE, 0, 0))
-		    && (0 == (user_session->sql_flags & SQLITE_ForeignKeys) ||	/* Condition 4 */
+		    /* Condition 4 */
+		    && (0 == (user_session->sql_flags & SQLITE_ForeignKeys) ||
 			(0 == pTab->pFKey && 0 == sqlite3FkReferences(pTab)))
 		    ) {
+			*optimization_opt = onError;
 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
 			continue;
 		}
@@ -1474,13 +1489,18 @@ sqlite3CompleteInsertion(Parse * pParse,	/* The parser context */
 			 int iIdxCur,		/* Primary index cursor */
 			 int *aRegIdx,		/* Register used by each index.  0 for unused indices */
 			 int useSeekResult,	/* True to set the USESEEKRESULT flag on OP_[Idx]Insert */
-			 u8 onError)
+			 u8 onError,
+			 int optimization_availability)
 {
 	Vdbe *v;		/* Prepared statements under construction */
 	Index *pIdx;		/* An index being inserted or updated */
 	u16 pik_flags;		/* flag values passed to the btree insert */
 	int opcode;
 
+	assert(optimization_availability == ON_CONFLICT_ACTION_NONE ||
+	       optimization_availability == ON_CONFLICT_ACTION_REPLACE ||
+	       optimization_availability == ON_CONFLICT_ACTION_IGNORE);
+
 	v = sqlite3GetVdbe(pParse);
 	assert(v != 0);
 	/* This table is not a VIEW */
@@ -1508,13 +1528,24 @@ sqlite3CompleteInsertion(Parse * pParse,	/* The parser context */
 	}
 	assert(pParse->nested == 0);
 
-	if (onError == ON_CONFLICT_ACTION_REPLACE) {
+	/**
+	 * onError represents override error in queries like
+	 * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE.
+	 * when error action is strictly specified by user and
+	 * therefore should have been used even when optimization
+	 * is possible (uniqueness can be checked by Tarantool).
+	 */
+	if (onError == ON_CONFLICT_ACTION_REPLACE ||
+	    (optimization_availability == ON_CONFLICT_ACTION_REPLACE &&
+	     onError == ON_CONFLICT_ACTION_DEFAULT)) {
 		opcode = OP_IdxReplace;
 	} else {
 		opcode = OP_IdxInsert;
 	}
 
-	if (onError == ON_CONFLICT_ACTION_IGNORE) {
+	if (onError == ON_CONFLICT_ACTION_IGNORE ||
+	    (optimization_availability == ON_CONFLICT_ACTION_IGNORE &&
+	     onError == ON_CONFLICT_ACTION_DEFAULT)) {
 		pik_flags |= OPFLAG_OE_IGNORE;
 	} else if (onError == ON_CONFLICT_ACTION_FAIL) {
 		pik_flags |= OPFLAG_OE_FAIL;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index e9c0b3195..cc90be537 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3702,8 +3702,8 @@ void sqlite3GenerateRowIndexDelete(Parse *, Table *, int, int);
 int sqlite3GenerateIndexKey(Parse *, Index *, int, int, int *, Index *, int);
 void sqlite3ResolvePartIdxLabel(Parse *, int);
 void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
-				     int, u8, u8, int, int *, int *);
-void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int, u8);
+				     int, u8, u8, int, int *, int *, int *);
+void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int, u8, int);
 int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
 			       int *, u8, u8);
 void sqlite3BeginWriteOperation(Parse *, int);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index bf413252d..15e02add8 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -562,6 +562,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	if (!isView) {
 		int addr1 = 0;	/* Address of jump instruction */
 		int bReplace = 0;	/* True if REPLACE conflict resolution might happen */
+		int optimization_availability = ON_CONFLICT_ACTION_NONE;
 
 		/* Do constraint checks. */
 		assert(regOldPk > 0);
@@ -569,7 +570,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 						iIdxCur, regNewPk,
 						regOldPk, chngPk, onError,
 						labelContinue, &bReplace,
-						aXRef);
+						aXRef,
+						&optimization_availability);
 
 		/* Do FK constraint checks. */
 		if (hasFK) {
@@ -616,7 +618,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		}
 
 		/* Insert the new index entries and the new record. */
-		sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, 0, onError);
+		sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, 0,
+					 onError, optimization_availability);
 
 		/* Do any ON CASCADE, SET NULL or SET DEFAULT operations required to
 		 * handle rows (possibly in other tables) that refer via a foreign key
diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result
index b011a6e6a..5e892eada 100644
--- a/test/sql/on-conflict.result
+++ b/test/sql/on-conflict.result
@@ -50,6 +50,32 @@ box.sql.execute("SELECT * FROM e")
   - [2, 2]
   - [3, 1]
 ...
+box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)")
+---
+...
+box.sql.execute("INSERT INTO t1 VALUES (9)")
+---
+...
+box.sql.execute("INSERT INTO t1 VALUES (9)")
+---
+...
+box.sql.execute("SELECT * FROM t1")
+---
+- - [9]
+...
+box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY ON CONFLICT IGNORE)")
+---
+...
+box.sql.execute("INSERT INTO t2 VALUES (9)")
+---
+...
+box.sql.execute("INSERT INTO t2 VALUES (9)")
+---
+...
+box.sql.execute("SELECT * FROM t2")
+---
+- - [9]
+...
 box.sql.execute('DROP TABLE t')
 ---
 ...
@@ -62,3 +88,9 @@ box.sql.execute('DROP TABLE p')
 box.sql.execute('DROP TABLE e')
 ---
 ...
+box.sql.execute('DROP TABLE t1')
+---
+...
+box.sql.execute('DROP TABLE t2')
+---
+...
diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua
index dbf572a8b..1ff199d32 100644
--- a/test/sql/on-conflict.test.lua
+++ b/test/sql/on-conflict.test.lua
@@ -19,7 +19,20 @@ box.sql.execute("SELECT * FROM p")
 box.sql.execute("INSERT INTO e values (1, 1), (2, 2), (3, 1)")
 box.sql.execute("SELECT * FROM e")
 
+box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)")
+box.sql.execute("INSERT INTO t1 VALUES (9)")
+box.sql.execute("INSERT INTO t1 VALUES (9)")
+box.sql.execute("SELECT * FROM t1")
+
+box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY ON CONFLICT IGNORE)")
+box.sql.execute("INSERT INTO t2 VALUES (9)")
+box.sql.execute("INSERT INTO t2 VALUES (9)")
+
+box.sql.execute("SELECT * FROM t2")
+
 box.sql.execute('DROP TABLE t')
 box.sql.execute('DROP TABLE q')
 box.sql.execute('DROP TABLE p')
 box.sql.execute('DROP TABLE e')
+box.sql.execute('DROP TABLE t1')
+box.sql.execute('DROP TABLE t2')
-- 
2.14.1