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

[tarantool-patches] [PATCH V2] 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 error action is REPLACE or IGNORE.

Then uniqueness can be ensured by Tarantool facilities, without
VDBE bytecode. This patch contains fix for that, in addition
related code was refactored 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

Changes from V1:
- Fixed remarks as per review by Nikita
- Refactored related code, necessary comments
  were added

 src/box/sql/insert.c          | 201 ++++++++++++++++++++++++++----------------
 src/box/sql/sqliteInt.h       |  35 +++++++-
 src/box/sql/update.c          |  26 +++---
 test/sql/on-conflict.result   |  32 +++++++
 test/sql/on-conflict.test.lua |  13 +++
 5 files changed, 217 insertions(+), 90 deletions(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b24d55b07..2c4391f4e 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -317,7 +317,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 	      SrcList * pTabList,	/* Name of table into which we are inserting */
 	      Select * pSelect,	/* A SELECT statement to use as the data source */
 	      IdList * pColumn,	/* Column names corresponding to IDLIST. */
-	      int onError)	/* How to handle constraint errors */
+	      enum on_conflict_action on_error)
 {
 	sqlite3 *db;		/* The main database structure */
 	Table *pTab;		/* The table to insert into.  aka TABLE */
@@ -434,7 +434,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 	 *
 	 * This is the 2nd template.
 	 */
-	if (pColumn == 0 && xferOptimization(pParse, pTab, pSelect, onError)) {
+	if (pColumn == 0 && xferOptimization(pParse, pTab, pSelect, on_error)) {
 		assert(!pTrigger);
 		assert(pList == 0);
 		goto insert_end;
@@ -621,7 +621,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		nIdx =
 		    sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0,
 					       -1, 0, &iDataCur, &iIdxCur,
-					       onError, 0);
+					       on_error, 0);
 
 		aRegIdx = sqlite3DbMallocRawNN(db, sizeof(int) * (nIdx + 1));
 		if (aRegIdx == 0) {
@@ -717,7 +717,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		/* Fire BEFORE or INSTEAD OF triggers */
 		sqlite3CodeRowTrigger(pParse, pTrigger, TK_INSERT, 0,
 				      TRIGGER_BEFORE, pTab,
-				      regCols - pTab->nCol - 1, onError,
+				      regCols - pTab->nCol - 1, on_error,
 				      endOfLoop);
 
 		sqlite3ReleaseTempRange(pParse, regCols, pTab->nCol + 1);
@@ -846,12 +846,16 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		   and do the insertion.
 		 */
 		int isReplace;	/* Set to true if constraints may cause a replace */
+		struct on_conflict on_conflict;
+		on_conflict.override_error = on_error;
+		on_conflict.optimized_action = 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);
-		vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], onError);
+		vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0],
+					       &on_conflict);
 	}
 
 	/* Update the count of rows that are inserted
@@ -864,7 +868,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		/* Code AFTER triggers */
 		sqlite3CodeRowTrigger(pParse, pTrigger, TK_INSERT, 0,
 				      TRIGGER_AFTER, pTab,
-				      regData - 2 - pTab->nCol, onError,
+				      regData - 2 - pTab->nCol, on_error,
 				      endOfLoop);
 	}
 
@@ -1038,10 +1042,12 @@ checkConstraintUnchanged(Expr * pExpr, int *aiChng)
  *
  *  CHECK            REPLACE      Illegal.  The results in an exception.
  *
- * Which action to take is determined by the overrideError parameter.
- * Or if overrideError==ON_CONFLICT_ACTION_DEFAULT, then the pParse->onError parameter
- * is used.  Or if pParse->onError==ON_CONFLICT_ACTION_DEFAULT then the onError value
- * for the constraint is used.
+ * Which action to take is determined by the override_error parameter
+ * in struct on_conflict.
+ * Or if override_error==ON_CONFLICT_ACTION_DEFAULT, then the
+ * pParse->onError parameter is used.  Or if
+ * pParse->onError==ON_CONFLICT_ACTION_DEFAULT then the on_error
+ * value for the constraint is used.
  */
 void
 sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
@@ -1052,7 +1058,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 */
@@ -1064,13 +1070,14 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 	int i;			/* loop counter */
 	int ix;			/* Index loop counter */
 	int nCol;		/* Number of columns */
