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

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

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.