[tarantool-patches] Re: [PATCH v4 2/4] box: run check constraint tests on space alter

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

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.