-	int onError;		/* Conflict resolution strategy */
 	int addr1;		/* Address of jump instruction */
 	int seenReplace = 0;	/* True if REPLACE is used to resolve INT PK conflict */
 	int nPkField;		/* Number of fields in PRIMARY KEY. */
 	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();
+	enum on_conflict_action override_error = on_conflict->override_error;
+	enum on_conflict_action on_error;
 
 	isUpdate = regOldData != 0;
 	db = pParse->db;
@@ -1102,25 +1109,20 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			&& pTab->iAutoIncPKey == i))
 			continue;	/* This column is allowed to be NULL */
 
-		onError = table_column_nullable_action(pTab, i);
-		if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
-			onError = overrideError;
-		} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
-			onError = ON_CONFLICT_ACTION_ABORT;
+		on_error = table_column_nullable_action(pTab, i);
+		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
+			on_error = override_error;
+		} else if (on_error == ON_CONFLICT_ACTION_DEFAULT) {
+			on_error = ON_CONFLICT_ACTION_ABORT;
 		}
 		struct Expr *dflt = NULL;
 		dflt = space_column_default_expr(
 			SQLITE_PAGENO_TO_SPACEID(pTab->tnum),
 			i);
-		if (onError == ON_CONFLICT_ACTION_REPLACE && dflt == 0)
-			onError = ON_CONFLICT_ACTION_ABORT;
+		if (on_error == ON_CONFLICT_ACTION_REPLACE && dflt == 0)
+			on_error = ON_CONFLICT_ACTION_ABORT;
 
-		assert(onError == ON_CONFLICT_ACTION_ROLLBACK
-		       || onError == ON_CONFLICT_ACTION_ABORT
-		       || onError == ON_CONFLICT_ACTION_FAIL
-		       || onError == ON_CONFLICT_ACTION_IGNORE
-		       || onError == ON_CONFLICT_ACTION_REPLACE);
-		switch (onError) {
+		switch (on_error) {
 		case ON_CONFLICT_ACTION_ABORT:
 			sqlite3MayAbort(pParse);
 			/* Fall through */
@@ -1131,7 +1133,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 						   pTab->aCol[i].zName);
 				sqlite3VdbeAddOp3(v, OP_HaltIfNull,
 						  SQLITE_CONSTRAINT_NOTNULL,
-						  onError, regNewData + 1 + i);
+						  on_error, regNewData + 1 + i);
 				sqlite3VdbeAppendP4(v, zMsg, P4_DYNAMIC);
 				sqlite3VdbeChangeP5(v, P5_ConstraintNotNull);
 				VdbeCoverage(v);
@@ -1145,7 +1147,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 				break;
 			}
 		default:{
-				assert(onError == ON_CONFLICT_ACTION_REPLACE);
+				assert(on_error == ON_CONFLICT_ACTION_REPLACE);
 				addr1 =
 				    sqlite3VdbeAddOp1(v, OP_NotNull,
 						      regNewData + 1 + i);
@@ -1165,9 +1167,11 @@ 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)
+			on_error = override_error;
+		else
+			on_error = ON_CONFLICT_ACTION_ABORT;
+
 		for (i = 0; i < pCheck->nExpr; i++) {
 			int allOk;
 			Expr *pExpr = pCheck->a[i].pExpr;
@@ -1177,17 +1181,17 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			allOk = sqlite3VdbeMakeLabel(v);
 			sqlite3ExprIfTrue(pParse, pExpr, allOk,
 					  SQLITE_JUMPIFNULL);
-			if (onError == ON_CONFLICT_ACTION_IGNORE) {
+			if (on_error == ON_CONFLICT_ACTION_IGNORE) {
 				sqlite3VdbeGoto(v, ignoreDest);
 			} else {
 				char *zName = pCheck->a[i].zName;
 				if (zName == 0)
 					zName = pTab->zName;
-				if (onError == ON_CONFLICT_ACTION_REPLACE)
-					onError = ON_CONFLICT_ACTION_ABORT;
+				if (on_error == ON_CONFLICT_ACTION_REPLACE)
+					on_error = ON_CONFLICT_ACTION_ABORT;
 				sqlite3HaltConstraint(pParse,
 						      SQLITE_CONSTRAINT_CHECK,
-						      onError, zName,
+						      on_error, zName,
 						      P4_TRANSIENT,
 						      P5_ConstraintCheck);
 			}
@@ -1209,10 +1213,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 facilitites 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;
 		}
 
@@ -1316,42 +1325,61 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
 			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.
+
+		on_error = pIdx->onError;
+		/*
+		 * If we are doing INSERT OR IGNORE,
+		 * INSERT OR FAIL, then error action will
+		 * be the same for all space indexes and
+		 * uniqueness can be ensured by Tarantool.
 		 */
-		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 */
+			continue;
 		}
