From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A13C72E5EA for ; Thu, 23 May 2019 06:37:55 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id w51Q9I0HQ8D7 for ; Thu, 23 May 2019 06:37:55 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5CB782E51E for ; Thu, 23 May 2019 06:37:55 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 2/4] box: run check constraint tests on space alter References: <9dfc18b3-8566-8b25-3c08-c94d83849f5f@tarantool.org> From: Kirill Shcherbatov Message-ID: <0d2a7918-7435-22b6-32d1-9dc76699636b@tarantool.org> Date: Thu, 23 May 2019 13:37:53 +0300 MIME-Version: 1.0 In-Reply-To: <9dfc18b3-8566-8b25-3c08-c94d83849f5f@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Vladislav Shpilevoy 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.