[tarantool-patches] Re: [PATCH V2] sql: fix non-working REPLACE optimization
n.pettik
korablev at tarantool.org
Mon Apr 9 15:03:09 MSK 2018
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.
More information about the Tarantool-patches
mailing list