-		if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
-			onError = overrideError;
-		} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
-			onError = ON_CONFLICT_ACTION_ABORT;
+
+		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
+			on_error = override_error;
+		} else if (on_error == ON_CONFLICT_ACTION_DEFAULT) {
+			on_error = 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 */
+		    && (on_error == ON_CONFLICT_ACTION_REPLACE ||
+			on_error == 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)))
 		    ) {
+			/*
+			 * Save that possible optimized error
+			 * action, which can be used later
+			 * during execution of
+			 * vdbe_emit_insertion_completion().
+			 */
+			on_conflict->optimized_action = on_error;
 			sqlite3VdbeResolveLabel(v, addrUniqueOk);
 			continue;
 		}
@@ -1373,7 +1401,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 		regR =
 		    (pIdx == pPk) ? regIdx : sqlite3GetTempRange(pParse,
 								 nPkField);
-		if (isUpdate || onError == ON_CONFLICT_ACTION_REPLACE) {
+		if (isUpdate || on_error == ON_CONFLICT_ACTION_REPLACE) {
 			int x;
 			int nPkCol = index_column_count(pPk);
 			/* Extract the PRIMARY KEY from the end of the index entry and
@@ -1423,16 +1451,23 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			}
 		}
 
-		/* Generate code that executes if the new index entry is not unique */
-		assert(onError == ON_CONFLICT_ACTION_ROLLBACK
-		       || onError == ON_CONFLICT_ACTION_ABORT
-		       || onError == ON_CONFLICT_ACTION_FAIL
-		       || onError == ON_CONFLICT_ACTION_IGNORE
-		       || onError == ON_CONFLICT_ACTION_REPLACE);
-		switch (onError) {
+		/*
+		 * 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 facilities.
+		 */
+		switch (on_error) {
 		case ON_CONFLICT_ACTION_FAIL:
 		case ON_CONFLICT_ACTION_ROLLBACK: {
-				sqlite3UniqueConstraint(pParse, onError, pIdx);
+				sqlite3UniqueConstraint(pParse, on_error, pIdx);
 				break;
 			}
 		case ON_CONFLICT_ACTION_ABORT: {
@@ -1444,7 +1479,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			}
 		default: {
 				Trigger *pTrigger = 0;
-				assert(onError == ON_CONFLICT_ACTION_REPLACE);
+				assert(on_error == ON_CONFLICT_ACTION_REPLACE);
 				sql_set_multi_write(pParse, true);
 				if (user_session->
 				    sql_flags & SQLITE_RecTriggers) {
@@ -1474,20 +1509,34 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 }
 
 void
-vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, u8 on_error)
+vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
+			       struct on_conflict *on_conflict)
 {
 	assert(v != NULL);
 	int opcode;
-	if (on_error == ON_CONFLICT_ACTION_REPLACE)
+	enum on_conflict_action override_error = on_conflict->override_error;
+	enum on_conflict_action optimized_action =
+						on_conflict->optimized_action;
+
+	if (override_error == ON_CONFLICT_ACTION_REPLACE ||
+	    (optimized_action == ON_CONFLICT_ACTION_REPLACE &&
+	     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
 		opcode = OP_IdxReplace;
-	else
+	} else {
 		opcode = OP_IdxInsert;
+	}
 
 	u16 pik_flags = OPFLAG_NCHANGE;
-	if (on_error == ON_CONFLICT_ACTION_IGNORE)
+	if (override_error == ON_CONFLICT_ACTION_IGNORE)
+		pik_flags |= OPFLAG_OE_IGNORE;
+	if (override_error == ON_CONFLICT_ACTION_IGNORE ||
+	    (optimized_action == ON_CONFLICT_ACTION_IGNORE &&
+	     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
 		pik_flags |= OPFLAG_OE_IGNORE;
-	else if (on_error == ON_CONFLICT_ACTION_FAIL)
+	}
+	else if (override_error == ON_CONFLICT_ACTION_FAIL) {
 		pik_flags |= OPFLAG_OE_FAIL;
+	}
 
 	sqlite3VdbeAddOp2(v, opcode, cursor_id, tuple_id);
 	sqlite3VdbeChangeP5(v, pik_flags);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 59662cf14..ea4f4dce6 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -682,6 +682,30 @@ 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.
+	 * That kind of error action is strictly specified by
+	 * user and therefore have highest priority.
+	 */
+	enum on_conflict_action override_error;
+	/**
+	 * Represents an ON CONFLICT action which can be
+	 * optimized and executed without VDBE bytecode, by
+	 * Tarantool facilities. 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.
+	 */
+	enum on_conflict_action optimized_action;
+};
+
 void *
 sqlite3_user_data(sqlite3_context *);
 
@@ -3580,7 +3604,8 @@ int sqlite3ViewGetColumnNames(Parse *, Table *);
 void
 sql_drop_table(struct Parse *, struct SrcList *, bool, bool);
 void sqlite3DeleteTable(sqlite3 *, Table *);
-void sqlite3Insert(Parse *, SrcList *, Select *, IdList *, int);
+void sqlite3Insert(Parse *, SrcList *, Select *, IdList *,
+		   enum on_conflict_action);
 void *sqlite3ArrayAllocate(sqlite3 *, void *, int, int *, int *);
 IdList *sqlite3IdListAppend(sqlite3 *, IdList *, Token *);
 int sqlite3IdListIndex(IdList *, const char *);
@@ -3617,7 +3642,8 @@ Expr *sqlite3LimitWhere(Parse *, SrcList *, Expr *, ExprList *, Expr *, Expr *,
 #endif
 void sqlite3DeleteFrom(Parse *, SrcList *, Expr *);
 void sqlite3DeleteByKey(Parse *, char *, const char **, Expr **, int);
-void sqlite3Update(Parse *, SrcList *, ExprList *, Expr *, int);
+void sqlite3Update(Parse *, SrcList *, ExprList *, Expr *,
+		   enum on_conflict_action);
 WhereInfo *sqlite3WhereBegin(Parse *, SrcList *, Expr *, ExprList *, ExprList *,
 			     u16, int);
 void sqlite3WhereEnd(WhereInfo *);
@@ -3699,7 +3725,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 *);
+				     int, u8, struct on_conflict *, int, int *,
+				     int *);
 /**
  * This routine generates code to finish the INSERT or UPDATE
  * operation that was started by a prior call to
@@ -3711,7 +3738,7 @@ void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
  */
 void
 vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
-			       u8 on_error);
+			       struct on_conflict *on_conflict);
 
 int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
 			       int *, u8, u8);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 83c05ab48..f97242547 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -93,14 +93,14 @@ sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, int iReg)
  *
  *   UPDATE OR IGNORE table_wxyz SET a=b, c=d WHERE e<5 AND f NOT NULL;
  *          \_______/ \________/     \______/       \________________/
