Consider formatting: it is almost unreadable. Don’t add extra tabs to quote. Also, I don’t see provided changes: you should attach them to your letter, send in reply to this letter or send patch ver.2. I can’t 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: > > > Branch: > https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt > > 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 ‘+’ with word. Don’t mention that it is ’small’ fix, let it be just fix. > > Done. > > related code was refactored a little bit and necessary comments > > The same: don’t use ‘little bit’ — 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 ‘only’: it confuses little bit (IMHO). > Done. > > + > +/* > + * Find out what action to take in case there is > + * a uniqueness conflict. > + */ > +onError = pIdx->onError; > + > > Nit-picking: don’t 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 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 = on_conflict->override_error; > +int optimized_error_action = on_conflict->optimized_on_conflict; > > If you used enum type, you wouldn’t 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 , 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 ‘on_conflict’, > so don’t repeat this phrase in field’s name. > Also, the same as previous field: better declare it as enum. > > Done. > > > > > > > -- > Bulat Niatshin