[tarantool-patches] [tarantool-patches] Re: [patches] [PATCH] sql: fix non-working REPLACE optimization
Bulat Niatshin
niatshin at tarantool.org
Wed Apr 4 11:51:05 MSK 2018
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 <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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180404/e10d5735/attachment.html>
More information about the Tarantool-patches
mailing list