-*            onError   pTabList      pChanges             pWhere
+*            on_error   pTabList      pChanges             pWhere
  */
 void
 sqlite3Update(Parse * pParse,		/* The parser context */
 	      SrcList * pTabList,	/* The table in which we should change things */
 	      ExprList * pChanges,	/* Things to be changed */
 	      Expr * pWhere,		/* The WHERE clause.  May be null */
-	      int onError)		/* How to handle constraint errors */
+	      enum on_conflict_action on_error)
 {
 	int i, j;		/* Loop counters */
 	Table *pTab;		/* The table to be updated */
@@ -402,7 +402,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		 * action, then we need to open all indices because we might need
 		 * to be deleting some records.
 		 */
-		if (onError == ON_CONFLICT_ACTION_REPLACE) {
+		if (on_error == ON_CONFLICT_ACTION_REPLACE) {
 			memset(aToOpen, 1, nIdx + 1);
 		} else {
 			for (pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
@@ -419,7 +419,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 				aToOpen[aiCurOnePass[1] - iBaseCur] = 0;
 		}
 		sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0,
-					   iBaseCur, aToOpen, 0, 0, onError, 1);
+					   iBaseCur, aToOpen, 0, 0,
+					   on_error, 1);
 	}
 
 	/* Top of the update loop */
