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.
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 smthin 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 <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 = 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 <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 ‘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