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

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

[Bulat Niatshin]

In some cases ON CONFLICT REPLACE/IGNORE can be optimized. If
following conditions are true:
1) Space has PRIMARY KEY index only.
2) There are no foreign keys to other spaces.
3) There are no delete triggers to be fired if conflict happens.
4) The errt or action is REPLACE or IGNORE.

Then uniqueness can be ensured by Tarantool only, without VDBE
bytecode. This patch contains a small fix for that +
related code was refactored a little bit and necessary comments
were added.

Closes #3293
---

Branch:
https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt

Issue:
https://github.com/tarantool/tarantool/issues/3293

 src/box/sql/insert.c          | 144 ++++++++++++++++++++++++++++++++----------
 src/box/sql/sqliteInt.h       |  27 +++++++-
 src/box/sql/update.c          |   9 ++-
 test/sql/on-conflict.result   |  32 ++++++++++
 test/sql/on-conflict.test.lua |  13 ++++
 5 files changed, 186 insertions(+), 39 deletions(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 0e305d34b..fc9b121b0 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -835,9 +835,13 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		 */
 		int isReplace;	/* Set to true if constraints may cause a replace */
 		int bUseSeek;	/* True to use OPFLAG_SEEKRESULT */
+
+		struct on_conflict on_conflict;
+		on_conflict.override_error = onError;
+		on_conflict.optimized_on_conflict = ON_CONFLICT_ACTION_NONE;
 		sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
 						iIdxCur, regIns, 0,
-						ipkColumn >= 0, onError,
+						ipkColumn >= 0, &on_conflict,
 						endOfLoop, &isReplace, 0);
 		sqlite3FkCheck(pParse, pTab, 0, regIns, 0);
 
@@ -855,7 +859,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 						SQLITE_ForeignKeys) == 0 ||
 					       sqlite3FkReferences(pTab) == 0));
 		sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx,
-					 bUseSeek, onError);
+					 bUseSeek, &on_conflict);
 	}
 
 	/* Update the count of rows that are inserted
@@ -1056,7 +1060,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 				int regNewData,		/* First register in a range holding values to insert */
 				int regOldData,		/* Previous content.  0 for INSERTs */
 				u8 pkChng,		/* Non-zero if the PRIMARY KEY changed */
-				u8 overrideError,	/* Override onError to this if not ON_CONFLICT_ACTION_DEFAULT */
+				struct on_conflict *on_conflict,
 				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 */
@@ -1075,6 +1079,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 	u8 isUpdate;		/* True if this is an UPDATE operation */
 	u8 bAffinityDone = 0;	/* True if the OP_Affinity operation has been run */
 	struct session *user_session = current_session();
+	int override_error = on_conflict->override_error;
 
 	isUpdate = regOldData != 0;
 	db = pParse->db;
@@ -1107,8 +1112,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			continue;	/* This column is allowed to be NULL */
 
 		onError = table_column_nullable_action(pTab, i);
-		if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
-			onError = overrideError;
+		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
+			onError = override_error;
 		} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
 			onError = ON_CONFLICT_ACTION_ABORT;
 		}