@@ -461,7 +462,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		oldmask |= sqlite3TriggerColmask(pParse,
 						 pTrigger, pChanges, 0,
 						 TRIGGER_BEFORE | TRIGGER_AFTER,
-						 pTab, onError);
+						 pTab, on_error);
 		for (i = 0; i < pTab->nCol; i++) {
 			if (oldmask == 0xffffffff
 			    || (i < 32 && (oldmask & MASKBIT32(i)) != 0)
@@ -492,7 +493,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	 */
 	newmask =
 	    sqlite3TriggerColmask(pParse, pTrigger, pChanges, 1, TRIGGER_BEFORE,
-				  pTab, onError);
+				  pTab, on_error);
 	for (i = 0; i < pTab->nCol; i++) {
 		if (i == pTab->iPKey) {
 			sqlite3VdbeAddOp2(v, OP_Null, 0, regNew + i);
@@ -526,7 +527,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		sqlite3TableAffinity(v, pTab, regNew);
 		sqlite3CodeRowTrigger(pParse, pTrigger, TK_UPDATE, pChanges,
 				      TRIGGER_BEFORE, pTab, regOldPk,
-				      onError, labelContinue);
+				      on_error, labelContinue);
 
 		/* The row-trigger may have deleted the row being updated. In this
 		 * case, jump to the next row. No updates or AFTER triggers are
@@ -562,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 = on_error;
+		on_conflict.optimized_action = 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);
 
@@ -615,7 +620,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		}
 
 		/* Insert the new index entries and the new record. */
-		vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], onError);
+		vdbe_emit_insertion_completion(v, 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
@@ -634,7 +640,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	}
 
 	sqlite3CodeRowTrigger(pParse, pTrigger, TK_UPDATE, pChanges,
-			      TRIGGER_AFTER, pTab, regOldPk, onError,
+			      TRIGGER_AFTER, pTab, regOldPk, on_error,
 			      labelContinue);
 
 	/* Repeat the above with the next record to be updated, until
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: [PATCH V2] sql: fix non-working REPLACE optimization

[n.pettik]

Hello. I have only minor remarks mostly concerning codestyle.
As for functional part of this patch, it seems to be OK.

>This patch contains fix for that, in addition
>related code was refactored and necessary comments were added.

From this message it is not clear what bug you have fixed.
I guess that this fix is for particular case, when ‘ON CONFLICT REPLACE’
is applied to PK. But it would be better to describe origins of bug in ticket/commit message.

> @@ -1038,10 +1042,12 @@ checkConstraintUnchanged(Expr * pExpr, int *aiChng)
>  *
>  *  CHECK            REPLACE      Illegal.  The results in an exception.
>  *
> - * Which action to take is determined by the overrideError parameter.
> - * Or if overrideError==ON_CONFLICT_ACTION_DEFAULT, then the pParse->onError parameter
> - * is used.  Or if pParse->onError==ON_CONFLICT_ACTION_DEFAULT then the onError value
> - * for the constraint is used.
> + * Which action to take is determined by the override_error parameter

Nitpicking: doesn’t fit into 66 chars.

> 	int seenReplace = 0;	/* True if REPLACE is used to resolve INT PK conflict */
> 	int nPkField;		/* Number of fields in PRIMARY KEY. */
> 	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();
> +	enum on_conflict_action override_error = on_conflict->override_error;
> +	enum on_conflict_action on_error;

Nitpicking: you don’t have to declare variables
beforehand as it happens in C89.

> +		on_error = table_column_nullable_action(pTab, i);
> +		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
> +			on_error = override_error;
> +		} else if (on_error == ON_CONFLICT_ACTION_DEFAULT) {
> +			on_error = ON_CONFLICT_ACTION_ABORT;

Nitpicking: I guess, this else-if is redundant.
It would be enough just to use ‘else’, since you have only two variants:
on_error equals to default action or not.
Moreover, for this reason, on_error = table_column_nullable… -
is redundant function call.

> -		assert(onError == ON_CONFLICT_ACTION_ROLLBACK
> -		       || onError == ON_CONFLICT_ACTION_ABORT
> -		       || onError == ON_CONFLICT_ACTION_FAIL
> -		       || onError == ON_CONFLICT_ACTION_IGNORE
> -		       || onError == ON_CONFLICT_ACTION_REPLACE);

I guess, this assert used to check that on_error action is not NONE.
And since you are using enum, add simple asset(on_error != …_NONE)


> -		if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
> -			onError = overrideError;
> -		} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> -			onError = ON_CONFLICT_ACTION_ABORT;
> +
> +		if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
> +			on_error = override_error;
> +		} else if (on_error == ON_CONFLICT_ACTION_DEFAULT) {
> +			on_error = ON_CONFLICT_ACTION_ABORT;
> 		}

