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 9FE102D412 for ; Thu, 5 Apr 2018 11:31:23 -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 l69fgOcUHk4k for ; Thu, 5 Apr 2018 11:31:23 -0400 (EDT) Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 287DB2D2EB for ; Thu, 5 Apr 2018 11:31:22 -0400 (EDT) Content-Type: multipart/alternative; boundary="Apple-Mail=_2053A6EE-2D93-40BA-9F3B-9CA399A2C764" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [patches] [PATCH] sql: fix non-working REPLACE optimization From: "n.pettik" In-Reply-To: <1522831865.978729858@f430.i.mail.ru> Date: Thu, 5 Apr 2018 18:31:20 +0300 Message-Id: References: <1522831865.978729858@f430.i.mail.ru> 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 --Apple-Mail=_2053A6EE-2D93-40BA-9F3B-9CA399A2C764 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Consider formatting: it is almost unreadable. Don=E2=80=99t add extra = tabs to quote. Also, I don=E2=80=99t see provided changes: you should attach them to = your letter, send in reply to this letter or send patch ver.2. I can=E2=80=99t give you any review, just because there is no subject of = it. Please, rebase on fresh 2.0 and send diff or whole patch again. Thanks. > On 4 Apr 2018, at 11:51, Bulat Niatshin = wrote: >=20 >=20 > Branch: > https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt = =20 >=20 > Issue: > https://github.com/tarantool/tarantool/issues/3293 = =20 >=20 > 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 errt or action is REPLACE or IGNORE. >=20 > errt: typo. >=20 > Done. >=20 > Then uniqueness can be ensured by Tarantool only, without VDBE > bytecode. This patch contains a small fix for that + >=20 > Replace =E2=80=98+=E2=80=99 with word. Don=E2=80=99t mention that it = is =E2=80=99small=E2=80=99 fix, let it be just fix. >=20 > Done. >=20 > related code was refactored a little bit and necessary comments >=20 > The same: don=E2=80=99t use =E2=80=98little bit=E2=80=99 =E2=80=94 it = sounds like you left smth > in a shitty state consciously. >=20 > Done. >=20 > +/* > + * ABORT and DEFAULT error actions can be handled > + * by Tarantool only without emitting VDBE >=20 > Remove word =E2=80=98only=E2=80=99: it confuses little bit (IMHO). > Done. >=20 > + > +/* > + * Find out what action to take in case there is > + * a uniqueness conflict. > + */ > +onError =3D pIdx->onError; > + >=20 > Nit-picking: don=E2=80=99t put too many empty lines. > In this particular case, you outline very elementary statement. >=20 > Done. >=20 > +/* > + * override_error is an action which is directly > + * specified by user in queries like > + * INSERT OR and therefore > + * should have higher priority than indexes > + * error actions. > + */ >=20 > You put almost the same comment to on_error struct. > Change it to more informative or remove. >=20 > Done, this comment was removed. >=20 > +int override_error =3D on_conflict->override_error; > +int optimized_error_action =3D on_conflict->optimized_on_conflict; >=20 > If you used enum type, you wouldn=E2=80=99t have to add assertion = below. >=20 > Done, all error actions are enums right now. >=20 > +/* > + * override_error represents error action in queries like > + * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE, > + * which is strictly specified by user and therefore > + * should have been used even when optimization is > + * possible (uniqueness can be checked by Tarantool). > + */ >=20 > This comment is almost copy of one from struct declaration.=20 > Remove or rephrase. >=20 > Done, removed. >=20 > +struct on_conflict { > +/** > + * Represents an error action in queries like > + * INSERT/UPDATE OR , which > + * overrides all space constraints error actions. > + */ > +int override_error; >=20 > Why do you declare type of this field as int, > when it can take values from on_conflict_action enum? >=20 > Done. >=20 > + * the value is ON_CONFLICT_ACTION_NONE, otherwise it is > + * ON_CONFLICT_ACTON_IGNORE or ON_CONFLICT_ACTION_REPLACE. > + */ > +int optimized_on_conflict; >=20 > Your structure is already called =E2=80=98on_conflict=E2=80=99, > so don=E2=80=99t repeat this phrase in field=E2=80=99s name. > Also, the same as previous field: better declare it as enum. >=20 > Done. >=20 >=20 >=20 >=20 >=20 >=20 > --=20 > Bulat Niatshin --Apple-Mail=_2053A6EE-2D93-40BA-9F3B-9CA399A2C764 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
Consider formatting: it is almost unreadable. = Don=E2=80=99t add extra tabs to quote.
Also, I = don=E2=80=99t see provided changes: you should attach them to your = letter,
send in reply to this letter or send patch = ver.2.
I can=E2=80=99t give you any review, just = because there is no subject of it.