@@ -1166,9 +1171,12 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			     SQLITE_IgnoreChecks) == 0) {
 		ExprList *pCheck = pTab->pCheck;
 		pParse->ckBase = regNewData + 1;
-		onError =
-		    overrideError != ON_CONFLICT_ACTION_DEFAULT ? overrideError
-			: ON_CONFLICT_ACTION_ABORT;
+
+		if (override_error != ON_CONFLICT_ACTION_DEFAULT)
+			onError = override_error;
+		else
+			onError = ON_CONFLICT_ACTION_ABORT;
+
 		for (i = 0; i < pCheck->nExpr; i++) {
 			int allOk;
 			Expr *pExpr = pCheck->a[i].pExpr;
@@ -1210,10 +1218,15 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 		int addrUniqueOk;	/* Jump here if the UNIQUE constraint is satisfied */
 		bool uniqueByteCodeNeeded = false;
 
+		/*
+		 * ABORT and DEFAULT error actions can be handled
+		 * by Tarantool only without emitting VDBE
+		 * bytecode.
+		 */
 		if ((pIdx->onError != ON_CONFLICT_ACTION_ABORT &&
 		     pIdx->onError != ON_CONFLICT_ACTION_DEFAULT) ||
-		    (overrideError != ON_CONFLICT_ACTION_ABORT &&
-		     overrideError != ON_CONFLICT_ACTION_DEFAULT)) {
+		    (override_error != ON_CONFLICT_ACTION_ABORT &&
+		     override_error != ON_CONFLICT_ACTION_DEFAULT)) {
 			uniqueByteCodeNeeded = true;
 		}
 
@@ -1318,41 +1331,73 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			continue;
 		}
 
-		/* Find out what action to take in case there is a uniqueness conflict */
-		onError = pIdx->onError;
-		if (onError == ON_CONFLICT_ACTION_NONE) {
+		if (!IsUniqueIndex(pIdx)) {
 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
-			continue;	/* pIdx is not a UNIQUE index */
+			continue;
 		}
-		/* If pIdx is not a UNIQUE or we are doing INSERT OR IGNORE,
-		 * INSERT OR FAIL then skip uniqueness checks and let it to be
-		 * done by Tarantool.
+
+		/*
+		 * Find out what action to take in case there is
+		 * a uniqueness conflict.
+		 */
+		onError = pIdx->onError;
+
+		/*
+		 * If we are doing INSERT OR IGNORE,
+		 * INSERT OR REPLACE then uniqueness can be
+		 * ensured by Tarantool only without bytecode,
+		 * because IGNORE/REPLACE error action will be
+		 * used for all indexes.
 		 */
-		if (overrideError == ON_CONFLICT_ACTION_FAIL ||
-		    overrideError == ON_CONFLICT_ACTION_IGNORE ||
-		    overrideError == ON_CONFLICT_ACTION_ABORT) {
+		if (override_error == ON_CONFLICT_ACTION_FAIL ||
+		    override_error == ON_CONFLICT_ACTION_IGNORE ||
+		    override_error == ON_CONFLICT_ACTION_ABORT) {
 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
 			continue;	/* pIdx is not a UNIQUE index */
 		}
-		if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
-			onError = overrideError;
+
+		/*
+		 * override_error is an action which is directly
+		 * specified by user in queries like
+		 * INSERT OR <override_error> and therefore
+		 * should have higher priority than indexes
+		 * error actions.
+		 */
+		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
+			onError = override_error;
 		} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
 			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)))
 		    ) {
+			/*
+			 * In that case assign optimized error
+			 * action as IGNORE or REPLACE.
+			 */
+			on_conflict->optimized_on_conflict = onError;
+
 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
 			continue;
 		}
@@ -1425,7 +1470,19 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			}
 		}
 
