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

["n.pettik" <korablev@tarantool.org>]

[-- 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 --]