Nitpicking: the same is here: 'else-if’ can be replaced with simple ‘else’.

> 		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 */
> +		    && (on_error == ON_CONFLICT_ACTION_REPLACE ||
> +			on_error == ON_CONFLICT_ACTION_IGNORE)
> +		    /* Condition 3 */
> +		    && (0 == (user_session->sql_flags & SQLITE_RecTriggers)
> 			||0 == sqlite3TriggersExist(pTab, TK_DELETE, 0, 0))

Nitpicking: place space after ||. Overall, this huge ‘if' looks bad-formatted.

> -		/* Generate code that executes if the new index entry is not unique */
> -		assert(onError == ON_CONFLICT_ACTION_ROLLBACK
> -		       || onError == ON_CONFLICT_ACTION_ABORT
> -		       || onError == ON_CONFLICT_ACTION_FAIL
> -		       || onError == ON_CONFLICT_ACTION_IGNORE
> -		       || onError == ON_CONFLICT_ACTION_REPLACE);

The same is here: add simple assert(on_error != ON_CONFLICT_ACTION_NONE).

> void
> -vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, u8 on_error)
> +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
> +			       struct on_conflict *on_conflict)

Update comment at function declaration in sqliteInt.h
Also, you have noticeably changed function’s logic,
so reflect it in comments somewhere.

> -	else
> +	} else {
> 		opcode = OP_IdxInsert;
> +	}
> 

Nitpicking: don’t separate one-line ‘if-else' with brackets.

