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

["Bulat Niatshin" <niatshin@tarantool.org>]

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.

--