[tarantool-patches] Re: [PATCH v4 2/4] box: run check constraint tests on space alter
Kirill Shcherbatov
kshcherbatov at tarantool.org
Thu May 23 13:37:53 MSK 2019
On 19.05.2019 19:02, Vladislav Shpilevoy wrote:
> Thanks for the patch! See 18 comments below.
>
> On 16/05/2019 16:56, Kirill Shcherbatov wrote:
>> To perform ck constraints tests before insert or update space operation,
>> we use precompiled VDBE machine is associated with each ck constraint,
>
> 1. 'we use machine is associated' - ??? Can't parse, please, rephrase.
> I see that error quite often in your English. Looks, like you don't know,
> that verbs can be adjectives.
Thank you.
>> that is executed in on_replace trigger. Each ck constraint VDBE code is
>> consists of
>
> 2. 'is consists' - ??? 'Consist' is a verb, you can't say that.
Fixed.
>
>> 1) prologue code that maps new(or updated) tuple fields via bindings,
>> 2) ck constraint code is generated by CK constraint AST.
>> In case of ck constraint error the transaction is aborted and ck
>
> 3. Why do you abort a transaction because of just one bad DML operation?
> A failed operation should not lead to rollback of the entire transaction.
It is not so actually. In fact, I just abort the current replace/insertion.
> 4. It is too expensive, triggers are virtual functions. Please, call
> them all from one. Also, it is necessary to be able to turn them
> off - you will just remove this single trigger.
Done. Now I use the only trigger.
> 5. When 'later'? This function (alter) is called already in
> alter_space_do().
Outdated.
>> +/**
>> + * Move CK constraints from old space to the new one.
>> + * Despite RebuildCkConstraints, this operation doesn't perform
>> + * objects rebuild.
>
> 6. 'Despite' does not mean the thing you wanted to say here. Please,
> rephrase.
Unlike actually.
>> - (void) new RebuildCkConstraints(alter);
>> + (void) new MoveCkConstraints(alter);
>
> 7. Why is not MoveCkConstraints in the previous patch?
Discussed verbally. New ck constraint object doesn't store
Expr AST so we may do not rebuild an object. Previously
there were space_def pointers that become outdated.
>> const char *ck_constraint_language_strs[] = {"SQL"};
>
> 8. Do not see that on the branch.
See the gopher? And he is =)
Maybe I didn't updated the branch, but I am not sure about it
because this change is old enough.
>> + * @param expr Check constraint expression AST is built for
>
> 9. When you say 'AST is built ...', I can't understand, how this
> sentence relates to expr. If you wanted to describe 'expr', then
> remove 'is'. See comment 1.
Ok...
>> + * given @ck_constraint_def, see for
>
> 10. Doxygen does not have '@ck_constraint_def' command. Please, see
> the list of commands and do not use not existing. Here you
> should use '@a ck_constraint'.
FIxed, thank you.
>> +/**
>> + * Run bytecode implementing check constraint on new tuple
>> + * before insert or replace in space space_def.
>
> 11. 'space space_def' ???
Outdated.
>
>> + * @param ck_constraint Check constraint to run.
>> + * @param space_def The space definition of the space this check
>> + * constraint is constructed for.
>
> 12. There is no parameter named 'space_def'.
Fixed.
>
>> + * @param new_tuple The tuple to be inserted in space.
>> + * @retval 0 if check constraint test is passed, -1 otherwise.
>
> 13. As I said earlier, multiple times, @retval is not designed to
> describe multiple values. @retval means 'return value', one value.
> Not 'valueS'. Use one @retval for each single value, or use
> @return, which does not require an argument and is used to describe
> return values in general.
Ok...
>> + /*
>> + * Prepare parameters for checks->stmt execution:
>
> 14. What is 'checks' here? I do not see such a variable here to
> dereference it.
Outdated.
>> + while (sql_step(ck_constraint->stmt) == SQL_ROW) {}
>
> 15. Why do you iterate? Only DQL has multiple steps, because
> it returns tuples. DML, DDL and any other statement can't
> return multiple tuples.
You are right. It is redundant.
>> +/**
>> + * Ck constraint trigger function. It ss expected to be executed
>
> 16. 'ss' -> 'is'.
Fixed.
> 17. SQL_CONSTRAINT_CHECK is unused now. P5_ConstraintCheck too.
Done.
>> +void
>> +vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
>> + const char *name, const char *expr_str,
>
> 18. expr_str is not described.
Fixed.
More information about the Tarantool-patches
mailing list