> 	u16 pik_flags = OPFLAG_NCHANGE;
> -	if (on_error == ON_CONFLICT_ACTION_IGNORE)
> +	if (override_error == ON_CONFLICT_ACTION_IGNORE)
> +		pik_flags |= OPFLAG_OE_IGNORE;
> +	if (override_error == ON_CONFLICT_ACTION_IGNORE ||
> +	    (optimized_action == ON_CONFLICT_ACTION_IGNORE &&
> +	     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
> 		pik_flags |= OPFLAG_OE_IGNORE;

You can merge this ‘if’ and previous into one.

> -	else if (on_error == ON_CONFLICT_ACTION_FAIL)
> +	}
> +	else if (override_error == ON_CONFLICT_ACTION_FAIL) {

Put 'else-if’ on previous line.

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

[Bulat Niatshin]

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

Branch:
https://github.com/tarantool/tarantool/tree/bn/gh-2963-proper-replace

I clearly understand that new branch is different from previous one, but this patch
was necessary for 3293. Because in last review there were only remarks about codestyle,
I decided to use this patch in order to accomplish 3293.


>>This patch contains fix for that, in addition
>>related code was refactored and necessary comments were added.
>
>From this message it is not clear what bug you have fixed.
>I guess that this fix is for particular case, when ‘ON CONFLICT REPLACE’
>is applied to PK. But it would be better to describe origins of bug in ticket/commit message.

Done, new commit message:

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 error action is REPLACE or IGNORE.

Then uniqueness can be ensured by Tarantool facilities, without
VDBE bytecode. However, it didn't work, there was a dublicate key
error during insertion. This patch contains fix for that, in
addition related code was refactored and necessary comments were
added.

>> + * Which action to take is determined by the override_error parameter
>
>Nitpicking: doesn’t fit into 66 chars.

Done.

+ * Which action to take is determined by the override_error
+ * parameter in struct on_conflict.
+ * Or if override_error==ON_CONFLICT_ACTION_DEFAULT, then the
+ * pParse->onError parameter is used. Or if
+ * pParse->onError==ON_CONFLICT_ACTION_DEFAULT then the on_error
+ * value for the constraint is used.

>
>> int seenReplace = 0;/* True if REPLACE is used to resolve INT PK conflict */
>> int nPkField;/* Number of fields in PRIMARY KEY. */
>> 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();
>> +enum on_conflict_action override_error = on_conflict->override_error;
>> +enum on_conflict_action on_error;
>
>Nitpicking: you don’t have to declare variables
>beforehand as it happens in C89.

Done,

+	enum on_conflict_action override_error = on_conflict->override_error;
+	enum on_conflict_action on_error;
 	/* Test all NOT NULL constraints.
 	*/
 	for (i = 0; i < nCol; i++) {
>
>> +on_error = table_column_nullable_action(pTab, i);
>> +if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
>> +on_error = override_error;
>> +} else if (on_error == ON_CONFLICT_ACTION_DEFAULT) {
>> +on_error = ON_CONFLICT_ACTION_ABORT;
>
>Nitpicking: I guess, this else-if is redundant.
>It would be enough just to use ‘else’, since you have only two variants:
>on_error equals to default action or not.
>Moreover, for this reason, on_error = table_column_nullable… -
>is redundant function call.

It is not redundant. If override error is not default, then it should be used
instead of error action for particular NOT NULL/UNIQUE constraint.
ABORT is a default error action, so we should explicitly assign it if
error action in index/column is default. Refactored version:

+	on_error = table_column_nullable_action(pTab, i);
+	if (override_error != ON_CONFLICT_ACTION_DEFAULT)
+	on_error = override_error;
+	/* ABORT is a default error action */
+	if (on_error == ON_CONFLICT_ACTION_DEFAULT)
+	on_error = ON_CONFLICT_ACTION_ABORT;
+

>
>> -assert(onError == ON_CONFLICT_ACTION_ROLLBACK
>> -       || onError == ON_CONFLICT_ACTION_ABORT
>> -       || onError == ON_CONFLICT_ACTION_FAIL
>> -       || onError == ON_CONFLICT_ACTION_IGNORE
>> -       || onError == ON_CONFLICT_ACTION_REPLACE);
>
>I guess, this assert used to check that on_error action is not NONE.
>And since you are using enum, add simple asset(on_error != …_NONE)

-	assert(onError == ON_CONFLICT_ACTION_ROLLBACK
-	|| onError == ON_CONFLICT_ACTION_ABORT
-	|| onError == ON_CONFLICT_ACTION_FAIL
-	|| onError == ON_CONFLICT_ACTION_IGNORE
-	|| onError == ON_CONFLICT_ACTION_REPLACE);
-	switch (onError) {
+	assert(on_error != ON_CONFLICT_ACTION_NONE);
+	switch (on_error) {

>> -if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
>> -onError = overrideError;
>> -} else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
>> -onError = ON_CONFLICT_ACTION_ABORT;
>> +
>> +if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
>> +on_error = override_error;
>> +} else if (on_error == ON_CONFLICT_ACTION_DEFAULT) {
>> +on_error = ON_CONFLICT_ACTION_ABORT;
>> }
>
>Nitpicking: the same is here: 'else-if’ can be replaced with simple ‘else’.

Same, see changes above.

>
>> 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 */
>> +    && (on_error == ON_CONFLICT_ACTION_REPLACE ||
>> +on_error == ON_CONFLICT_ACTION_IGNORE)
>> +    /* Condition 3 */
>> +    && (0 == (user_session->sql_flags & SQLITE_RecTriggers)
>> ||0 == sqlite3TriggersExist(pTab, TK_DELETE, 0, 0))
>
>Nitpicking: place space after ||. Overall, this huge ‘if' looks bad-formatted.

+	/*
+	* 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 */
-	||0 == sqlite3TriggersExist(pTab, TK_DELETE, 0, 0))
-	&& (0 == (user_session->sql_flags & SQLITE_ForeignKeys) ||	/* Condition 4 */
-	(0 == pTab->pFKey && 0 == sqlite3FkReferences(pTab)))
-	) {
+	bool no_secondary_indexes = (ix == 0 &&
+	pIdx->pNext == 0);
+	bool proper_error_action =
+	(on_error == ON_CONFLICT_ACTION_REPLACE ||
+	on_error == ON_CONFLICT_ACTION_IGNORE);
+	bool no_delete_triggers =
+	(0 == (user_session->sql_flags &
+	SQLITE_RecTriggers) ||
+	0 == sqlite3TriggersExist(pTab,
+	TK_DELETE,
+	0, 0));
+	bool no_foreign_keys =
+	(0 == (user_session->sql_flags &
+	SQLITE_ForeignKeys) ||
+	(0 == pTab->pFKey &&
+	0 == sqlite3FkReferences(pTab)));
+
+	if (no_secondary_indexes && no_foreign_keys &&
+	proper_error_action && no_delete_triggers) {
+	/*
+	* Save that possible optimized error
+	* action, which can be used later
+	* during execution of
+	* vdbe_emit_insertion_completion().
+	*/
+	on_conflict->optimized_action = on_error;
 	sqlite3VdbeResolveLabel(v, addrUniqueOk);
 	continue;
 	}


>
>> -/* Generate code that executes if the new index entry is not unique */
>> -assert(onError == ON_CONFLICT_ACTION_ROLLBACK
>> -       || onError == ON_CONFLICT_ACTION_ABORT
>> -       || onError == ON_CONFLICT_ACTION_FAIL
>> -       || onError == ON_CONFLICT_ACTION_IGNORE
>> -       || onError == ON_CONFLICT_ACTION_REPLACE);
>
>The same is here: add simple assert(on_error != ON_CONFLICT_ACTION_NONE).

Done,
+	assert(on_error != ON_CONFLICT_ACTION_NONE);
+	switch (on_error) {


>
>> void
>> -vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, u8 on_error)
>> +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
>> +       struct on_conflict *on_conflict)
>
>Update comment at function declaration in sqliteInt.h
>Also, you have noticeably changed function’s logic,
>so reflect it in comments somewhere.

 /**
  * This routine generates code to finish the INSERT or UPDATE
  * operation that was started by a prior call to
@@ -3691,11 +3718,13 @@ void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
  * @param v Virtual database engine.
  * @param cursor_id Primary index cursor.
  * @param tuple_id Register with data to insert.
- * @param on_error Error action on failed insert/replace.
+ * @param on_conflict Structure, which contains override error
+ * action on failed insert/replaceand and optimized error
+ * action after generating bytecode for constraints checks.
  */
 void
 vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
-	u8 on_error);
+	struct on_conflict *on_conflict);