Please, rebase on fresh 2.0 and send = diff or whole patch again.
Thanks.

On 4 Apr 2018, at 11:51, Bulat Niatshin <niatshin@tarantool.org> wrote:


Branch:
https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-o= pt 

Issue:
https://github.com/tarantool/tarantool/issues/3293 

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 errt or action is REPLACE or IGNORE.

errt: typo.

Done.

Then uniqueness can be ensured by = Tarantool only, without VDBE
bytecode. This patch contains = a small fix for that +

Replace =E2=80=98+=E2=80=99 with = word. Don=E2=80=99t mention that it is =E2=80=99small=E2=80=99 fix, let = it be just fix.

Done.

related code was = refactored a little bit and necessary comments

The same: don=E2=80=99t use =E2=80=98little bit=E2=80=99 =E2=80= =94 it sounds like you left smth
in a shitty state = consciously.

Done.

+/*
+ = * ABORT and DEFAULT error actions can be handled
+ = * by Tarantool only without emitting VDBE

Remove word =E2=80=98only=E2=80=99: it confuses little bit = (IMHO).
Done.

+
+/*
+ = * Find out what action to take in case there is
+ = * a uniqueness conflict.
+ = */
+onError =3D pIdx->onError;
+

Nit-picking: don=E2=80=99t put too many empty = lines.
In this particular case, you outline very elementary = statement.

=
Done.

+/*
+ = * override_error is an action which is directly
+ = * specified by user in queries like
+ = * INSERT OR <override_error> and therefore
+ = * should have higher priority than indexes
+ = * error actions.
+ = */

You put almost the same comment to on_error struct.
Change it to more informative or = remove.

Done, this comment was = removed.

+int override_error =3D = on_conflict->override_error;
+int = optimized_error_action =3D = on_conflict->optimized_on_conflict;

If you used enum type, you wouldn=E2=80=99t have to add = assertion below.

Done, all error = actions are enums right now.

+/*
+ = * override_error represents error action in queries like
+ = * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE,
+ = * which is strictly specified by user and therefore
+ = * should have been used even when optimization is
+ = * possible (uniqueness can be checked by Tarantool).
+ = */

This comment is almost copy of one from struct = declaration. =
Remove or rephrase.

Done, removed.

+struct on_conflict {
+/**
+ = * Represents an error action in queries like
+ = * INSERT/UPDATE OR <override_error>, which
+ = * overrides all space constraints error actions.
+ = */
+int override_error;

Why do you declare type of this field as int,
when it can take values from on_conflict_action = enum?

Done.

+ = * the value is ON_CONFLICT_ACTION_NONE, otherwise it is
+ = * ON_CONFLICT_ACTON_IGNORE or ON_CONFLICT_ACTION_REPLACE.
+ = */
+int optimized_on_conflict;

Your structure is already called =E2=80=98on_conflict=E2=80=99,=
so don=E2=80=99t repeat this phrase in field=E2=80=99s = name.
Also, the same as previous field: better declare it as = enum.

Done.





--
Bulat Niatshin

= --Apple-Mail=_2053A6EE-2D93-40BA-9F3B-9CA399A2C764--