-		/* Generate code that executes if the new index entry is not unique */
+		/*
+		 * Generate bytecode which executes when entry is
+		 * not unique for constraints with following
+		 * error actions:
+		 * 1) ROLLBACK.
+		 * 2) FAIL.
+		 * 3) IGNORE.
+		 * 4) REPLACE.
+		 *
+		 * ON CONFLICT ABORT is a default error action
+		 * for constraints and therefore can be handled
+		 * by Tarantool only.
+		 */
 		assert(onError == ON_CONFLICT_ACTION_ROLLBACK
 		       || onError == ON_CONFLICT_ACTION_ABORT
 		       || onError == ON_CONFLICT_ACTION_FAIL
@@ -1490,12 +1547,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)
+			 struct on_conflict *on_conflict)
 {
 	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;
+	int override_error = on_conflict->override_error;
+	int optimized_error_action = on_conflict->optimized_on_conflict;
+
+	assert(optimized_error_action == ON_CONFLICT_ACTION_NONE ||
+	       optimized_error_action == ON_CONFLICT_ACTION_REPLACE ||
+	       optimized_error_action == ON_CONFLICT_ACTION_IGNORE);
 
 	v = sqlite3GetVdbe(pParse);
 	assert(v != 0);
@@ -1524,15 +1587,26 @@ sqlite3CompleteInsertion(Parse * pParse,	/* The parser context */
 	}
 	assert(pParse->nested == 0);
 
-	if (onError == ON_CONFLICT_ACTION_REPLACE) {
+	/*
+	 * override_error represents error action in queries like
+	 * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE,
+	 * which is strictly specified by user and therefore
+	 * should have been used even when optimization is
+	 * possible (uniqueness can be checked by Tarantool).
+	 */
+	if (override_error == ON_CONFLICT_ACTION_REPLACE ||
+	    (optimized_error_action == ON_CONFLICT_ACTION_REPLACE &&
+	     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
 		opcode = OP_IdxReplace;
 	} else {
 		opcode = OP_IdxInsert;
 	}
 
-	if (onError == ON_CONFLICT_ACTION_IGNORE) {
+	if (override_error == ON_CONFLICT_ACTION_IGNORE ||
+	    (optimized_error_action == ON_CONFLICT_ACTION_IGNORE &&
+	     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
 		pik_flags |= OPFLAG_OE_IGNORE;
-	} else if (onError == ON_CONFLICT_ACTION_FAIL) {
+	} else if (override_error == ON_CONFLICT_ACTION_FAIL) {
 		pik_flags |= OPFLAG_OE_FAIL;
 	}
 
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index dc045f97b..5492ca44c 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -666,6 +666,27 @@ enum sql_type {
 	SQLITE_NULL = 5,
 };
 
+/**
+ * Structure for internal usage during INSERT/UPDATE
+ * statements compilation.
+ */
+struct on_conflict {
+	/**
+	 * Represents an error action in queries like
+	 * INSERT/UPDATE OR <override_error>, which
+	 * overrides all space constraints error actions.
+	 */
+	int override_error;
+	/**
+	 * Represents an ON CONFLICT action which can be
+	 * optimized and executed without VDBE bytecode, by
+	 * Tarantool only. If optimization is not available, then
+	 * the value is ON_CONFLICT_ACTION_NONE, otherwise it is
+	 * ON_CONFLICT_ACTON_IGNORE or ON_CONFLICT_ACTION_REPLACE.
+	 */
+	int optimized_on_conflict;
+};
+
 void *
 sqlite3_user_data(sqlite3_context *);
 
@@ -3709,8 +3730,10 @@ 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, struct on_conflict *, int,
+				     int *, int *);
+void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int,
+			      struct on_conflict *);
 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..c888aab1a 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -563,11 +563,15 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		int addr1 = 0;	/* Address of jump instruction */
 		int bReplace = 0;	/* True if REPLACE conflict resolution might happen */
 
+		struct on_conflict on_conflict;
+		on_conflict.override_error = onError;
+		on_conflict.optimized_on_conflict = ON_CONFLICT_ACTION_NONE;
+
 		/* Do constraint checks. */
 		assert(regOldPk > 0);
 		sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
 						iIdxCur, regNewPk,
-						regOldPk, chngPk, onError,
+						regOldPk, chngPk, &on_conflict,
 						labelContinue, &bReplace,
 						aXRef);
 
@@ -616,7 +620,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,
+					 &on_conflict);
 
 		/* 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

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

[n.pettik]

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

Accidentally I have sent patch to outdate mailing list.
Resending it again to the new one.
Reply with fixes to this mail.

> On 3 Apr 2018, at 13:20, n.pettik <korablev@tarantool.org> wrote:
> 
> Hello. See comments below.
> 
>> On 30 Mar 2018, at 01:25, Bulat Niatshin <niatshin@tarantool.org <mailto:niatshin@tarantool.org>> wrote:
>> 
>> In some cases ON CONFLICT REPLACE/IGNORE can be optimized. If
>> following conditions are true:
>> 1) Space has PRIMARY KEY index only.
>> 2) There are no foreign keys to other spaces.
>> 3) There are no delete triggers to be fired if conflict happens.
>> 4) The errt or action is REPLACE or IGNORE.
> 
> errt: typo.
> 
>> 
>> Then uniqueness can be ensured by Tarantool only, without VDBE
>> bytecode. This patch contains a small fix for that +
> 
> Replace ‘+’ with word. Don’t mention that it is ’small’ fix, let it be just fix.
> 
>> related code was refactored a little bit and necessary comments
> 
> The same: don’t use ‘little bit’ — it sounds like you left smth
> in a shitty state consciously.
> 
>> were added.
>> 
>> Closes #3293
>> ---
>> 
>> Branch:
>> https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt
>> 
>> Issue:
>> https://github.com/tarantool/tarantool/issues/3293
>> 
>> src/box/sql/insert.c          | 144 ++++++++++++++++++++++++++++++++----------
>> src/box/sql/sqliteInt.h       |  27 +++++++-
>> src/box/sql/update.c          |   9 ++-
>> test/sql/on-conflict.result   |  32 ++++++++++
>> test/sql/on-conflict.test.lua |  13 ++++
>> 5 files changed, 186 insertions(+), 39 deletions(-)
>> 
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index 0e305d34b..fc9b121b0 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -835,9 +835,13 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
>> 		 */
>> 		int isReplace;	/* Set to true if constraints may cause a replace */
>> 		int bUseSeek;	/* True to use OPFLAG_SEEKRESULT */
>> +
>> +		struct on_conflict on_conflict;
>> +		on_conflict.override_error = onError;
>> +		on_conflict.optimized_on_conflict = ON_CONFLICT_ACTION_NONE;
>> 		sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
>> 						iIdxCur, regIns, 0,
>> -						ipkColumn >= 0, onError,
>> +						ipkColumn >= 0, &on_conflict,
>> 						endOfLoop, &isReplace, 0);
>> 		sqlite3FkCheck(pParse, pTab, 0, regIns, 0);
>> 
>> @@ -855,7 +859,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
>> 						SQLITE_ForeignKeys) == 0 ||
>> 					       sqlite3FkReferences(pTab) == 0));
>> 		sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx,
>> -					 bUseSeek, onError);
>> +					 bUseSeek, &on_conflict);
>> 	}
>> 
>> 	/* Update the count of rows that are inserted
>> @@ -1056,7 +1060,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>> 				int regNewData,		/* First register in a range holding values to insert */
>> 				int regOldData,		/* Previous content.  0 for INSERTs */
>> 				u8 pkChng,		/* Non-zero if the PRIMARY KEY changed */
>> -				u8 overrideError,	/* Override onError to this if not ON_CONFLICT_ACTION_DEFAULT */
>> +				struct on_conflict *on_conflict,
>> 				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 */
>> @@ -1075,6 +1079,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>> 	u8 isUpdate;		/* True if this is an UPDATE operation */
>> 	u8 bAffinityDone = 0;	/* True if the OP_Affinity operation has been run */
>> 	struct session *user_session = current_session();
>> +	int override_error = on_conflict->override_error;
>> 
>> 	isUpdate = regOldData != 0;
>> 	db = pParse->db;
>> @@ -1107,8 +1112,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>> 			continue;	/* This column is allowed to be NULL */
>> 
>> 		onError = table_column_nullable_action(pTab, i);
>> -		if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
>> -			onError = overrideError;
>> +		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
>> +			onError = override_error;
>> 		} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
>> 			onError = ON_CONFLICT_ACTION_ABORT;
>> 		}
>> @@ -1166,9 +1171,12 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>> 			     SQLITE_IgnoreChecks) == 0) {
>> 		ExprList *pCheck = pTab->pCheck;
>> 		pParse->ckBase = regNewData + 1;
>> -		onError =
>> -		    overrideError != ON_CONFLICT_ACTION_DEFAULT ? overrideError
>> -			: ON_CONFLICT_ACTION_ABORT;
>> +
>> +		if (override_error != ON_CONFLICT_ACTION_DEFAULT)
>> +			onError = override_error;
>> +		else
>> +			onError = ON_CONFLICT_ACTION_ABORT;
>> +
>> 		for (i = 0; i < pCheck->nExpr; i++) {
>> 			int allOk;
>> 			Expr *pExpr = pCheck->a[i].pExpr;
>> @@ -1210,10 +1218,15 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>> 		int addrUniqueOk;	/* Jump here if the UNIQUE constraint is satisfied */
>> 		bool uniqueByteCodeNeeded = false;
>> 
>> +		/*
>> +		 * ABORT and DEFAULT error actions can be handled
>> +		 * by Tarantool only without emitting VDBE
> 
> Remove word ‘only’: it confuses little bit (IMHO).
> 
>> +		 * bytecode.
>> +		 */
>> 		if ((pIdx->onError != ON_CONFLICT_ACTION_ABORT &&
>> 		     pIdx->onError != ON_CONFLICT_ACTION_DEFAULT) ||
>> -		    (overrideError != ON_CONFLICT_ACTION_ABORT &&
>> -		     overrideError != ON_CONFLICT_ACTION_DEFAULT)) {
>> +		    (override_error != ON_CONFLICT_ACTION_ABORT &&
>> +		     override_error != ON_CONFLICT_ACTION_DEFAULT)) {
>> 			uniqueByteCodeNeeded = true;
>> 		}
>> 
>> @@ -1318,41 +1331,73 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>> 			continue;
>> 		}
>> 
>> -		/* Find out what action to take in case there is a uniqueness conflict */
>> -		onError = pIdx->onError;
>> -		if (onError == ON_CONFLICT_ACTION_NONE) {
>> +		if (!IsUniqueIndex(pIdx)) {
>> 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
>> -			continue;	/* pIdx is not a UNIQUE index */
>> +			continue;
>> 		}
>> -		/* If pIdx is not a UNIQUE or we are doing INSERT OR IGNORE,
>> -		 * INSERT OR FAIL then skip uniqueness checks and let it to be
>> -		 * done by Tarantool.
>> +
>> +		/*
>> +		 * Find out what action to take in case there is
>> +		 * a uniqueness conflict.
>> +		 */
>> +		onError = pIdx->onError;
>> +
> 
> Nit-picking: don’t put too many empty lines.
> In this particular case, you outline very elementary statement.
> 
>> +		/*
>> +		 * If we are doing INSERT OR IGNORE,
>> +		 * INSERT OR REPLACE then uniqueness can be
>> +		 * ensured by Tarantool only without bytecode,
>> +		 * because IGNORE/REPLACE error action will be
>> +		 * used for all indexes.
>> 		 */
>> -		if (overrideError == ON_CONFLICT_ACTION_FAIL ||
>> -		    overrideError == ON_CONFLICT_ACTION_IGNORE ||
>> -		    overrideError == ON_CONFLICT_ACTION_ABORT) {
>> +		if (override_error == ON_CONFLICT_ACTION_FAIL ||
>> +		    override_error == ON_CONFLICT_ACTION_IGNORE ||
>> +		    override_error == ON_CONFLICT_ACTION_ABORT) {
>> 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
>> 			continue;	/* pIdx is not a UNIQUE index */
>> 		}
>> -		if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
>> -			onError = overrideError;
>> +
>> +		/*
>> +		 * override_error is an action which is directly
>> +		 * specified by user in queries like
>> +		 * INSERT OR <override_error> and therefore
>> +		 * should have higher priority than indexes
>> +		 * error actions.
>> +		 */
> 
> You put almost the same comment to on_error struct.
> Change it to more informative or remove.
> 
>> +		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
>> +			onError = override_error;
>> 		} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
>> 			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)))
>> 		    ) {
>> +			/*
>> +			 * In that case assign optimized error
>> +			 * action as IGNORE or REPLACE.
>> +			 */
>> +			on_conflict->optimized_on_conflict = onError;
>> +
>> 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
>> 			continue;
>> 		}
>> @@ -1425,7 +1470,19 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>> 			}
>> 		}
>> 
>> -		/* Generate code that executes if the new index entry is not unique */
>> +		/*
>> +		 * Generate bytecode which executes when entry is
>> +		 * not unique for constraints with following
>> +		 * error actions:
>> +		 * 1) ROLLBACK.
>> +		 * 2) FAIL.
>> +		 * 3) IGNORE.
>> +		 * 4) REPLACE.
>> +		 *
>> +		 * ON CONFLICT ABORT is a default error action
>> +		 * for constraints and therefore can be handled
>> +		 * by Tarantool only.
>> +		 */
>> 		assert(onError == ON_CONFLICT_ACTION_ROLLBACK
>> 		       || onError == ON_CONFLICT_ACTION_ABORT
>> 		       || onError == ON_CONFLICT_ACTION_FAIL
>> @@ -1490,12 +1547,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)
>> +			 struct on_conflict *on_conflict)
>> {
>> 	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;
>> +	int override_error = on_conflict->override_error;
>> +	int optimized_error_action = on_conflict->optimized_on_conflict;
> 
> If you used enum type, you wouldn’t have to add assertion below.
> 
>> +
>> +	assert(optimized_error_action == ON_CONFLICT_ACTION_NONE ||
>> +	       optimized_error_action == ON_CONFLICT_ACTION_REPLACE ||
>> +	       optimized_error_action == ON_CONFLICT_ACTION_IGNORE);
>> 
>> 	v = sqlite3GetVdbe(pParse);
>> 	assert(v != 0);
>> @@ -1524,15 +1587,26 @@ sqlite3CompleteInsertion(Parse * pParse,	/* The parser context */
>> 	}
>> 	assert(pParse->nested == 0);
>> 
>> -	if (onError == ON_CONFLICT_ACTION_REPLACE) {
>> +	/*
>> +	 * override_error represents error action in queries like
>> +	 * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE,
>> +	 * which is strictly specified by user and therefore
>> +	 * should have been used even when optimization is
>> +	 * possible (uniqueness can be checked by Tarantool).
>> +	 */
> 
> This comment is almost copy of one from struct declaration. 
> Remove or rephrase.
> 
>> +	if (override_error == ON_CONFLICT_ACTION_REPLACE ||
>> +	    (optimized_error_action == ON_CONFLICT_ACTION_REPLACE &&
>> +	     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
>> 		opcode = OP_IdxReplace;
>> 	} else {
>> 		opcode = OP_IdxInsert;
>> 	}
>> 
>> -	if (onError == ON_CONFLICT_ACTION_IGNORE) {
>> +	if (override_error == ON_CONFLICT_ACTION_IGNORE ||
>> +	    (optimized_error_action == ON_CONFLICT_ACTION_IGNORE &&
>> +	     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
>> 		pik_flags |= OPFLAG_OE_IGNORE;
>> -	} else if (onError == ON_CONFLICT_ACTION_FAIL) {
>> +	} else if (override_error == ON_CONFLICT_ACTION_FAIL) {
>> 		pik_flags |= OPFLAG_OE_FAIL;
>> 	}
>> 
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index dc045f97b..5492ca44c 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -666,6 +666,27 @@ enum sql_type {
>> 	SQLITE_NULL = 5,
>> };
>> 
>> +/**
>> + * Structure for internal usage during INSERT/UPDATE
>> + * statements compilation.
>> + */
>> +struct on_conflict {
>> +	/**
>> +	 * Represents an error action in queries like
>> +	 * INSERT/UPDATE OR <override_error>, which
>> +	 * overrides all space constraints error actions.
>> +	 */
>> +	int override_error;
> 
> Why do you declare type of this field as int,
> when it can take values from on_conflict_action enum?
> 
>> +	/**
>> +	 * Represents an ON CONFLICT action which can be
>> +	 * optimized and executed without VDBE bytecode, by
>> +	 * Tarantool only. If optimization is not available, then
> 
> "Only by Tarantool facilities" would sound better, wouldn’t it?
> 
>> +	 * the value is ON_CONFLICT_ACTION_NONE, otherwise it is
>> +	 * ON_CONFLICT_ACTON_IGNORE or ON_CONFLICT_ACTION_REPLACE.
>> +	 */
>> +	int optimized_on_conflict;
> 
> Your structure is already called ‘on_conflict’,
> so don’t repeat this phrase in field’s name.
> Also, the same as previous field: better declare it as enum.
> 
>> +};
>> +
>> void *
>> sqlite3_user_data(sqlite3_context *);
>> 
>> @@ -3709,8 +3730,10 @@ 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, struct on_conflict *, int,
>> +				     int *, int *);
>> +void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int,
>> +			      struct on_conflict *);
>> 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..c888aab1a 100644
>> --- a/src/box/sql/update.c
>> +++ b/src/box/sql/update.c
>> @@ -563,11 +563,15 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>> 		int addr1 = 0;	/* Address of jump instruction */
>> 		int bReplace = 0;	/* True if REPLACE conflict resolution might happen */
>> 
>> +		struct on_conflict on_conflict;
>> +		on_conflict.override_error = onError;
>> +		on_conflict.optimized_on_conflict = ON_CONFLICT_ACTION_NONE;
>> +
>> 		/* Do constraint checks. */
>> 		assert(regOldPk > 0);
>> 		sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
>> 						iIdxCur, regNewPk,
>> -						regOldPk, chngPk, onError,
>> +						regOldPk, chngPk, &on_conflict,
>> 						labelContinue, &bReplace,
>> 						aXRef);
>> 
>> @@ -616,7 +620,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,
>> +					 &on_conflict);
>> 
>> 		/* 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


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

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

[n.pettik]

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

Consider formatting: it is almost unreadable. Don’t add extra tabs to quote.
Also, I don’t see provided changes: you should attach them to your letter,
send in reply to this letter or send patch ver.2.
I can’t give you any review, just because there is no subject of it.

Please, rebase on fresh 2.0 and send diff or whole patch again.
Thanks.

> On 4 Apr 2018, at 11:51, Bulat Niatshin <niatshin@tarantool.org> wrote:
> 
> 
> Branch:
> https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt <https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt> 
> 
> Issue:
> https://github.com/tarantool/tarantool/issues/3293 <https://github.com/tarantool/tarantool/issues/3293> 
> 
> In some cases ON CONFLICT REPLACE/IGNORE can be optimized. If
> following conditions are true:
> 1) Space has PRIMARY KEY index only.
> 2) There are no foreign keys to other spaces.
> 3) There are no delete triggers to be fired if conflict happens.
> 4) The errt or action is REPLACE or IGNORE.
> 
> errt: typo.
> 
> Done.
> 
> Then uniqueness can be ensured by Tarantool only, without VDBE
> bytecode. This patch contains a small fix for that +
> 
> Replace ‘+’ with word. Don’t mention that it is ’small’ fix, let it be just fix.
> 
> Done.
> 
> related code was refactored a little bit and necessary comments
> 
> The same: don’t use ‘little bit’ — it sounds like you left smth
> in a shitty state consciously.
> 
> Done.
> 
> +/*
> + * ABORT and DEFAULT error actions can be handled
> + * by Tarantool only without emitting VDBE
> 
> Remove word ‘only’: it confuses little bit (IMHO).
> Done.
> 
> +
> +/*
> + * Find out what action to take in case there is
> + * a uniqueness conflict.
> + */
> +onError = pIdx->onError;
> +
> 
> Nit-picking: don’t put too many empty lines.
> In this particular case, you outline very elementary statement.
> 
> Done.
> 
> +/*
> + * override_error is an action which is directly
> + * specified by user in queries like
> + * INSERT OR <override_error> and therefore
> + * should have higher priority than indexes
> + * error actions.
> + */
> 
> You put almost the same comment to on_error struct.
> Change it to more informative or remove.
> 
> Done, this comment was removed.
> 
> +int override_error = on_conflict->override_error;
> +int optimized_error_action = on_conflict->optimized_on_conflict;
> 
> If you used enum type, you wouldn’t have to add assertion below.
> 
> Done, all error actions are enums right now.
> 
> +/*
> + * override_error represents error action in queries like
> + * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE,
> + * which is strictly specified by user and therefore
> + * should have been used even when optimization is
> + * possible (uniqueness can be checked by Tarantool).
> + */
> 
> This comment is almost copy of one from struct declaration. 
> Remove or rephrase.
> 
> Done, removed.
> 
> +struct on_conflict {
> +/**
> + * Represents an error action in queries like
> + * INSERT/UPDATE OR <override_error>, which
> + * overrides all space constraints error actions.
> + */
> +int override_error;
> 
> Why do you declare type of this field as int,
> when it can take values from on_conflict_action enum?
> 
> Done.
> 
> + * the value is ON_CONFLICT_ACTION_NONE, otherwise it is
> + * ON_CONFLICT_ACTON_IGNORE or ON_CONFLICT_ACTION_REPLACE.
> + */
> +int optimized_on_conflict;
> 
> Your structure is already called ‘on_conflict’,
> so don’t repeat this phrase in field’s name.
> Also, the same as previous field: better declare it as enum.
> 
> Done.
> 
> 
> 
> 
> 
> 
> -- 
> Bulat Niatshin


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