>
>> -else
>> +} else {
>> opcode = OP_IdxInsert;
>> +}
>> 
>
>Nitpicking: don’t separate one-line ‘if-else' with brackets.

Done,
+	override_error == ON_CONFLICT_ACTION_DEFAULT))
 	opcode = OP_IdxReplace;
 	else
 	opcode = OP_IdxInsert;

>
>> u16 pik_flags = OPFLAG_NCHANGE;
>> -if (on_error == ON_CONFLICT_ACTION_IGNORE)
>> +if (override_error == ON_CONFLICT_ACTION_IGNORE)
>> +pik_flags |= OPFLAG_OE_IGNORE;
>> +if (override_error == ON_CONFLICT_ACTION_IGNORE ||
>> +    (optimized_action == ON_CONFLICT_ACTION_IGNORE &&
>> +     override_error == ON_CONFLICT_ACTION_DEFAULT)) {
>> pik_flags |= OPFLAG_OE_IGNORE;
>
>You can merge this ‘if’ and previous into one.

 	u16 pik_flags = OPFLAG_NCHANGE;
-	if (on_error == ON_CONFLICT_ACTION_IGNORE)
+	if (override_error == ON_CONFLICT_ACTION_IGNORE)
+	pik_flags |= OPFLAG_OE_IGNORE;
+	else if (override_error == ON_CONFLICT_ACTION_IGNORE ||
+	(optimized_action == ON_CONFLICT_ACTION_IGNORE &&
+	override_error == ON_CONFLICT_ACTION_DEFAULT))
 	pik_flags |= OPFLAG_OE_IGNORE;
-	else if (on_error == ON_CONFLICT_ACTION_FAIL)
+	else if (override_error == ON_CONFLICT_ACTION_FAIL)
 	pik_flags |= OPFLAG_OE_FAIL;
>
>> -else if (on_error == ON_CONFLICT_ACTION_FAIL)
>> +}
>> +else if (override_error == ON_CONFLICT_ACTION_FAIL) {
>
>Put 'else-if’ on previous line.

Brackets were removed, so that remarks is useless now.

--