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

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: Bulat Niatshin <niatshin@xxxxxxxxxxxxx>
  • Date: Mon, 9 Apr 2018 15:03:09 +0300

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.



Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 04-2018