From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 34E852D131 for ; Mon, 9 Apr 2018 08:03:13 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zDzhMnYhaDXI for ; Mon, 9 Apr 2018 08:03:13 -0400 (EDT) Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id A6C8C2D12E for ; Mon, 9 Apr 2018 08:03:12 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH V2] sql: fix non-working REPLACE optimization From: "n.pettik" In-Reply-To: <20180409051748.91507-1-niatshin@tarantool.org> Date: Mon, 9 Apr 2018 15:03:09 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <9A6F0200-0C9B-41A8-8184-FFA395864139@tarantool.org> References: <20180409051748.91507-1-niatshin@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Bulat Niatshin Cc: tarantool-patches@freelists.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. =46rom this message it is not clear what bug you have fixed. I guess that this fix is for particular case, when =E2=80=98ON CONFLICT = REPLACE=E2=80=99 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=3D=3DON_CONFLICT_ACTION_DEFAULT, then the = pParse->onError parameter > - * is used. Or if pParse->onError=3D=3DON_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=E2=80=99t fit into 66 chars. > int seenReplace =3D 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 =3D 0; /* True if the OP_Affinity operation has = been run */ > struct session *user_session =3D current_session(); > + enum on_conflict_action override_error =3D = on_conflict->override_error; > + enum on_conflict_action on_error; Nitpicking: you don=E2=80=99t have to declare variables beforehand as it happens in C89. > + on_error =3D table_column_nullable_action(pTab, i); > + if (override_error !=3D ON_CONFLICT_ACTION_DEFAULT) { > + on_error =3D override_error; > + } else if (on_error =3D=3D ON_CONFLICT_ACTION_DEFAULT) { > + on_error =3D ON_CONFLICT_ACTION_ABORT; Nitpicking: I guess, this else-if is redundant. It would be enough just to use =E2=80=98else=E2=80=99, since you have = only two variants: on_error equals to default action or not. Moreover, for this reason, on_error =3D table_column_nullable=E2=80=A6 - is redundant function call. > - assert(onError =3D=3D ON_CONFLICT_ACTION_ROLLBACK > - || onError =3D=3D ON_CONFLICT_ACTION_ABORT > - || onError =3D=3D ON_CONFLICT_ACTION_FAIL > - || onError =3D=3D ON_CONFLICT_ACTION_IGNORE > - || onError =3D=3D 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 !=3D =E2=80=A6_NON= E) > - if (overrideError !=3D ON_CONFLICT_ACTION_DEFAULT) { > - onError =3D overrideError; > - } else if (onError =3D=3D ON_CONFLICT_ACTION_DEFAULT) { > - onError =3D ON_CONFLICT_ACTION_ABORT; > + > + if (override_error !=3D ON_CONFLICT_ACTION_DEFAULT) { > + on_error =3D override_error; > + } else if (on_error =3D=3D ON_CONFLICT_ACTION_DEFAULT) { > + on_error =3D ON_CONFLICT_ACTION_ABORT; > } Nitpicking: the same is here: 'else-if=E2=80=99 can be replaced with = simple =E2=80=98else=E2=80=99. > if ((ix =3D=3D 0 && pIdx->pNext =3D=3D 0) /* = Condition 2 */ > - && onError =3D=3D ON_CONFLICT_ACTION_REPLACE = /* Condition 1 */ > - && (0 =3D=3D (user_session->sql_flags & = SQLITE_RecTriggers) /* Condition 3 */ > + /* Condition 1 */ > + && (on_error =3D=3D ON_CONFLICT_ACTION_REPLACE || > + on_error =3D=3D ON_CONFLICT_ACTION_IGNORE) > + /* Condition 3 */ > + && (0 =3D=3D (user_session->sql_flags & = SQLITE_RecTriggers) > ||0 =3D=3D sqlite3TriggersExist(pTab, TK_DELETE, = 0, 0)) Nitpicking: place space after ||. Overall, this huge =E2=80=98if' looks = bad-formatted. > - /* Generate code that executes if the new index entry is = not unique */ > - assert(onError =3D=3D ON_CONFLICT_ACTION_ROLLBACK > - || onError =3D=3D ON_CONFLICT_ACTION_ABORT > - || onError =3D=3D ON_CONFLICT_ACTION_FAIL > - || onError =3D=3D ON_CONFLICT_ACTION_IGNORE > - || onError =3D=3D ON_CONFLICT_ACTION_REPLACE); The same is here: add simple assert(on_error !=3D = 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=E2=80=99s logic, so reflect it in comments somewhere. > - else > + } else { > opcode =3D OP_IdxInsert; > + } >=20 Nitpicking: don=E2=80=99t separate one-line =E2=80=98if-else' with = brackets. > u16 pik_flags =3D OPFLAG_NCHANGE; > - if (on_error =3D=3D ON_CONFLICT_ACTION_IGNORE) > + if (override_error =3D=3D ON_CONFLICT_ACTION_IGNORE) > + pik_flags |=3D OPFLAG_OE_IGNORE; > + if (override_error =3D=3D ON_CONFLICT_ACTION_IGNORE || > + (optimized_action =3D=3D ON_CONFLICT_ACTION_IGNORE && > + override_error =3D=3D ON_CONFLICT_ACTION_DEFAULT)) { > pik_flags |=3D OPFLAG_OE_IGNORE; You can merge this =E2=80=98if=E2=80=99 and previous into one. > - else if (on_error =3D=3D ON_CONFLICT_ACTION_FAIL) > + } > + else if (override_error =3D=3D ON_CONFLICT_ACTION_FAIL) { Put 'else-if=E2=80=99 on previous line.